Non-existent background page blocks extension startup

VERIFIED FIXED in Firefox 57

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: smartfon.reddit, Assigned: haik)

Tracking

unspecified
mozilla58
Points:
---

Firefox Tracking Flags

(firefox56+ wontfix, firefox57 verified, firefox58 verified)

Details

Attachments

(2 attachments)

Reporter

Description

2 years ago
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20170921220243

Steps to reproduce:

Install "TrafikLite" WebExtension from AMO. Open add-ons manager and disable it. The extension icon doesn't disappear. Close Nightly 58 and use its shortcut to launch a new instance. A popup message says a process is already running. Need to force close it to be able to launch Nightly. 
Does not happen with other WebExtensions that were tested. Tested in a fresh profile.
Comment hidden (obsolete)
This is a problem only with extensions.webextensions.remote is set to true.
I notice that the extension manifest includes this:
```
  "background": {
    "scripts": ["background-script.js","jquery.js","numeral.min.js"],
	"persistent": false
  },
```
But the extension doesn't actually include a file called numeral.min.js (it does include numeral.js)
As a result, this promise never resolves:
http://searchfox.org/mozilla-central/rev/15ce5cb2db0c85abbabe39a962b0e697c9ef098f/toolkit/components/extensions/ext-backgroundPage.js#36

This causes the extension startup to never finish, which eventually leads to the symptoms reported (the extension doesn't shut down, the browser action icon isn't removed, browser shutdown gets blocked).
From IRC, Kris suspects an issue with moz-extension: remoting.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1
Summary: A WebExtension requires a browser restart to disable or remove it. → Non-existent background page blocks extension startup
Following one small step further, this suggests we're not getting DOMContentLoaded when the background page has a <script src"moz-extension://XXX"> tag that references a non-existent file.
Haik, looks like an issue with moz-extension: remoting?
Flags: needinfo?(haftandilian)
Assignee

Comment 5

2 years ago
Loading the nonexistent js file ends up in ExtensionStreamGetter::OnFD() where calling mJarChannel->AsyncOpen2(listener) fails. On failure, we call mChannel->Cancel(NS_BINDING_ABORTED) with mChannel being the SimpleChannel, but that isn't sufficient in this case. Needs a little more debugging to confirm what's happening, but I'll take this bug and get it fixed.
Assignee: nobody → haftandilian
Flags: needinfo?(haftandilian)
Assignee

Comment 7

2 years ago
Will post patch shortly. The fix adds calls to OnStartRequest() and OnStopRequest() on the AsyncOpen2() failure handling in ExtensionProtocolHandler::OnFD() and similarly to OnStream(). Before the fix, we just called mChannel->Cancel() which ends up in nsBaseChannel::Cancel() which just sets mStatus to the error, but won't do anything further because mRequest is null.
Assignee

Comment 8

2 years ago
Resynced and re-ran the Windows try due to all the web platform test failures. New run is clean:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f00cd2af1cd00f060bfe26c1feeb2e21947f122b
Comment hidden (mozreview-request)
Assignee

Comment 11

2 years ago
ExtensionProtocolHandler::OnFD() and OnStream() have two minor issues I'd like to fix in other bug because this fix should be as minimal as possible because it will need to be uplifted to 56.

First, the comments "We must keep an owning reference to the listener" are slightly wrong. We do need to keep an owning reference to the listener because after OnFD()/OnStream() return, the SimpleChannel and ExtensionStreamGetter objects which reference the listener may both be deallocated. But we pass the listener on before returning so calling mListener.forget() shouldn't be needed.

Second, OnFD()'s error handling calls OnStream() (originally) to reduce duplicated code. That seems confusing now.
Comment hidden (mozreview-request)
Assignee

Comment 13

2 years ago
smartfon.reddit, could you verify that the fix works for you? Windows download available here

  https://queue.taskcluster.net/v1/task/Z1KeekJIThu23EKb7Fw40w/runs/0/artifacts/public/build/target.zip

Thanks!
Flags: needinfo?(smartfon.reddit)
Reporter

