1609000 - nsIControllers.removeController fails on extension shutdown

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[Shane Caraveo (:mixedpuppy)]

I'm seeing a lot of failure tracebacks in extension tests when we destroy the background script browser that is in a hidden window. It happens during extension shutdown.

Console message: [JavaScript Error: "[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIControllers.removeController]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: chrome://global/content/elements/browser-custom-element.js :: destroy :: line 1374" data: no]"]

https://searchfox.org/mozilla-central/rev/9e45d74b956be046e5021a746b0c8912f1c27318/toolkit/content/widgets/browser-custom-element.js#1374

_releaseBrowser@resource://gre/modules/ExtensionParent.jsm:1427:18

https://searchfox.org/mozilla-central/rev/9e45d74b956be046e5021a746b0c8912f1c27318/toolkit/components/extensions/ExtensionParent.jsm#1427

e.g. bug 1590787: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=284751157&repo=autoland&lineNumber=43972

Comment 1

[Rob Wu [:robwu]]

This also happens when an extension popup (e.g. browserAction) is closed. I was looking into this as a part of bug 1394019

Comment 2

[Rob Wu [:robwu]]

Regressed by https://hg.mozilla.org/mozilla-central/rev/8f97448e08d6

Comment 3

[Rob Wu [:robwu]]

Attachment: Bug 1609000 - Stop logspam when <browser> is removed

Comment 4

[Chris Peterson [:cpeterson]]

(In reply to Rob Wu [:robwu] from comment #2)

Regressed by https://hg.mozilla.org/mozilla-central/rev/8f97448e08d6

The regressing bug 1604003 change is now in 73 Beta. Do we need to uplift this fix to 73 Beta?

Comment 5

[Rob Wu [:robwu]]

Yes, let's do that. I'll request uplift once the patch has landed.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d035b29accb1
Stop logspam when <browser> is removed r=kmag

Comment 7

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/d035b29accb1

Comment 8

[Alex Cornestean]

Hello,

Will this fix require manual validation? If yes, please provide some steps to reproduce in order to correctly test it and also, please set the "qe-verify+" flag. Otherwise, could the "qe-verify-" flag be added?

Thanks!

Comment 9

[Ryan VanderMeulen [:RyanVM]]

We're in RC week now for 73. How badly do we need to uplift this?

Comment 10

[Rob Wu [:robwu]]

This patch resolves a recent regression that caused logspam in a frequently used component.
For this reason I'd like to uplift this.

Comment 11

[Rob Wu [:robwu]]

Comment on attachment 9122423 [details]
Bug 1609000 - Stop logspam when <browser> is removed

Beta/Release Uplift Approval Request

• User impact if declined: Logspam in the browser console by frequently used browser features (non-tab <browser> elements). E.g. when the user hovers the mouse over the extension button.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small patch: the code reponsible for logspam is wrapped in a condition that does not throw.

An alternative fix is to remove the log call, as shown at: https://phabricator.services.mozilla.com/D60720?id=221020

• String changes made/needed: -

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9122423 [details]
Bug 1609000 - Stop logspam when <browser> is removed

No end user impact, just makes some spammy warnings go away. Approved for 73.0 RC2.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/5c99d1184932