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

RESOLVED FIXED in Firefox 36

Status

Hello (Loop)
Client
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

unspecified
mozilla39
Points:
1
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(firefox36+ verified, firefox37+ verified, firefox38 fixed, firefox39 fixed, relnote-firefox 36+)

Details

Attachments

(1 attachment)

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.
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.
status-firefox36: --- → affected
tracking-firefox36: --- → ?
Flags: needinfo?(sledru)
Created attachment 8570163 [details] [diff] [review]
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).
Attachment #8570163 - Flags: review?(mdeboer)
Attachment #8570163 - Flags: review?(jaws)
Attachment #8570163 - Flags: review?(dmose)
[Tracking Requested - why for this release]:(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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.
status-firefox37: --- → affected
status-firefox38: --- → affected
tracking-firefox37: --- → ?
OK, please make sure that it lands asap.
status-firefox39: --- → affected
tracking-firefox36: ? → +
tracking-firefox37: ? → +
Flags: needinfo?(sledru)
Attachment #8570163 - Flags: review?(jaws)
Attachment #8570163 - Flags: review?(dmose)
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.
Attachment #8570163 - Flags: review?(mdeboer) → review+
Status: NEW → ASSIGNED
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.
Iteration: --- → 39.1 - 9 Mar
Points: --- → 1
Flags: qe-verify+
Flags: in-testsuite+
Flags: firefox-backlog+
Target Milestone: --- → mozilla39
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
Attachment #8570163 - Flags: approval-mozilla-release?
Attachment #8570163 - Flags: approval-mozilla-beta?
Attachment #8570163 - Flags: approval-mozilla-aurora?
Attachment #8570163 - Flags: approval-mozilla-release?
Attachment #8570163 - Flags: approval-mozilla-release+
Attachment #8570163 - Flags: approval-mozilla-beta?
Attachment #8570163 - Flags: approval-mozilla-beta+
Attachment #8570163 - Flags: approval-mozilla-aurora?
Attachment #8570163 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/bcae3d285873
status-firefox38: affected → fixed
status-firefox39: affected → fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/c0698821db6c
status-firefox37: affected → fixed
https://hg.mozilla.org/releases/mozilla-release/rev/63286f849ae3
status-firefox36: affected → fixed
https://hg.mozilla.org/mozilla-central/rev/38f803ca8922
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
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.
status-firefox37: fixed → verified
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.
status-firefox36: fixed → verified
Added to the 36.0.1 release notes with "36.0.1 - In some cases, Hello could be inactive (1137469)"
relnote-firefox: --- → 36+
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.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.