Closed Bug 1154868 Opened 10 years ago Closed 10 years ago

exceptions thrown in bufferedUpdatedVideo callback get lost

Categories

(Hello (Loop) :: Client, defect, P3)

x86
macOS
defect
Points:
1

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Iteration:
40.2 - 27 Apr
Tracking Status
firefox40 --- fixed

People

(Reporter: dmosedale, Assigned: dmosedale)

References

Details

(Whiteboard: [error])

Attachments

(1 file, 1 obsolete file)

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.
Attachment #8592988 - Flags: review?(standard8)
Attachment #8592988 - Flags: review?(mdeboer)
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+
(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.
Rank: 30
Flags: firefox-backlog+
Priority: -- → P3
Whiteboard: [error]
Updated patch with nits fixed, matching what was landed, attached above.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: