1012466 - exception on shutdown: "Exception calling provider shutdown", NS_ERROR_FAILURE from nsIObserverService.removeObserver from PreviousExperimentProvider.shutdown

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

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

I shut down a recent trunk build and got:

1400448613755	addons.manager	ERROR	Exception calling provider shutdown: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource:///modules/experiments/Experiments.jsm :: this.Experiments.PreviousExperimentProvider.prototype<.shutdown :: line 2091"  data: no] Stack trace: this.Experiments.PreviousExperimentProvider.prototype<.shutdown()@resource:///modules/experiments/Experiments.jsm:2091 < callProvider()@resource://gre/modules/AddonManager.jsm:194 < AMI_unregisterProvider()@resource://gre/modules/AddonManager.jsm:849 < AMP_unregisterProvider()@resource://gre/modules/AddonManager.jsm:2322 < Experiments.Experiments.prototype._unregisterWithAddonManager()@resource:///modules/experiments/Experiments.jsm:500 < Experiments.Experiments.prototype.uninit<()@resource:///modules/experiments/Experiments.jsm:446 < TaskImpl_run()@resource://gre/modules/Task.jsm:282 < TaskImpl_handleResultValue()@resource://gre/modules/Task.jsm:338 < TaskImpl_run()@resource://gre/modules/Task.jsm:290 < TaskImpl()@resource://gre/modules/Task.jsm:247 < createAsyncFunction/asyncFunction()@resource://gre/modules/Task.jsm:224 < Barrier.prototype<._wait()@resource://gre/modules/AsyncShutdown.jsm:552 < Barrier.prototype<.wait()@resource://gre/modules/AsyncShutdown.jsm:518 < Spinner.prototype.observe()@resource://gre/modules/AsyncShutdown.jsm:337 < <file:unknown>


Can't seem to easily reproduce.

Comment 1

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/annotate/41a54c8add09/browser/experiments/Experiments.jsm#l2019

I wonder if we never postInit()ed it? I think I've seen this in mochitest-chrome logs also, although it's not fatal.

Comment 2

[Benjamin Smedberg]

I'm wrong, it's here: http://hg.mozilla.org/mozilla-central/annotate/41a54c8add09/browser/experiments/Experiments.jsm#l2091

Comment 3

[Georg Fritzsche [:gfritzsche]]

With the async shutdown, i wonder if we can end up with the following order:
* AddonManager shuts down, calls PreviousExperimentProvider.shutdown()
* Async shutdown triggers Experiments.uninit() -> _unregisterWithAddonManager() -> AddonManagerPrivate.unregisterProvider() -> PreviousExperimentProvider.shutdown()
... as i don't see any protection against that in AM or Experiments.

Also, we end up not saving the cache to disk in this case; we should at the very least take care of that.

Comment 4

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

