1493770 - AsyncShutdown timeout in asyncEmitManifestEntry("chrome_settings_overrides")

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P1)

Description

[Kris Maglione [:kmag]]

This happens in particular with the DuckDuckGo search add-on:

{
    "conditions": [
        {
            "filename": "resource://gre/modules/addons/XPIProvider.jsm",
            "lineNumber": 2211,
            "name": "Extension shutdown: jid1-ZAdIEUB7XOzOJw@jetpack",
            "stack": ["resource://gre/modules/addons/XPIProvider.jsm:startup/<:2211"],
            "state": {"state": "Startup: Run manifest: asyncEmitManifestEntry(\"chrome_settings_overrides\")"}
        }
    ],
    "phase": "profile-change-teardown"
}

Comment 1

[Mike Kaply [:mkaply] (Out until 9/29)]

Can you give me some more details as to what this means?

Comment 2

[Kris Maglione [:kmag]]

(In reply to Mike Kaply [:mkaply] from comment #1)
> Can you give me some more details as to what this means?

The promise returned by the onManifest handler in the chrome_settings_override API never resolves for some users.

Comment 3

[Kris Maglione [:kmag]]

Here's another one:

https://crash-stats.mozilla.com/report/index/05b26d1d-d38e-4323-b9e5-020c50180922#tab-metadata

Comment 4

[Mike Kaply [:mkaply] (Out until 9/29)]

In the second case, the add-on was Ecosia.

What's odd is these are two very different scenarios. In the DuckDuckGo case, no engine is added (because it already exists).

In the second case, an engine is added

Where did your information come for the opening the original problem?

Is this something you were able to recreate?

Comment 5

[Andrew Swan [:aswan]]

From offline conversation with mkaply:

Which suggests that one of the search-related promises isn't resolving in time (ie "await searchInitialized" or "await addSearchEngine()".  Is there some possible case where the search service "init-complete" event never gets fired?

Comment 6

[Mike Kaply [:mkaply] (Out until 9/29)]

Have a recreation scenario on Windows:

Create a new profile and make sure it doesn't show the "multiple tabs open" message on shutdown.

1. Use the web page method to install an add-on which includes a search engine (eg. https://ext.ask.com/index.jhtml)
2. When the second prompt is showing, which is for the default search, close Firefox using the X button in the window title bar

If anything interferes with the closing of Firefox, it won't happen (dialog popup).

What you'll see is that multiple processes are left open in the task manager and after a minute or so, Firefox crashes

Comment 7

[Mike Kaply [:mkaply] (Out until 9/29)]

Attachment: Bug 1493770 - Switch WebExt default search prompt away from promise.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6c39cc812c87
Switch WebExt default search prompt away from promise. r=aswan

Comment 9

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/6c39cc812c87

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Is this something we might consider taking in a 65 RC build or is it safer at this point to just let it ride with 66?

Comment 11

[Kris Maglione [:kmag]]

This is super trivial. I'd suggest taking it.

Comment 12

[Mike Kaply [:mkaply] (Out until 9/29)]

I'm personally torn. On the one hand, it hasn't been seen a lot, on the other hand it's a crasher.

As Kris said, it's a pretty trivial patch.

Comment 13

[Mike Kaply [:mkaply] (Out until 9/29)]

Comment on attachment 9035795 [details]
Bug 1493770 - Switch WebExt default search prompt away from promise.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: Occasional crash when extension with search engine is installed.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: See https://bugzilla.mozilla.org/show_bug.cgi?id=1493770#c6

Works on Mac too.

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Refactors very self contained code.

String changes made/needed:

Comment 14

[Daniel Bodea [:danibodea]]

This issue appears to only be reproducible on Windows platform. Is this correct?

In any case, the issue was reproduced on Nightly v66.0a1 from 20190115 and Windows 10 and the fix is now verified on Nightly v66
.0a1 from 20190123 and Windows 10, using the str form comment 6. I will mark this bug as verified.

@Mike: Could this issue also be reproduced on other OSes than Windows? Could/Should I verify the fix on other OSes?

Thank you!

Comment 15

[Mike Kaply [:mkaply] (Out until 9/29)]

It is recreatable on Mac (you have to use the keystroke to close the app).

But Windows was most prevalent, so seeing that you are able to verify the fix worked, I think we're good.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9035795 [details]
Bug 1493770 - Switch WebExt default search prompt away from promise.

[Triage Comment]
Fixes an occasional crash when an extension with a search engine is installed. Verified by QA on nightly. Approved for 65.0 RC2.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/4cc5f35b2e80

Comment 18

[[:philipp]]

i don't really see a decline of those crashes in 65.0rc2 - should we reopen the bug or follow up in a new one?
https://crash-stats.mozilla.com/search/?async_shutdown_timeout=~chrome_settings_overrides&version=65.0rc2

Comment 19

[Mike Kaply [:mkaply] (Out until 9/29)]

Are those this bug?

https://bugzilla.mozilla.org/show_bug.cgi?id=1464938

Comment 20

[Kris Maglione [:kmag]]

The MOZ_CRASH reason is now sanitized, and I don't have access to unsanitized probes, so I can't even begin to guess whether the crashes are still from this issue, or from another.

(In reply to Mike Kaply [:mkaply] from comment #19)

Are those this bug?

https://bugzilla.mozilla.org/show_bug.cgi?id=1464938

Bug 1464938 is an umbrella bug for all extension shutdown hangs, including this one.

Comment 21

[Kris Maglione [:kmag]]

OK, apparently there's an AsyncShutdownTimeout metadata field with the necessary information now. And this is definitely still a bug in chrome_settings_overrides, though perhaps in a different place now.

Comment 22

[Daniel Bodea [:danibodea]]

As an addition to comment 14, I have reproduced the issue on Mac OS 10.13.6 with Nightly v66.0a1 from 2019-01-15 (using the COMMAND + Q keystroke to quit the browser, and then verified the fix in Nightly v67.0a1 from 2019-01-30.

Furthermore, while continuing testing for uplift, the changeset from comment 17 is to the release channel, not to beta. I have reproduced the issue in Firefox Release v64.0 and then validated the fix on Firefox Release v65.0 on Windows 10 and Mac OS 10.13.6.
I have also verified the fix on the latest Nightly for certainty.

Thank you for your contribution!