Closed
Bug 1154868
Opened 10 years ago
Closed 10 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•10 years ago
|
||
Attachment #8592988 -
Flags: review?(standard8)
Assignee | ||
Updated•10 years ago
|
Attachment #8592988 -
Flags: review?(mdeboer)
Comment 2•10 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•10 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•10 years ago
|
Rank: 30
Flags: firefox-backlog+
Priority: -- → P3
Whiteboard: [error]
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8592988 -
Attachment is obsolete: true
Attachment #8593528 -
Flags: review+
Assignee | ||
Comment 6•10 years ago
|
||
Updated patch with nits fixed, matching what was landed, attached above.
Comment 7•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 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
•