Comment 14

2 years ago
(In reply to Haik Aftandilian [:haik] from comment #13)
> smartfon.reddit, could you verify that the fix works for you? Windows
> download available here
> 
>  
> https://queue.taskcluster.net/v1/task/Z1KeekJIThu23EKb7Fw40w/runs/0/
> artifacts/public/build/target.zip
> 
> Thanks!

It works. Extension is disabled and the browser can be properly closed.
Flags: needinfo?(smartfon.reddit)

Comment 15

2 years ago
mozreview-review
Comment on attachment 8912049 [details]
Bug 1402205 - Non-existent background page blocks extension startup.

https://reviewboard.mozilla.org/r/183430/#review188858

Please file a follow-up bug to add tests for this.
Attachment #8912049 - Flags: review?(kmaglione+bmo) → review+
Assignee

Comment 16

2 years ago
Thanks, Kris.

Test bug: Bug 1398908 - Add automated test that uses nonexistent script from JAR file (for bug 1395898, 1402205)

Comment 17

2 years ago
Pushed by haftandilian@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/611582e42451
Non-existent background page blocks extension startup. r=kmag
Assignee

Updated

2 years ago
See Also: → 1398908

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/611582e42451
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Assignee

Comment 19

2 years ago
I'll request uplift to Beta/57 and Release/56 after this is stable in Nightly for 24 hours.
Assignee

Comment 20

2 years ago
Comment on attachment 8912049 [details]
Bug 1402205 - Non-existent background page blocks extension startup.

Approval Request Comment
[Feature/Bug causing the regression]:
The fix for bug 1334550 which shipped in build 56 caused this regression.

[User impact if declined]:
Extensions with broken links in their scripts/HTML may fail to uninstall cleanly and may have resources fail to load. The bug has only been reproducible with extensions.webextensions.remote=true (which is the default for Windows only.)

[Is this code covered by automated tests?]:
Not all the error paths are covered. Bug 1398908 is filed to add tests for (at a minimum) some of these error paths. The test bug isn't ready to land yet.

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]:
No 

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
The changes are small and isolated to error paths not frequently hit.

[String changes made/needed]:
None
Attachment #8912049 - Flags: approval-mozilla-beta?
Comment on attachment 8912049 [details]
Bug 1402205 - Non-existent background page blocks extension startup.

Bad UX with extensions, taking it.
Should be in 57b5
Attachment #8912049 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee

Comment 23

2 years ago
Comment on attachment 8912049 [details]
Bug 1402205 - Non-existent background page blocks extension startup.

Approval Request Comment
[Feature/Bug causing the regression]:
The fix for bug 1334550 which shipped in build 56 caused this regression.

[User impact if declined]:
Extensions with broken links in their scripts/HTML may fail to uninstall cleanly and may have resources fail to load. The bug has only been reproducible with extensions.webextensions.remote=true (which is the default for Windows only.)

[Is this code covered by automated tests?]:
Not all the error paths are covered. Bug 1398908 is filed to add tests for (at a minimum) some of these error paths. The test bug isn't ready to land yet.

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]:
No 

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
The changes are small and isolated to error paths not frequently hit.

[String changes made/needed]:
None
Attachment #8912049 - Flags: approval-mozilla-release?

Comment 24

2 years ago
This bug is verified on Firefox 57.0b5 (20171002181526) and Firefox 58.0a1 (20171002220204) under Windows 10 64bit/Mac OS X 10.12.3.

Please see the attached screenshot.

Updated

2 years ago
Status: RESOLVED → VERIFIED
We can consider this for uplift if we have a dot release driver for 56. I'll track it in case we need to build 56.0.2. I don't want to uplift until we have a clear reason for the dot release, though.
Seems like this could be fixed by affected extensions fixing their broken links. Since I don't know yet that this is a widespread issue, I'd like to hold off on uplift.  It's good that it's fixed and verified for 57.
Attachment #8912049 - Flags: approval-mozilla-release? → approval-mozilla-release-
You need to log in before you can comment on or make changes to this bug.