1288901 - window.open-ed moz-extension: tab is not closed when an extension closes

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Rob Wu [:robwu]]

1. Visit about:debugging, debug any WebExtension.
2. Run chrome.tabs.create({url:'manifest.json'}); to get a visible extension page
3. Open the console for that tab and run window.open(document.URL) to open the same page.
4. Go back to about:debugging and click on Reload, OR go to about:addons and disable/remove the addon.

Expected:
- Both moz-extension:-tabs should close.

Actual:
- Only the tabs created by chrome.tabs.create are closed, the tab created by window.open is still open. The tab would only close automatically at extension reload if I reload the window.open tab.

This happens with Firefox 47.0.1 and Firefox Nightly 50 (2016-07-22).

Results of my investigation:
When an extension page is opened via window.open, an ExtensionContext is created in the browser process with the URL about:blank (observed via page-load) and then closed (observed through page-unload). It is then not registered again. When an extension unloads, page-shutdown is triggered but since the tab is not registered any more, the tab is not closed.

The root cause is that the ExtensionContext is destroyed too early: A same-origin window.open invocation re-uses the global from about:blank before navigating to the specified destination. Between this switch the unload event is fired. As a result, the ExecutionContext is unloaded and the registration is lost. It is not registered again because creation happens in the content-document-global-created notification, which is not triggered when the global is re-used.

Another consequence of this bug is that APIs that require an ExtensionContext at the other end don't work in tabs opened via window.open. For instance, browser.tabs.query({}).then(tabs => console.log(tabs)); never prints anything in the tab that was opened via window.open.

Next steps:
- Find a notification that is only sent when the global is disposed (opposed to the unload event that may be triggered more than once). (Inner window stuff?)
- What should we do with ExecutionContext's URL? There are two options:
  * Initialize with the moz-extension:-URL (if we can find it...).
    Pro: The URL can stay immutable.
    Con: I don't know how to get this info.
  * Switch URL from about:blank to moz-extension: as soon as the URL changes.
    Pro: The ExtensionContext's URL is always accurate (and can also be made aware of history.pushState).
    Con: The URL is not immutable any more and the state must be synchronized.
- Investigate to what extent this problem also holds for ExtensionContext in the content process (ExtensionContent.jsm).

Comment 1

[Rob Wu [:robwu]]

Adding dependency on bug 1288276 because the unit tests for that bug helped with identifying the root cause of this bug.

Comment 2

[Rob Wu [:robwu]]

Attachment: Bug 1288901 - Destroy ExtensionContext at inner window destruction instead of unload

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

Comment 3

[Rob Wu [:robwu]]

Comment on attachment 8775372 [details]
Bug 1288901 - Destroy ExtensionContext at inner window destruction instead of unload

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

Comment 4

[Rob Wu [:robwu]]

Comment on attachment 8775372 [details]
Bug 1288901 - Destroy ExtensionContext at inner window destruction instead of unload

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

Comment 5

[Kris Maglione [:kmag]]

Have you considered just changing the observer to use document-element-inserted rather than content-document-global-created, like we do in ExtensionContent.jsm?

We incidentally have the same bfcache issues here that we used to have with content scripts, so we should really be adding an unload listener directly to the window, to prevent it from ever ending up in the bfcache.

Comment 6

[Rob Wu [:robwu]]

