#719 ✓ staged
brianchirls

Safely wrap calls to plugin functions in try/catch to prevent crashes

Reported by brianchirls | September 13th, 2011 @ 05:52 PM | in 0.9 (closed)

I recommend wrapping all calls to plugin functions (start, end, frame, _teardown, _setup) in try/catch. Since these functions are not called inside their own events, if one breaks, it kills the rest of the update loop.

Errors should be reported to the console to assist in debugging.

We may want to hold off on this one until #641 and #703 are resolved, since they will likely require a significant amount of work around timeUpdate.

Comments and changes to this ticket

  • Rick

    Rick September 13th, 2011 @ 07:28 PM

    I'm strongly opposed to stifling errors with try/catch, but I want to hear some other thoughts

  • brianchirls

    brianchirls September 13th, 2011 @ 07:42 PM

    I assume your objection is because catching errors prevents developers from seeing them and therefore correcting them. Correct?

    This is a good point and one that I've considered. My concern is with errors that are not caught in early development and testing, but then show up in production or beta tests. In those cases, it's best to protect the interests of the user/viewer by allowing the other plugins to continue.

    Logging errors to the console is a good compromise. Experienced developers who are paying close attention will keep an eye on the console during development and testing, and we'll have unit tests for officially packaged plugins.

    When a non-developer (e.g. a client or designer) is reporting an error, they're not going to check the console anyway. But at least they'll be able to report that a specific plugin is not showing up properly, rather than that the whole thing just stopped.

  • Rick

    Rick September 13th, 2011 @ 07:53 PM

    I'd like Scott, Dave and Chris to weigh in on this - the compromise might be doable

  • cadecairos

    cadecairos September 14th, 2011 @ 10:13 AM

    I'm not opposed to catching errors from plugin method calls. I can see how it would be useful in identifying exactly which plug-in killed the timeupdate loop.

  • Rick

    Rick September 14th, 2011 @ 11:59 AM

    • State changed from “new” to “peer-review-requested”
    • Assigned user set to “cadecairos”
    • Milestone set to 0.9
    • Milestone order changed from “79” to “0”

    https://github.com/rwldrn/popcorn-js/tree/t719

    Adds Popcorn.plugin.errors array for storing errors thrown by plugin functions, along with the source of the function that caused the error. Adds safeTry() function to wrap all plugin functions in a try/catch safety wrapper

  • Rick

    Rick September 14th, 2011 @ 12:01 PM

    (from [4e3b52ad230ed77f9544a72ed5049f25434e1013]) [#719] Adds Popcorn.plugin.errors array for storing errors thrown by plugin functions, along with the source of the function that caused the error. Adds safeTry() function to wrap all plugin functions in a try/catch safety wrapper https://github.com/rwldrn/popcorn-js/commit/4e3b52ad230ed77f9544a72...

  • Rick
  • Rick

    Rick September 14th, 2011 @ 12:15 PM

    • Assigned user changed from “cadecairos” to “David Seifried”
  • Rick

    Rick September 22nd, 2011 @ 11:56 AM

    • Assigned user changed from “David Seifried” to “cadecairos”
  • cadecairos

    cadecairos September 22nd, 2011 @ 02:40 PM

    • State changed from “peer-review-requested” to “super-review-requested”
    • Assigned user changed from “cadecairos” to “David Seifried”

    Had trouble initially with a failing (leaking?) var or test... But rick fixed it so:

    Tested the core unit test suite on:

    Firefox 6, 7, Aurora
    Chrome 13

    All passed.

    Lint passes.

    PR+

  • David Seifried

    David Seifried September 22nd, 2011 @ 05:17 PM

    • State changed from “super-review-requested” to “review-needs-work”
    • Assigned user changed from “David Seifried” to “Rick”

    Im getting fails on FF 6 for this, mind taking a look at it now Rick, maybe some of the stuff that has been merged into develop today is causing weird behaviour or something.

  • Rick

    Rick September 22nd, 2011 @ 08:39 PM

    • State changed from “review-needs-work” to “super-review-requested”
    • Assigned user changed from “Rick” to “David Seifried”

    Merged with develop no problems at all: http://gyazo.com/50e967a6a19dfed35024f42c8d797c56.png

  • Scott Downe

    Scott Downe September 23rd, 2011 @ 08:36 AM

    I'm not convinced we need this.

    While I agree particularly complex plugins may benefit from this, our current plugins don't. There is nothing stopping someone from using try/catch inside their own plugin's methods, and handle it uniquely to that case, but that doesn't mean every plugin needs that layer.

    Those functions are essentially callbacks, so I ask how do other libraries handle callbacks? Is this a common way to handle callbacks?

    Also, I've been thinking about popcorn events being tied to the video. They live and die with the video, and the video lives and dies with them. They should be part of the whole experience, as if they were part of the frame itself. We've talked about best practice in plugins that will pause the video on their start if content is still being loaded, then once loaded, continue to play.

    I see this as the same thing. Best practice for complex plugins that can bring down all of popcorn. Not all plugins have loading issues, thus not all plugins need to pause the video.

    My mind may be changed if this becomes a growing problem, but as it stands, I have not experienced this. Then again, as a dev, errors are to be expected, so maybe I'm not the best to comment on this.

    I want more opinions on this.

    If we do go ahead with this, I think we should also log the data about that track (in time, out time, text, ect). Would be good to see which event killed it, as it might not be the function itself, but a function reacting to broken data.

  • brianchirls

    brianchirls September 23rd, 2011 @ 09:43 AM

    Logging track data is an excellent idea. But I would be careful about logging every single field. There might be a big, fat array or a giant html blob that would look pretty awful in a console. Maybe log just the plugin name and start/end times.

  • Rick

    Rick September 23rd, 2011 @ 10:15 AM

    • State changed from “super-review-requested” to “assigned”
    • Assigned user changed from “David Seifried” to “Rick”

    After reading through Scott's comments above, I'm questioning the need for this as well. If a plugin is well tested, then there shouldn't be a need for re-wrapping all of its callbacks in another function and a try/catch.

    Also, I think this is actually a really good use case for Scott's composable plugins system

  • cadecairos

    cadecairos September 23rd, 2011 @ 10:17 AM

    • State changed from “assigned” to “super-review-requested”
    • Assigned user changed from “Rick” to “David Seifried”

    I was thinking about this on the way to work and I realized that with the debug flag in popcorn we might want to approach this differently.

    Would it be sane to have the callbacks wrapped in try-catch blocks only if the debug flag is true? this way, plug-ins won't have to build in debug modes anymore. Then again, most of the current plug-ins in the repo already have debug built into them. In the end this would be an easier way to have popcorn put into a debug mode. What do you think?

  • cadecairos

    cadecairos September 23rd, 2011 @ 10:18 AM

    • State changed from “super-review-requested” to “assigned”
    • Assigned user changed from “David Seifried” to “Rick”

    whoops

  • brianchirls

    brianchirls September 23rd, 2011 @ 10:24 AM

    The point of this is to catch errors that happen outside normal debugging. So having this work when debug is true defeats the purpose. If debug is set to true, we can assume there is a developer tracing errors, so we don't need this at all in these cases.

    Not all plugin developers are so thorough as to build extensive unit tests, and not all unit tests are perfect. This is for those cases.

  • cadecairos

    cadecairos September 23rd, 2011 @ 10:48 AM

    I find myself a bit divided here.

    On the one hand, we do want someone who's experiencing a popcorn page to not have it die on them because of one bad plug-in argument. On the other hand, it just seems unnecessary to have this done by wrapping every plug-in callback in a function with a try-catch. I worry that it might become a performance issue in larger projects using popcorn (or is the overhead for doubling function calls not that great? I'm not entirely sure.).

    Maybe this is a plug-in best practices issue, where a plug-in developer should have the insight to have their plug-in fail "gracefully"? If that were the case then we wouldn't really have to worry about this.

    Rick's solution of having it as an option for plug-ins using compose is a ideal middle ground.

  • Scott Downe

    Scott Downe September 23rd, 2011 @ 11:01 AM

    I think chris' idea with debug is right, he just got it backwards. try/catch is to suppress errors from console, and allow Popcorn to continue running. So turning debug to true would turn off the try/catch.

  • brianchirls

    brianchirls September 23rd, 2011 @ 11:03 AM

    We can do a little performance research:
    http://jsperf.com/try-catch-slow-down/2

    try/catch appears to make almost no difference in Firefox. If you glance quickly at the charts, it looks like it makes a huge difference in Chrome and IE9. Indeed, the tested operation (var a = 1;) is 74% slower inside a try/catch structure. But if you calculate the amount of time try/catch adds to each operation, it comes out to about 4x10^-6 milliseconds. I think we can spare it.

    Hooray for math!

  • Jon Buckley

    Jon Buckley September 23rd, 2011 @ 11:03 AM

    I wrote up a quick performance test on http://jsperf.com/try-perf that does a simple add loop, and adding a try catch statement is not so bad in Firefox, but kills perf in all the other browsers. A few months ago, Dave Mandelin @ Mozilla did a presentation on browser performance, and mentions try...catch specifically as a perf killer: http://people.mozilla.com/~dmandelin/KnowYourEngines_Velocity2011.pdf

    So, avoid try...catch as much as we can!

  • brianchirls

    brianchirls September 23rd, 2011 @ 11:04 AM

    @scott, in that case, I totally agree.

    Have you guys been talking about adding a debug flag elsewhere?

  • Scott Downe

    Scott Downe September 23rd, 2011 @ 11:12 AM

    Considering the performance hit, both Jon and Brian lay it out with different views. To me, this is just one more reason why we should leave this choice in the hands of the plugin author.

  • brianchirls

    brianchirls September 23rd, 2011 @ 11:25 AM

    Nice seeing all you guys over at jsperf.

    Jon's test and the one I cited show very similar results - they are, after all, very similar code.

    These tests show very large percentage differences because the code in the middle is so short. Watch what happens when we do more stuff inside the try/catch.
    http://jsperf.com/try-perf/5

    SPOILER: the percentage differences go away.

  • Rick

    Rick September 23rd, 2011 @ 11:35 AM

    This more accurately illustrates the perf hit that popcorn core will take if it implements broad try/catch wrapping on all plugin callback methods:

    http://jsperf.com/try-perf/6

  • Scott Downe

    Scott Downe September 23rd, 2011 @ 11:40 AM

    The fact that larger more complex blocks are not so bad, is another reason why we leave this up to the plugin author.

  • brianchirls

    brianchirls September 23rd, 2011 @ 11:50 AM

    Even with Rick's added overhead, the difference still comes down to about 3x10^-5 milliseconds per operation (in Chrome). That's 30 nanoseconds. You could do almost a thousand of these in the time it takes to play a two-byte sample of an audio file.

  • Scott Downe

    Scott Downe September 23rd, 2011 @ 01:05 PM

    The performance really is that small of a hit?

    Hm, I think I am back on the fence with this.

    I still feel, performance issues aside, it has pros and cons.

    Pros:
    larger or bugged plugins won't bring down popcorn.
    Developer friendly in that plugin authors don't have to worry about errors.

    Cons:
    Plugin developers need to know this exists, if not, they can get quite frustrated. Documentation issue. A simple API is an easier API.
    Bloat. Maintenance. Surface areas for bugs. (this is all one con)
    Promotes errors in code.

    That's all I got for now.

  • David Humphrey

    David Humphrey September 26th, 2011 @ 03:15 PM

    • State changed from “assigned” to “review-needs-work”

    I'm in favour of us putting some defenses around the timeUpdate loop, such that a rogue plugin, network outage, or some other mishap doesn't kill the whole app. The use of safeTry() is what we want here. The only thing I'd like to see added is a callback (with no args) indicating that there was an error, and checking the errors[] array would be useful. Something like the media element's onerror, which means "look at media.error" would be ideal.

    PR+ with that for Rick's t719 branch.

  • Rick
  • David Humphrey

    David Humphrey September 26th, 2011 @ 03:39 PM

    • State changed from “review-needs-work” to “super-review-requested”
    • Assigned user changed from “Rick” to “cadecairos”

    PR+

  • cadecairos

    cadecairos September 26th, 2011 @ 04:21 PM

    • State changed from “super-review-requested” to “review-needs-work”

    popcorn.plugin.errors is causing problems with the remove plugin unit tests:

    "Popcorn.plugin.errors has one item, expected: 1 result: 0, diff: 1 0 "

  • cadecairos

    cadecairos September 26th, 2011 @ 04:22 PM

    • Assigned user changed from “cadecairos” to “Rick”
  • Rick
  • Rick

    Rick September 28th, 2011 @ 01:42 PM

    • State changed from “review-needs-work” to “super-review-requested”
    • Assigned user changed from “Rick” to “cadecairos”
  • cadecairos

    cadecairos September 28th, 2011 @ 02:40 PM

    • State changed from “super-review-requested” to “review-looks-good”

    Tested: Core unit tests

    on: Firefox 7, Aurora, Chrome 14

    all passing.

    passes lint.

    SR+

  • cadecairos

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Popcorn.js is an HTML5 video framework that lets you bring elements of the web into your videos.

Popcorn.js is a project of Web Made Movies, Mozilla's Open Video Lab.

Shared Ticket Bins

Referenced by

Pages