Closed Bug 1059674 Opened 5 years ago Closed 5 years ago

"AddonManager: shutting down providers" should provide the list of misbehaving providers

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox33 --- fixed
firefox34 + fixed
firefox35 --- fixed

People

(Reporter: Yoric, Assigned: Irving)

References

Details

Attachments

(5 files, 1 obsolete file)

No description provided.
I think the right approach to this is:

a) Add a 'providerName' field to the add-on providers we control, so we can log this info as part of our error handling / status reporting

b) for add-ons that use AddonManager.registerProvider() and don't have a 'providerName', dig into the JS stack and record where the call came from

c) Rather than calling this.callProvidersAsync("shutdown") (at around http://mxr.mozilla.org/mozilla-beta/source/toolkit/mozapps/extensions/AddonManager.jsm#951), register each provider's shutdown method with AddonManager.shutdown.addBlocker() and let AsyncShutdown call them - use the name from (a) or (b) above to identify each blocker.
(In reply to :Irving Reid from comment #1)
> b) for add-ons that use AddonManager.registerProvider() and don't have a
> 'providerName', dig into the JS stack and record where the call came from

Fwiw, `addBlocker` already digs in the JS stack and records where the call comes from.
Assignee: nobody → irving
Status: NEW → ASSIGNED
Changes sprawled a little farther than I was hoping, mostly to handle run time register/unregister.

The one thing missing is a test that we actually report a hanging provider shutdown correctly...

r? for David to make sure I'm using AsyncShutdown properly, and sr? for Blair to make sure I'm not breaking the AddonManager.

Try run at https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3e9ee68c1c0b
Attachment #8495314 - Flags: superreview?(bmcbride)
Attachment #8495314 - Flags: review?(dteller)
Comment on attachment 8495314 [details] [diff] [review]
Use AsyncShutdown.Blocker to shut down AddonManager providers

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

It is my understanding that you need a fast review, so I only looked at AsyncShutdown usage, which looks good.
I can't help but think that there must be a simpler way, though. Have you looked at http://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Sqlite.jsm?from=Sqlite.jsm#73-153 to see how we handle a related question in Sqlite.jsm?

::: toolkit/components/asyncshutdown/AsyncShutdown.jsm
@@ +561,5 @@
>  }
>  Barrier.prototype = Object.freeze({
> +  get name() {
> +    return this._name;
> +  },

The information is already available in `barrier.client.name`, so it's probably not necessary here.
Attachment #8495314 - Flags: review?(dteller) → review+
Comment on attachment 8495314 [details] [diff] [review]
Use AsyncShutdown.Blocker to shut down AddonManager providers

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

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +463,5 @@
>  var gUpdateEnabled = true;
>  var gAutoUpdateDefault = true;
>  var gHotfixID = null;
>  var gShutdownBarrier = null;
> +var gRepoShutdownState = "pending";

I'm not hugely familiar with some parts of blockers. Seems rather odd that during startup, and all the time this *isn't* shutting down, it's pending shutdown.

@@ +624,5 @@
> +                                 Cr.NS_ERROR_NOT_INITIALIZED);
> +
> +    callProvider(aProvider, "startup", null, aAppChanged, aOldAppVersion, aOldPlatformVersion);
> +    if ('shutdown' in aProvider) {
> +      let name = aProvider.name || "Provider";

"<unnamed-provider>"? (similar to "<anonymous>" we use for functions)
Unnamed providers should be intentionally ugly. Also, should be a const, since you use it more than once.
Attachment #8495314 - Flags: superreview?(bmcbride) → superreview+
Added a test, and it's a good thing I did because my shutdownState() method wasn't actually returning its state so the status reporting wouldn't have worked o_O

Cleaned up the nits and added a bit of race-condition prevention for unregistering providers during shutdown.
Attachment #8495314 - Attachment is obsolete: true
Attachment #8496467 - Flags: review?(bmcbride)
Attachment #8496467 - Flags: review?(bmcbride) → review+
OK, this puts the addon manager shutdown under control of AsyncShutdown, but it doesn't specifically report the XPI Provider's shut down of individual add-ons, so it doesn't completely satisfy the original bug description. Filed bug 1073976 for tracking individual bootstrap add-ons in XPIProvider.

Note that the XPI Provider shuts down bootstrap add-ons in quit-application-granted, a little before profile-before-change, so it would need a separate Blocker: http://hg.mozilla.org/mozilla-central/annotate/5e704397529b/toolkit/mozapps/extensions/internal/XPIProvider.jsm#l2174

https://hg.mozilla.org/integration/fx-team/rev/8ae4bfe77e21
Summary: "AddonManager: shutting down providers" should provide the list of misbehaving add-ons → "AddonManager: shutting down providers" should provide the list of misbehaving providers
https://hg.mozilla.org/mozilla-central/rev/8ae4bfe77e21
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment on attachment 8496467 [details] [diff] [review]
Use AsyncShutdown.Blocker to shut down AddonManager providers, with test

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

::: toolkit/mozapps/extensions/test/xpcshell/test_provider_shutdown.js
@@ +26,5 @@
> +    },
> +  };
> +  mockProvider.donePromise = new Promise((resolve, reject) => {
> +    mockProvider.doneResolve = resolve;
> +    mockProvider.doneResject = reject;

s/Resject/Reject/
(In reply to Georg Fritzsche [:gfritzsche] from comment #10)
> Comment on attachment 8496467 [details] [diff] [review]
> Use AsyncShutdown.Blocker to shut down AddonManager providers, with test

> > +    mockProvider.doneResject = reject;
> 
> s/Resject/Reject/

I'll fix that in bug 1074135, along with few other nits I noticed.
Depends on: 1075625
Is this going into 34 so we can get better data in the next Beta cycle?
Comment on attachment 8496467 [details] [diff] [review]
Use AsyncShutdown.Blocker to shut down AddonManager providers, with test

Approval Request Comment
[Feature/regressing bug #]:1074135
[User impact if declined]: Shutdown hangs / crashes
[Describe test coverage new/current, TBPL]:
Has been on Nightly for a week. Crash-stats shows substantial reduction in AsyncShutdown crashes caused by Experiments.jsm, AddonManager exception telemetry shows no new forms of failure.
[Risks and why]: Low risk - at worst, would cause other forms of shutdown crash.
[String/UUID change made/needed]: None
Attachment #8496467 - Flags: approval-mozilla-aurora?
Comment on attachment 8496467 [details] [diff] [review]
Use AsyncShutdown.Blocker to shut down AddonManager providers, with test

Yes. Let's take this now in time for 34 beta. Aurora+
Attachment #8496467 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This or bug 1074135 caused mass mochitest bustage. Backed out.
https://hg.mozilla.org/releases/mozilla-aurora/rev/d847271bce7b

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=314204&repo=mozilla-aurora

17:37:44 INFO - 1412987864858 addons.manager ERROR startup failed: TypeError: Expected nothing or a function as third argument (resource://gre/modules/AsyncShutdown.jsm:432) JS Stack trace: Barrier/this.client.addBlocker<@AsyncShutdown.jsm:432:1 < Spinner.prototype.addBlocker@AsyncShutdown.jsm:312:5 < getPhase/phase<.addBlocker@AsyncShutdown.jsm:247:7 < AMI_startup@AddonManager.jsm:813:1 < AMP_startup@AddonManager.jsm:2393:5 < AMC_observe@addonManager.js:55:7 

And a lot more spew after that.
Flags: needinfo?(irving)
There was a small API change to AsyncShutdown.addBlocker between 34 and 35; I'm preparing a small follow-up patch to update the one place where I call it (and the corresponding test).
Flags: needinfo?(irving)
This is the already reviewed patch, de-bitrotted for Fx 34 for the sheriffs' convenience. This does *not* include the changes necessary to work correctly with the Fx 34 version of the AsyncShutdown API; I'm attaching a separate patch for that to make the changes clear.
Attachment #8503664 - Flags: review+
This patch should be applied after the Fx34 port of the original patch (https://bugzilla.mozilla.org/attachment.cgi?id=8503664); if you want to squash the two into one before landing I don't mind.

Up to you whether you want review, or are willing to land it a=bustage-fix.

Once this is applied, the existing patch for bug 1074135 should apply cleanly.
Attachment #8503665 - Flags: review?(dteller)
Comment on attachment 8503665 [details] [diff] [review]
Fixes to work with Fx 34 AsyncShutdown API

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

The `_name` thing is not very nice, but as long as it's for 34, that's fine with me.
Attachment #8503665 - Flags: review?(dteller) → review+
Comment on attachment 8503665 [details] [diff] [review]
Fixes to work with Fx 34 AsyncShutdown API

Approval Request Comment
[Feature/regressing bug #]: 1057312
[User impact if declined]: Shutdown hang/crashes
[Describe test coverage new/current, TBPL]:
[Risks and why]: Bustage fix for back-port of previously approved patch, low risk.
[String/UUID change made/needed]: None
Attachment #8503665 - Flags: approval-mozilla-aurora?
Comment on attachment 8503665 [details] [diff] [review]
Fixes to work with Fx 34 AsyncShutdown API

We usually don't make simple bustage fixes go through re-approval.
Attachment #8503665 - Flags: approval-mozilla-aurora?
David or Irving, could you fill the uplift requests to beta & release? Thanks
Flags: needinfo?(irving)
Flags: needinfo?(dteller)
I'd rather let Irving do that, as he knows that piece of code better than me.
Flags: needinfo?(dteller)
Irving, FYI, this is critical for 33.0.1.
Ryan (thanks btw) has launched a try job on Mozilla-release with this patch and bug 1074135.
It is busted:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=533d0b3beb49
Could you have a look to his asap? thanks
Approval Request Comment
[Feature/regressing bug #]: 1057312
[User impact if declined]: Shutdown crashes
[Describe test coverage new/current, TBPL]: Running on Beta and Aurora for 1 week, includes unit tests
[Risks and why]: Exposes bug 1081702, but does not make things worse (Experiments shutdown has always intermittently failed, just with different symptoms now)
[String/UUID change made/needed]: None


The existing patch used the new ES6 method declaration syntax that was not supported in the 33 branch. This patch includes both the de-bitrot and Fx34 patches created for the Aurora/Beta uplift, plus the method syntax changes.
Attachment #8509004 - Flags: review?(dtownsend+bugmail)
Attachment #8509004 - Flags: approval-mozilla-release?
Any way you can get me a diff of just the backport changes?
this is the interdiff between ("Aurora de-bitrot of AsyncShutdown patch" + "Fixes to work with Fx 34 AsyncShutdown API") and "Use AsyncShutdown.Blocker, back-ported to 33 branch"
Flags: needinfo?(irving)
Attachment #8509169 - Flags: feedback?(dtownsend+bugmail)
Do you need help from the Manual QA team to verify this fix for 33.0.1? If yes, can you also please provide some clear instructions to test?
In reply to Florin Mezei, QA (:FlorinMezei) from comment #30)
> Do you need help from the Manual QA team to verify this fix for 33.0.1? If
> yes, can you also please provide some clear instructions to test?

Just noticed that this isn't in fact in 33.0.1, so disregard the above comment.
Comment on attachment 8509169 [details] [diff] [review]
Interdiff of release backport

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

Awesome, thanks
Attachment #8509169 - Flags: feedback?(dtownsend+bugmail) → feedback+
Attachment #8509004 - Flags: review?(dtownsend+bugmail) → review+
Comment on attachment 8509004 [details] [diff] [review]
Use AsyncShutdown.Blocker, back-ported to 33 branch

To the sheriff, please land that also in GECKO330_2014101104_RELBRANCH
thanks
Attachment #8509004 - Flags: approval-mozilla-release? → approval-mozilla-release+
I hear build 2 for 33.0.1 should come out soon... if this is the case, please provide details for testing this: what Add-ons are affected, what's the scenario to reproduce this, and anything else that may allow us to verify it.
This bug did not affect particular add-ons. It caused occasional shutdown hangs / crashes (see bug 1057312 for further discussion).

Because the failure is intermittent, we don't have specific steps to reproduce. I can't think of any symptoms to look for other that the the hang itself. If the race condition involved doesn't go down the path to the hang, everything works normally. In the failure case there may be some messages printed to standard output / error if the browser is running from a terminal session, but you'll also get the shutdown crash.

If I can think of anything else to look for, I'll follow up.
You need to log in before you can comment on or make changes to this bug.