1402205 - Non-existent background page blocks extension startup

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

[smartfon.reddit]

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 1

[YF (Yang)]

WFM in Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0 ID:20170921220243.

Comment 2

[Andrew Swan [:aswan]]

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.

Comment 3

[Andrew Swan [:aswan]]

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?

Comment 4

[Andrew Swan [:aswan]]

whoops, mean to include this in the previous comment
http://searchfox.org/mozilla-central/rev/15ce5cb2db0c85abbabe39a962b0e697c9ef098f/toolkit/components/extensions/ExtensionPageChild.jsm#334-341

Comment 5

[Haik Aftandilian [:haik]]

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.

Comment 6

[Haik Aftandilian [:haik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3be1c769b0812287bff1116877749c2d4ae493d0

Comment 7

[Haik Aftandilian [:haik]]

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.

Comment 8

[Haik Aftandilian [:haik]]

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 9

[Haik Aftandilian [:haik]]

Attachment: Bug 1402205 - Non-existent background page blocks extension startup.

Review commit: https://reviewboard.mozilla.org/r/183430/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183430/

Comment 10

[Haik Aftandilian [:haik]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183430/diff/1-2/

Comment 11

[Haik Aftandilian [:haik]]

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 12

[Haik Aftandilian [:haik]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183430/diff/2-3/

Comment 13

[Haik Aftandilian [:haik]]

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!

Comment 14

[smartfon.reddit]

(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.

Comment 15

[Kris Maglione [:kmag]]

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.

Comment 16

[Haik Aftandilian [:haik]]

Thanks, Kris.

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/611582e42451
Non-existent background page blocks extension startup. r=kmag

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/611582e42451

Comment 19

[Haik Aftandilian [:haik]]

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

Comment 20

[Haik Aftandilian [:haik]]

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

Comment 21

[Sylvestre Ledru [:Sylvestre]]

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

Bad UX with extensions, taking it.
Should be in 57b5

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/667e147f2e0a

Comment 23

[Haik Aftandilian [:haik]]

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

Comment 24

[marius.santa]

Attachment: not already running.gif

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.

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

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.