1137469 - If an uncaught exception occurs whilst processing an action, the dispatcher can fail, rendering parts of Loop inactive

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect)

Description

[Mark Banner (:standard8)]

This is the best explanation I've got so far for the end results of bug 1137439.

The dispatcher is vulnerable to uncaught exceptions whilst actions are being processed - if they throw an exception, then it could stop the dispatcher working.

For the panel, this will stop all rooms related activities working, for the conversation window this would stop the whole window working.

Bug 1137439 is the only one I know of so far that has hit this case. I haven't seen other reports of similar possibilities yet.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Sylvestre: just a heads up that from chatting with Mark, I think we may want to take a patch for this as a ride-along for 36.0.1.

Comment 2

[Mark Banner (:standard8)]

Attachment: If an uncaught exception occurs whilst processing an action, the dispatcher can fail, rendering parts of Loop inactive.

Simple fix to add a try/catch, this protects the dispatcher and ensures that the "_active" flag gets reset even if there's an error (and hence further actions can be handled).

Comment 3

[Mark Banner (:standard8)]

[Tracking Requested - why for this release]:(In reply to :Gavin Sharp [email: [e-mail]] from comment #1)
> Sylvestre: just a heads up that from chatting with Mark, I think we may want
> to take a patch for this as a ride-along for 36.0.1.

To expand on that, this is a simple patch to add a try/catch to stop possible uncaught exceptions.

If one of these exceptions does occur, then it'll stop the main part of the Hello panel from working, or it will stop the conversation window working.

For the panel, the user would have to restart the browser, or opening a new window to get a working panel.

The conversation window would just need to be re-opened.

Comment 4

[Sylvestre Ledru [:Sylvestre]]

OK, please make sure that it lands asap.

Comment 5

[Mike de Boer [:mikedeboer]]

Comment on attachment 8570163 [details] [diff] [review]
If an uncaught exception occurs whilst processing an action, the dispatcher can fail, rendering parts of Loop inactive.

Review of attachment 8570163 [details] [diff] [review]:
-----------------------------------------------------------------

Just a small note: It'd be nice for next time to have the tests attached as a separate patch, so that the reviewer can run the tests with and without the fix(es) applied. That's a very nice way to _see_ that the patch fixes the issue. In this case it's rather obvious, but still :)

::: browser/components/loop/content/shared/js/dispatcher.js
@@ +74,5 @@
>        registeredStores.forEach(function(store) {
> +        try {
> +          store[type](action);
> +        } catch (x) {
> +          console.error(x);

Can you make this log message stand out a bit more by prefixing it with a message? It'd help for potential future bug reports.

Comment 6

[Mark Banner (:standard8)]

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

For verification:

We haven't got STR for bug 1137439 or for causing the exception. However, considering this is going to be uplifted, I think it would be worth a basic test just to ensure that the features of loop keep on working - i.e. Conversations open and can be renamed/deleted as expected, and conversations with other people can be held.

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8570163 [details] [diff] [review]
If an uncaught exception occurs whilst processing an action, the dispatcher can fail, rendering parts of Loop inactive.

Approval Request Comment
[Feature/regressing bug #]: Firefox Hello, Conversations implementation
[User impact if declined]: If the user hits an error (we aren't sure of the root causes of these yet), then they may get in a position where they can't use any of the Loop panel UI until they restart the browser (although opening a window would cause just the new window to work), or the conversation window could stop working until re-opened.
[Describe test coverage new/current, TreeHerder]: Now covered by unit tests to check that the dispatcher keeps working if there's an exception.
[Risks and why]: Low. Simple try/catch to handle uncaught errors. This should allow the user to continue even if there's been an error.
[String/UUID change made/needed]: None

Comment 8

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bcae3d285873

Comment 9

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/mozilla-beta/rev/c0698821db6c

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/63286f849ae3

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/38f803ca8922

Comment 12

[Petruta Horea [:phorea], Desktop Test Engineering]

Marking as verified on Firefox 37 as we performed several calls between different OSs while testing the beta 2 build for it's sign-off.

Comment 13

[Petruta Horea [:phorea], Desktop Test Engineering]

We performed several calls between Win 7 64-bit, Win 8.1 32-bit, Mac OSX 10.9.5 and Ubuntu 14.04 32-bit using Firefox 36.0.1 and didn't see any issues. Marking as verified.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Added to the 36.0.1 release notes with "36.0.1 - In some cases, Hello could be inactive (1137469)"

Comment 15

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Already verified for 36 and 37 - no targeted manual testing needed for 38 and 39 - if additional issues with Loop are spotted we'll revisit this.