1154868 - exceptions thrown in bufferedUpdatedVideo callback get lost

Status: RESOLVED FIXED

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

Description

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Log exceptions in bufferedUpdateVideo callbacks

Comment 2

[Mike de Boer [:mikedeboer]]

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.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

(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.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

https://hg.mozilla.org/integration/fx-team/rev/476547232217

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Log exceptions in bufferedUpdateVideo callbacks,r=mikedeboer

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Updated patch with nits fixed, matching what was landed, attached above.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/476547232217