1010289 - TEST-UNEXPECTED-FAIL | resource://app/modules/experiments/Experiments.jsm | A promise chain failed to handle a rejection: AlreadyShutdownError: uninit() already called

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Comment 1

[David Teller [:Yoric] - still alive but not very active]

See https://tbpl.mozilla.org/php/getParsedLog.php?id=39573107&tree=Try&full=1#error5

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Attachment: Fixing the issue

This should also fix bug 1010291.

Comment 3

[Benjamin Smedberg]

Comment on attachment 8422676 [details] [diff] [review]
Fixing the issue

It appears that the meat of this patch is:

* the changes to AlreadyShutdownError
* test_api.js

Is that correct? Why are the changes to AlreadyShutdownError necessary?

Is the rest just style fixup? If so why is the new style preferable?

Comment 4

[David Teller [:Yoric] - still alive but not very active]

* The changes to AlreadyShutdownError is necessary to get the correct stack. Otherwise, the stack obtained was pure noise.

* The apparent style fixups are actually the meat of the bugfix, as they put the `catch` in the right place. Prior to the fix, the error handler only caught the errors that were raised before the call to .then() and was missing any error from the success handler. Rewriting them as Tasks makes the error more visible.

* I am not 100% sure that the change in test_api.js is necessary, but there was also potential for an uncaught error.

Comment 5

[Benjamin Smedberg]

Comment on attachment 8422676 [details] [diff] [review]
Fixing the issue

The extra Task is so ugly :-(

Since shutdown exceptions are the only exceptions that are expected here, maybe just checking this._shutdown would make this cleaner and avoid the extra nested task/spawn/bind stuff.

Other exceptions really should be turning the testsuite orange.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

I for one find Task less error-prone. YMMV. If you prefer I can rewrite that patch with raw Promise instead of Task.


By the way, my patch fixes 3 uncaught errors in the various tests of Experiments.jsm (errors 1, 2 and 3 of https://tbpl.mozilla.org/php/getParsedLog.php?id=39573107&tree=Try&full=1 ). Could you take a look at the errors and tell me if any of these should be real oranges?

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Attachment: Fixing the issue, v2

This version doesn't introduce a new Task and is a bit finer-grained on the exceptions it catches.

Comment 8

[Benjamin Smedberg]

`AddonManager is not initialized` is not an expected error.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d50053fec04

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2d50053fec04

Comment 12

[Legacy TBPL/Treeherder Robot]

submit_timestamp: 2014-10-13T18:43:42
log: https://treeherder.mozilla.org/ui/logviewer.html#?repo=mozilla-inbound&job_id=2961732
repository: mozilla-inbound
who: wkocher[at]mozilla[dot]com
machine: tst-linux64-spot-657
buildname: Ubuntu VM 12.04 x64 mozilla-inbound opt test mochitest-browser-chrome-3
revision: 4c4092b50ccb

477 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/test-window/browser_experiments.js | A promise chain failed to handle a rejection:  - at resource://app/modules/experiments/Experiments.jsm:876 - Error: uninit() alrady called