(In reply to Kris Maglione [:kmag] from comment #5)
> Have you considered just changing the observer to use
> document-element-inserted rather than content-document-global-created, like
> we do in ExtensionContent.jsm?

document-element-inserted may be sent multiple times (e.g. use document.write('<html>') ), so that's probably out of the question.

> We incidentally have the same bfcache issues here that we used to have with
> content scripts, so we should really be adding an unload listener directly
> to the window, to prevent it from ever ending up in the bfcache.

What is the relevance of this remark (referencing bug 1284942?) in this bug?

Comment 7

[Kris Maglione [:kmag]]

(In reply to Rob Wu [:robwu] from comment #6)
> (In reply to Kris Maglione [:kmag] from comment #5)
> > Have you considered just changing the observer to use
> > document-element-inserted rather than content-document-global-created, like
> > we do in ExtensionContent.jsm?
> 
> document-element-inserted may be sent multiple times (e.g. use
> document.write('<html>') ), so that's probably out of the question.

Hm. You have a point. There are ways of dealing with that, though. Either way,
we already use it for this purpose elsewhere, so we should do something
consistent.

> > We incidentally have the same bfcache issues here that we used to have with
> > content scripts, so we should really be adding an unload listener directly
> > to the window, to prevent it from ever ending up in the bfcache.
> 
> What is the relevance of this remark (referencing bug 1284942?) in this bug?

I was actually referencing bug 1191557, because the code here has a very
similar purpose, and very similar issues. But mainly, my point is that if
we're going to make changes to how we track unload events here, we should move
to adding an unload listener to the window itself, rather than moving to an
observer, because it also solves that problem.

Comment 8

[Rob Wu [:robwu]]

Attachment: Bug 1288901 - Create ExtensionContext at document-element-inserted

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

Comment 9

[Rob Wu [:robwu]]

Comment on attachment 8775372 [details]
Bug 1288901 - Destroy ExtensionContext at inner window destruction instead of unload

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

Comment 10

[Rob Wu [:robwu]]

(In reply to Kris Maglione [:kmag] from comment #7)
> (In reply to Rob Wu [:robwu] from comment #6)
> > (In reply to Kris Maglione [:kmag] from comment #5)
> > > Have you considered just changing the observer to use
> > > document-element-inserted rather than content-document-global-created, like
> > > we do in ExtensionContent.jsm?
> > 
> > document-element-inserted may be sent multiple times (e.g. use
> > document.write('<html>') ), so that's probably out of the question.
> 
> Hm. You have a point. There are ways of dealing with that, though. Either
> way,
> we already use it for this purpose elsewhere, so we should do something
> consistent.

I experimented a little bit, and it appears that document.write blows away the current DOMWindow (triggering inner-window-destroyed), so using the same notifications as content script will work just fine.

Comment 11

[Rob Wu [:robwu]]

Attachment: Bug 1288901 - Hide inactive extension views from getViews

When we close the ExtensionContext at inner-window-destroyed instead of
unload, the browser_ext_getViews.js test fails due to the extension view
appearing again in getViews() immediately after "closing" the extension
button popup. With this change that test passes again.

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

Comment 12

[Rob Wu [:robwu]]

Comment on attachment 8775372 [details]
Bug 1288901 - Destroy ExtensionContext at inner window destruction instead of unload

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67538/diff/4-5/

Comment 13

[Rob Wu [:robwu]]

Comment on attachment 8777172 [details]
Bug 1288901 - Create ExtensionContext at document-element-inserted

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

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8775372 [details]
Bug 1288901 - Destroy ExtensionContext at inner window destruction instead of unload

https://reviewboard.mozilla.org/r/67538/#review67830

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8777172 [details]
Bug 1288901 - Create ExtensionContext at document-element-inserted

https://reviewboard.mozilla.org/r/68784/#review67832

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8779140 [details]
Bug 1288901 - Hide inactive extension views from getViews

https://reviewboard.mozilla.org/r/70182/#review67836

Comment 17

[Rob Wu [:robwu]]

Please land this after the patches for bug 1288276 land.

The try job for both patch sets passes on Linux, Windows and Android: https://treeherder.mozilla.org/#/jobs?repo=try&revision=66c4a75374d0&selectedJob=25330984

Comment 18

[Rob Wu [:robwu]]

Removing checkin-needed because I need to revise the pagehide logic because of the patch for bug 1292369.

Comment 19

[Rob Wu [:robwu]]

Comment on attachment 8779140 [details]
Bug 1288901 - Hide inactive extension views from getViews

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

Comment 20

[Rob Wu [:robwu]]

Comment on attachment 8775372 [details]
Bug 1288901 - Destroy ExtensionContext at inner window destruction instead of unload

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67538/diff/5-6/

Comment 21

[Rob Wu [:robwu]]

Comment on attachment 8777172 [details]
Bug 1288901 - Create ExtensionContext at document-element-inserted

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

Comment 22

[Rob Wu [:robwu]]

With bug 1292369, the context.active value (view.active) is always accurate with respect to the bfcache, so I switched from using contentWindow.add/removeEventListener in context to checking view.active in extension.getViews.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

The Try push has browser_ext_getViews.js failures all over it.

Comment 24

[Rob Wu [:robwu]]

The try push you're looking at [1] fails because the try bot uses commit c4ad5f94a5bc as a parent of my changeset, which does not include the patch from bug 1292369.
Try bot : https://hg.mozilla.org/mozilla-central/shortlog/c4ad5f94a5bc (does not include patch for bug 1292369)
Required: https://hg.mozilla.org/mozilla-central/shortlog/4c4fcb84ee39 (includes both the patch and the above commit)

Locally the tests ran fine, and a try issued via mach try is also flawless: https://treeherder.mozilla.org/#/jobs?repo=try&revision=61310b9f8d5b&selectedJob=25745378 


[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf34036cbb3f&selectedJob=25759639

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/249faabb860c
Hide inactive extension views from getViews r=billm
https://hg.mozilla.org/integration/autoland/rev/adf4d9f4e668
Destroy ExtensionContext at inner window destruction instead of unload r=billm
https://hg.mozilla.org/integration/autoland/rev/174ca5f95419
Create ExtensionContext at document-element-inserted r=billm

Comment 26

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

https://hg.mozilla.org/mozilla-central/rev/249faabb860c
https://hg.mozilla.org/mozilla-central/rev/adf4d9f4e668
https://hg.mozilla.org/mozilla-central/rev/174ca5f95419

Comment 27

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce the initial issue on Firefox 50.02 (2016-09-05) under Windows 10 64-bit.

Verified as fixed on Firefox 51.0a1 (2016-09-05) under Windows 10 64-bit. Both moz-extension:-tabs are successfully closed.