(In reply to :Gavin Sharp (email [e-mail]) from comment #0)
> I shut down a recent trunk build and got:
[...]

Did this finish with a crash, btw?

Comment 5

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

No.

Comment 6

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #3)
> With the async shutdown, i wonder if we can end up with the following order:
> * AddonManager shuts down, calls PreviousExperimentProvider.shutdown()
> * Async shutdown triggers Experiments.uninit() ->
> _unregisterWithAddonManager() -> AddonManagerPrivate.unregisterProvider() ->
> PreviousExperimentProvider.shutdown()

So, Felipe reproduces this consistently and it turns out we have a double PreviousExperimentProvider.shutdown() - one of them before the experiments manager shutdown.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Attachment: Double shutdown

My log

Comment 8

[:Felipe Gomes (needinfo for replies!)]

------- Call stack for first call ----------
> this.Experiments.PreviousExperimentProvider.prototype<.shutdown@resource:///modules/experiments/Experiments.jsm:2095:7
> AMI_callProviders@resource://gre/modules/AddonManager.jsm:904:15
> AMI_shutdown@resource://gre/modules/AddonManager.jsm:942:7
> Barrier.prototype<._wait@resource://gre/modules/AsyncShutdown.jsm:565:15
> Barrier.prototype<.wait@resource://gre/modules/AsyncShutdown.jsm:531:5
> Spinner.prototype.observe@resource://gre/modules/AsyncShutdown.jsm:337:7

------- Call stack for second call ----------
> this.Experiments.PreviousExperimentProvider.prototype<.shutdown@resource:///modules/experiments/Experiments.jsm:2095:7
> callProvider@resource://gre/modules/AddonManager.jsm:194:5
> AMI_unregisterProvider@resource://gre/modules/AddonManager.jsm:849:7
> AMP_unregisterProvider@resource://gre/modules/AddonManager.jsm:2321:5
> Experiments.Experiments.prototype._unregisterWithAddonManager@resource:///modules/experiments/Experiments.jsm:502:7
> Experiments.Experiments.prototype.uninit<@resource:///modules/experiments/Experiments.jsm:448:7
> TaskImpl_run@resource://gre/modules/Task.jsm:282:13
> TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:338:7
> TaskImpl_run@resource://gre/modules/Task.jsm:290:11
> TaskImpl@resource://gre/modules/Task.jsm:247:3
> createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:224:7
> Barrier.prototype<._wait@resource://gre/modules/AsyncShutdown.jsm:565:15
> Barrier.prototype<.wait@resource://gre/modules/AsyncShutdown.jsm:531:5
> Spinner.prototype.observe@resource://gre/modules/AsyncShutdown.jsm:337:7

Comment 9

[Georg Fritzsche [:gfritzsche]]

Thanks Felipe!

I think the cleanest solution here is to trigger Experiments._unregisterWithAddonManager() when the AddonManager shuts down.

However i don't see right now how to either
a) get notified of AddonManager shutdown, so we can unregister or to
b) tell AsyncShutdown to order Experiments shutdown before the AddonManager

Blair, any recommendations?

Comment 10

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

There is now a mechanism for ordering shutdown between services:
http://dutherenverseauborddelatable.wordpress.com/2014/05/26/shutting-down-asynchronously-part-2/

AddonManager should publish an instance of AsyncShutdown.Barrier. That's most likely a few lines of change.

Comment 11

[Georg Fritzsche [:gfritzsche]]

Ah, thanks, that looks good.
Blair, is that reasonable for you?

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

WFM. Although, exposed only on AddonManagerPrivate please.

Comment 14

[Georg Fritzsche [:gfritzsche]]

Some more discussion is found in the duped bug 1006771.

Comment 15

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

