Closed
Bug 1154868
Opened 9 years ago
Closed 9 years ago
exceptions thrown in bufferedUpdatedVideo callback get lost
Categories
(Hello (Loop) :: Client, defect, P3)
Tracking
(firefox40 fixed)
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: dmosedale, Assigned: dmosedale)
References
Details
(Whiteboard: [error])
Attachments
(1 file, 1 obsolete file)
4.26 KB,
patch
|
dmosedale
:
review+
|
Details | Diff | Splinter Review |
Since the exceptions occur in a setTimeout callback, it's currently hard to realize they're happening since the exception propagates up the call stack into nothingness and never gets logged.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8592988 -
Flags: review?(standard8)
Assignee | ||
Updated•9 years ago
|
Attachment #8592988 -
Flags: review?(mdeboer)
Comment 2•9 years ago
|
||
Comment on attachment 8592988 [details] [diff] [review] Log exceptions in bufferedUpdateVideo callbacks Review of attachment 8592988 [details] [diff] [review]: ----------------------------------------------------------------- I'm r+'ing this, because I don't see any real downside into wrapping this in a try...catch. However, when does this ever happen? Or did you work this out while auditing the code? I mean, I'd be a bit weary of putting everything we do using a setTimeout inside a try...catch block just for the sake of logging, don't you think? Perhaps your bug report could've benefited from an STR. Additionally, when you put up a reformatting patch like this I'd like to suggest one with whitespace changes ignored (-w flag). ::: browser/components/loop/content/shared/js/mixins.js @@ +436,5 @@ > } > > this._bufferedUpdateVideo = rootObject.setTimeout(function() { > + // Since this is being called from setTimeout, any exceptions thrown > + // will propagate upwards into nothingness, unless we go out of our nit: trailing space. @@ +437,5 @@ > > this._bufferedUpdateVideo = rootObject.setTimeout(function() { > + // Since this is being called from setTimeout, any exceptions thrown > + // will propagate upwards into nothingness, unless we go out of our > + // way to catch and log them explicitly, so... nit: one space too many. @@ +456,2 @@ > > + // Update the position and dimensions of the containers of local and nit: trailing space. @@ +471,5 @@ > + this.updateRemoteCameraPosition(ratio); > + } > + }, this); > + } catch (ex) { > + console.error("updateVideoContainer: _bufferedVideoUpdate exception:", Please make this fit on one line. 80ch is a soft limit.
Attachment #8592988 -
Flags: review?(standard8)
Attachment #8592988 -
Flags: review?(mdeboer)
Attachment #8592988 -
Flags: review+
Assignee | ||
Comment 3•9 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #2) > > I'm r+'ing this, because I don't see any real downside into wrapping this in > a try...catch. However, when does this ever happen? Or did you work this out > while auditing the code? I ran into this while making one of the views work in the ui-showcase. Since the ui-showcase is an artificial environment, making a view work in it involves a certain amount of trial and error to do the minimum scaffolding required, which is when I got tripped up by unseen exceptions. It took substantially longer to debug than it will going forwards. :-) > I mean, I'd be a bit weary of putting everything we do using a setTimeout > inside a try...catch block just for the sake of logging, don't you think? Indeed, if we wanted to make a habit of this (which seems worth thinking about separately), it would be much more reasonable to have a wrapper function do this, rather than doing it by hand. > Perhaps your bug report could've benefited from an STR. Yeah, next time I'll include more details about what motivated the change. > Additionally, when you put up a reformatting patch like this I'd like to suggest one with > whitespace changes ignored (-w flag). Yeah, sorry about that.
Updated•9 years ago
|
Rank: 30
Flags: firefox-backlog+
Priority: -- → P3
Whiteboard: [error]
Assignee | ||
Comment 4•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/476547232217
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8592988 -
Attachment is obsolete: true
Attachment #8593528 -
Flags: review+
Assignee | ||
Comment 6•9 years ago
|
||
Updated patch with nits fixed, matching what was landed, attached above.
Comment 7•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/476547232217
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•