Closed
Bug 1059674
Opened 10 years ago
Closed 10 years ago
"AddonManager: shutting down providers" should provide the list of misbehaving providers
Categories
(Toolkit :: Add-ons Manager, defect)
Toolkit
Add-ons Manager
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: Yoric, Assigned: Irving)
References
Details
Attachments
(5 files, 1 obsolete file)
24.07 KB,
patch
|
Unfocused
:
review+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
24.19 KB,
patch
|
Irving
:
review+
|
Details | Diff | Splinter Review |
2.78 KB,
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
24.41 KB,
patch
|
mossop
:
review+
Sylvestre
:
approval-mozilla-release+
|
Details | Diff | Splinter Review |
2.43 KB,
patch
|
mossop
:
feedback+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
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.
Reporter | ||
Comment 2•10 years ago
|
||
(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 | ||
Updated•10 years ago
|
Assignee: nobody → irving
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•10 years ago
|
||
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)
Reporter | ||
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
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)
Assignee | ||
Comment 7•10 years ago
|
||
Updated•10 years ago
|
Attachment #8496467 -
Flags: review?(bmcbride) → review+
Assignee | ||
Comment 8•10 years ago
|
||
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
Comment 9•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 10•10 years ago
|
||
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/
Assignee | ||
Comment 11•10 years ago
|
||
(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.
Comment 12•10 years ago
|
||
Is this going into 34 so we can get better data in the next Beta cycle?
Assignee | ||
Comment 13•10 years ago
|
||
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 14•10 years ago
|
||
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+
Updated•10 years ago
|
status-firefox33:
--- → wontfix
status-firefox34:
--- → affected
status-firefox35:
--- → fixed
tracking-firefox34:
--- → +
Comment 15•10 years ago
|
||
Comment 16•10 years ago
|
||
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)
Assignee | ||
Comment 17•10 years ago
|
||
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)
Assignee | ||
Comment 18•10 years ago
|
||
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+
Assignee | ||
Comment 19•10 years ago
|
||
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)
Reporter | ||
Comment 20•10 years ago
|
||
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+
Assignee | ||
Comment 21•10 years ago
|
||
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 22•10 years ago
|
||
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?
Comment 23•10 years ago
|
||
Folded together. Thanks!
https://hg.mozilla.org/releases/mozilla-aurora/rev/a721c4fbe22a
Comment 24•10 years ago
|
||
David or Irving, could you fill the uplift requests to beta & release? Thanks
Flags: needinfo?(irving)
Flags: needinfo?(dteller)
Reporter | ||
Comment 25•10 years ago
|
||
I'd rather let Irving do that, as he knows that piece of code better than me.
Flags: needinfo?(dteller)
Comment 26•10 years ago
|
||
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
Assignee | ||
Comment 27•10 years ago
|
||
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?
Comment 28•10 years ago
|
||
Any way you can get me a diff of just the backport changes?
Assignee | ||
Comment 29•10 years ago
|
||
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)
Comment 30•10 years ago
|
||
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?
Comment 31•10 years ago
|
||
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 32•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8509004 -
Flags: review?(dtownsend+bugmail) → review+
Comment 33•10 years ago
|
||
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+
Updated•10 years ago
|
Comment 34•10 years ago
|
||
Comment 35•10 years ago
|
||
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.
Assignee | ||
Comment 36•10 years ago
|
||
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.
Description
•