(In reply to Blair McBride [:Unfocused] from comment #12)
> WFM. Although, exposed only on AddonManagerPrivate please.

Why private? These days, a number of modules expose shutdown barriers for synchronization with other modules: OS.File, FHR Storage, Sqlite.jsm, ...

Comment 16

[Georg Fritzsche [:gfritzsche]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #15)
> (In reply to Blair McBride [:Unfocused] from comment #12)
> > WFM. Although, exposed only on AddonManagerPrivate please.
> 
> Why private? These days, a number of modules expose shutdown barriers for
> synchronization with other modules: OS.File, FHR Storage, Sqlite.jsm, ...

Can you make a call on that Blair?

Comment 17

[Marco Mucci [:MarcoM]]

Added to Iteration 33.2

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #16)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #15)
> > (In reply to Blair McBride [:Unfocused] from comment #12)
> > > WFM. Although, exposed only on AddonManagerPrivate please.
> > 
> > Why private? These days, a number of modules expose shutdown barriers for
> > synchronization with other modules: OS.File, FHR Storage, Sqlite.jsm, ...
> 
> Can you make a call on that Blair?

Fair enough. AddonManager then.

Comment 19

[Georg Fritzsche [:gfritzsche]]

Attachment: Add shutdown barrier for AddonManager clients

Irving, hg blame tells me you were working on the AM shutdown. What do you think of this?

Comment 20

[Georg Fritzsche [:gfritzsche]]

Attachment: Use AddonManager shutdown barrier in Experiments

Comment 21

[Georg Fritzsche [:gfritzsche]]

https://tbpl.mozilla.org/?tree=Try&rev=49ca386cc188

Comment 22

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

Comment on attachment 8448700 [details] [diff] [review]
Add shutdown barrier for AddonManager clients

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

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +746,2 @@
>        AsyncShutdown.profileBeforeChange.addBlocker("AddonManager: shutting down providers",
>                                                     this.shutdown.bind(this));

You might consider adding as third argument of `addBlocker` a function that returns the current shutdown state, as a JS object. This will help debugging if something fails during shutdown.

@@ +2806,5 @@
>      return AddonManagerInternal.escapeAddonURI(aAddon, aUri, aAppVersion);
> +  },
> +
> +  get shutdownBarrier() {
> +    return gShutdownBarrier.client;

For the moment, the convention used in other modules is to call this simply `shutdown`.

Comment 23

[Georg Fritzsche [:gfritzsche]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #22)
> ::: toolkit/mozapps/extensions/AddonManager.jsm
> @@ +746,2 @@
> >        AsyncShutdown.profileBeforeChange.addBlocker("AddonManager: shutting down providers",
> >                                                     this.shutdown.bind(this));
> 
> You might consider adding as third argument of `addBlocker` a function that
> returns the current shutdown state, as a JS object. This will help debugging
> if something fails during shutdown.

Hm, i'm not sure what information this would contain, can you expand on that?

> @@ +2806,5 @@
> > +  get shutdownBarrier() {
> > +    return gShutdownBarrier.client;
> 
> For the moment, the convention used in other modules is to call this simply
> `shutdown`.

There is already a shutdown method on AddonManagerInternal, so this seems like too much potential for confusion.

Comment 24

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

(In reply to Georg Fritzsche [:gfritzsche] from comment #23)
> Hm, i'm not sure what information this would contain, can you expand on that?

For Sqlite.jsm, it's the list of databases that are still open.

For OS.File, it's the latest messages sent and received from/to the worker, as well as status regarding the current stage of shutdown.

Here, I imagine the list of providers that haven't completed shutdown yet could be useful.

> 
> > @@ +2806,5 @@
> > > +  get shutdownBarrier() {
> > > +    return gShutdownBarrier.client;
> > 
> > For the moment, the convention used in other modules is to call this simply
> > `shutdown`.
> 
> There is already a shutdown method on AddonManagerInternal, so this seems
> like too much potential for confusion.

Fair enough.

Comment 25

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8448701 [details] [diff] [review]
Use AddonManager shutdown barrier in Experiments

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

::: browser/experiments/test/xpcshell/test_api.js
@@ +1529,1 @@
>    restartManager();

Since you're in a task, you could use 'yield promiseRestartManager();' here (there's also promiseShutdownManager() for yielding until the manager shuts down).

The non-promise versions spin a nested event loop inside themselves, which can lead to surprises.

Comment 26

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8448700 [details] [diff] [review]
Add shutdown barrier for AddonManager clients

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

One long IRC conversation with Unfocused later, I'm OK with adding AsyncShutdown.Blocker support to AddonManager, with a follow up bug to convert our existing support for provider.shutdown() methods to also use the Blocker.

If the Experiments provider is *only* shut down by the AddonManager blocker, Experiments.uninit() doesn't need to do AddonManagerPrivate.unregisterProvider() because AM automatically forgets all its registered providers when it shuts down.

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +947,5 @@
>      let shuttingDown = null;
>      if (gStarted) {
> +      shuttingDown = gShutdownBarrier.wait()
> +        .then(() => this.callProvidersAsync("shutdown"),
> +              err => logger.error("Failure during wait for shutdown barrier", err))

Put the error handler in a separate .then() directly after gShutdownBarrier.wait(). With the implementation here the callProvidersAsync("shutdown") won't be executed if gShutdownBarrier.wait() rejects, and we *do* want to execute it in that case.

@@ +2806,5 @@
>      return AddonManagerInternal.escapeAddonURI(aAddon, aUri, aAppVersion);
> +  },
> +
> +  get shutdownBarrier() {
> +    return gShutdownBarrier.client;

That's a bit messy in this case because we already have a shutdown() method, though it would be easy enough to rename that; it's not an external API.

Comment 27

[Georg Fritzsche [:gfritzsche]]

(In reply to :Irving Reid from comment #26)
> If the Experiments provider is *only* shut down by the AddonManager blocker,
> Experiments.uninit() doesn't need to do
> AddonManagerPrivate.unregisterProvider() because AM automatically forgets
> all its registered providers when it shuts down.

We do at least use uninit() explicitly in tests. Is this a potential issue or can we leave it?

Comment 28

[:Irving Reid (No longer working on Firefox)]

If you uninit() and init() the Experiments provider without restarting the Addon Manager, you need to be careful not to leave multiple shutdown blockers registered, and/or you need to correctly handle the case where the AM shuts down at a time when the Experiments provider isn't expecting it to.

For the XPI provider, in tests we always completely shut down the Addon Manager and unload/reload XPIProvider.jsm to simulate a browser restart as closely as possible.

Comment 29

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 1: Add shutdown barrier for AddonManager clients

Comment 30

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8448700 [details] [diff] [review]
Add shutdown barrier for AddonManager clients

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

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +966,5 @@
>          this.typeListeners.splice(0, this.typeListeners.length);
>          for (let type in this.startupChanges)
>            delete this.startupChanges[type];
>          gStarted = false;
>          gStartupComplete = false;

+     gShutdownBarrier = null;

to make sure we release all registered blockers when the test harness shuts down the AddonManager to simulate browser restarts.

Comment 31

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 2: Use AddonManager shutdown barrier in Experiments

Comment 32

[Georg Fritzsche [:gfritzsche]]

Attachment: Bonus: Promise-based restarting of AddonManager in experiments tests.

Comment 33

[Georg Fritzsche [:gfritzsche]]

https://tbpl.mozilla.org/?tree=Try&rev=f79ed3fe82a0

Comment 34

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 1: Add shutdown barrier for AddonManager clients

Comment 35

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8450275 [details] [diff] [review]
Part 1: Add shutdown barrier for AddonManager clients

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

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +2808,5 @@
> +  },
> +
> +  get shutdown() {
> +    if (!gShutdownBarrier) {
> +      return null;

Side-note: This is needed to avoid test-failures here: http://hg.mozilla.org/mozilla-central/annotate/ac6960197eb6/toolkit/mozapps/extensions/test/xpcshell/test_shutdown.js#l25

Comment 36

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8450275 [details] [diff] [review]
Part 1: Add shutdown barrier for AddonManager clients

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

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +931,5 @@
>     * up everything in order for automated tests to fake restarts.
>     * @return Promise{null} that resolves when all providers and dependent modules
>     *                       have finished shutting down
>     */
> +  shutdownManager: function AMI_shutdown() {

we don't need the function name any more, now that JS infers it from the object & attribute names, but don't rev the patch just to change that.

@@ +2808,5 @@
> +  },
> +
> +  get shutdown() {
> +    if (!gShutdownBarrier) {
> +      return null;

test_shutdown.js drives me crazy - I always end up having to rev patches because it fails on something I added.

The right fix is probably to add shutdown to the IGNORE list, and check IGNORE before 'typeof != function' at lines 25-28 and 42-45 of test_shutdown.js, but I can live with this as is.

Comment 38

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 1: Add shutdown barrier for AddonManager clients

Comment 39

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 2: Use AddonManager shutdown barrier in Experiments

Comment 40

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 3: Test overhaul

Comment 41

[Georg Fritzsche [:gfritzsche]]

The above patches are mostly finished, but currently blocked by at least one test issue in Part 3 that i'm too tired today to resolve:

> TEST-UNEXPECTED-FAIL | [...]/obj/_tests/xpcshell/browser/experiments/test/xpcshell/test_api.js | getActiveExperimentID should return the active experiment1 - null == "[e-mail]" - See following stack:
>    [...]/obj/_tests/xpcshell/browser/experiments/test/xpcshell/test_api.js:test_getActiveExperimentID:348
...


This is proceeded by apparently a successful experiment activation, including the addon install, but then we fail to get the addon:

> 0:04.88 1404836201629	Browser.Experiments.Experiments	TRACE	ExperimentEntry #2::_installAddon() - install ended for [e-mail]
> 0:04.88 1404836201630	Browser.Experiments.Experiments	TRACE	ExperimentEntry #2::Add-on is disabled. Enabling.
> 0:04.88 1404836201631	addons.xpi-utils	DEBUG	Updating active state for add-on [e-mail] to true
> 0:04.89 1404836201631	DeferredSave.extensions.json	DEBUG	Save changes
> 0:04.89 1404836201631	addons.xpi	DEBUG	Registering manifest for /var/folders/tp/mgx9wlds46j6q3ck3xhjzm2w0000gn/T/tmphVU72e/extensions/[e-mail]
> 0:04.89 1404836201631	addons.xpi	WARN	Add-on [e-mail] is missing bootstrap method startup
> 0:04.89 1404836201632	Browser.Experiments.Experiments	INFO	ExperimentEntry #2::onEnabled() for [e-mail]
> 0:04.89 1404836201632	addons.xpi	DEBUG	Registering manifest for /var/folders/tp/mgx9wlds46j6q3ck3xhjzm2w0000gn/T/tmphVU72e/extensions/[e-mail]
> 0:04.89 1404836201632	addons.xpi	WARN	Add-on [e-mail] is missing bootstrap method startup
> 0:04.89 1404836201632	DeferredSave.extensions.json	DEBUG	Starting timer
> 0:04.89 1404836201638	addons.xpi	DEBUG	removeTemporaryFile: http://localhost:63323/data/experiment-1.xpi removing temp file /Users/gfritzsche/Library/Caches/TemporaryItems/tmp-zoi.xpi
> 0:04.89 1404836201639	Browser.Experiments.Experiments	ERROR	Experiments #1::evaluateExperiments() - Unable to start experiment: Could not obtain add-on for experiment that should be enabled.
> 0:04.89 1404836201639	Browser.Experiments.Experiments	TRACE	ExperimentEntry #2::reconcileAddonState()
> 0:04.89 1404836201640	Browser.Experiments.Experiments	TRACE	ExperimentEntry #2::reconcileAddonState() - Inactive experiment has no add-on. Doing nothing.

Comment 42

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 3: Test overhaul

Finally found the (test-only) issue, this should be good now.

https://tbpl.mozilla.org/?tree=Try&rev=f228a901623f

Comment 43

[Benjamin Smedberg]

Try run from comment 42 is green.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/4e67f3f51610
https://hg.mozilla.org/integration/fx-team/rev/100fce3292d4
https://hg.mozilla.org/integration/fx-team/rev/eba5f233f01b

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4e67f3f51610
https://hg.mozilla.org/mozilla-central/rev/100fce3292d4
https://hg.mozilla.org/mozilla-central/rev/eba5f233f01b

Comment 46

[Benjamin Smedberg]

Setting NEEDINFO for gfritzsche: I think we want an aurora uplift for this, but I'm not sure about risk assessment.

Comment 47

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8452407 [details] [diff] [review]
Part 1: Add shutdown barrier for AddonManager clients

Approval Request Comment
[Feature/regressing bug #]: Telemetry experiments.
[User impact if declined]: Exception on shutdown, possibly out-of-sync experiments data.
[Describe test coverage new/current, TBPL]: baked on mc for a bit, automatic test-coverage not feasible.
[Risks and why]: Leaning to medium due to an addon manager change. It is well understood though and should be ok given proper bake-time.
[String/UUID change made/needed]: None.

Comment 48

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8452408 [details] [diff] [review]
Part 2: Use AddonManager shutdown barrier in Experiments

Approval Request Comment: See comment 47.

Comment 49

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8453029 [details] [diff] [review]
Part 3: Test overhaul

Approval Request Comment: See comment 47.

Comment 50

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Georg Fritzsche [:gfritzsche] [away July 10-14] from comment #47)
> Comment on attachment 8452407 [details] [diff] [review]
> Part 1: Add shutdown barrier for AddonManager clients
> 
> Approval Request Comment
> [Feature/regressing bug #]: Telemetry experiments.
What is the oldest release that is impacted?

> [User impact if declined]: Exception on shutdown, possibly out-of-sync
> experiments data.
I take it from the early comments that this bug impacts all users not just those with an active experiment. Can you confirm?

> [Describe test coverage new/current, TBPL]: baked on mc for a bit, automatic
> test-coverage not feasible.
Was this shutdown crash 100% reproducible? How have you confirmed that the issue has been addressed?

> [Risks and why]: Leaning to medium due to an addon manager change. It is
> well understood though and should be ok given proper bake-time.
We're near the end of the Aurora 32 cycle. What would you say is proper bake time in this case?

Comment 51

[Georg Fritzsche [:gfritzsche]]

(In reply to Lawrence Mandel [:lmandel] from comment #50)
> (In reply to Georg Fritzsche [:gfritzsche] [away July 10-14] from comment
> #47)
> > Comment on attachment 8452407 [details] [diff] [review]
> > Part 1: Add shutdown barrier for AddonManager clients
> > 
> > Approval Request Comment
> > [Feature/regressing bug #]: Telemetry experiments.
> What is the oldest release that is impacted?

Firefox 31, which is where we enabled experiments.

> > [User impact if declined]: Exception on shutdown, possibly out-of-sync
> > experiments data.
> I take it from the early comments that this bug impacts all users not just
> those with an active experiment. Can you confirm?

Yes, that is correct.

> > [Describe test coverage new/current, TBPL]: baked on mc for a bit, automatic
> > test-coverage not feasible.
> Was this shutdown crash 100% reproducible? How have you confirmed that the
> issue has been addressed?

It was not 100% reproducible, but confirmed by an affected user.

> > [Risks and why]: Leaning to medium due to an addon manager change. It is
> > well understood though and should be ok given proper bake-time.
> We're near the end of the Aurora 32 cycle. What would you say is proper bake
> time in this case?

I'm not 100% sure on an optimal bake-time, but personally i'd expect us to be free of major issues and fine for Aurora after 4-5 days.

Comment 52

[Georg Fritzsche [:gfritzsche]]

Judging from Yorics shutdown dashboard (which may not be bug-free yet though), we probably fixed the experiments shutdown issue (bug 1012466) on 33 and can have a guess at numbers (with a sample size of 500):
http://yoric.github.io/are-we-shutting-down-yet-/?sample_size=500#Experiments.jsm%20shutdown

In the samples i checked, no recent 33 build ids are present.

Comment 53

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8452407 [details] [diff] [review]
Part 1: Add shutdown barrier for AddonManager clients

Aurora+

Can we use Yoric's dashboard to confirm that this is fixed on Aurora?

Comment 54

[Georg Fritzsche [:gfritzsche]]

(In reply to Lawrence Mandel [:lmandel] from comment #53)
> Can we use Yoric's dashboard to confirm that this is fixed on Aurora?

It has a limited sample-size, but we can go for "probably fixed" as above.

Comment 55

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7d1746cdf765
https://hg.mozilla.org/releases/mozilla-aurora/rev/2af1ca98c935
https://hg.mozilla.org/releases/mozilla-aurora/rev/e776a6a21d1c

Comment 56

[Georg Fritzsche [:gfritzsche]]

Backout while i take a bit to fix test failures:
https://hg.mozilla.org/releases/mozilla-aurora/rev/ada88bfa9f0a
https://hg.mozilla.org/releases/mozilla-aurora/rev/5465ee62d826
https://hg.mozilla.org/releases/mozilla-aurora/rev/a3386f2441d6

Comment 57

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] [away July 10-14] from comment #56)
> Backout while i take a bit to fix test failures:

... and the push with failures:
https://tbpl.mozilla.org/?tree=Mozilla-Aurora&rev=e776a6a21d1c

Comment 58

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/eba99269ef53
https://hg.mozilla.org/releases/mozilla-aurora/rev/c2c33b7f9006
https://hg.mozilla.org/releases/mozilla-aurora/rev/a87021ee40b8

Comment 59

[Georg Fritzsche [:gfritzsche]]

A quick inspection of two days back showed no reports from after the landing of the patch here:
http://yoric.github.io/are-we-shutting-down-yet-/?sample_size=500&version=Firefox+32.0a2#Experiments.jsm%20shutdown

The previous caveats still apply though (caveats: bounded sample size, dashboard not guaranteed to be bug free).

Comment 60

[Robert Kaiser]

(In reply to Georg Fritzsche [:gfritzsche] from comment #59)
> A quick inspection of two days back showed no reports from after the landing
> of the patch here

Awesome. I guess the real verification will be 32.0b1 shipping to the beta audience (search experiment will still be ongoing there, right?) which should then also not suffer from the crash. Does that sound correct?