Closed Bug 1288901 Opened 3 years ago Closed 3 years ago

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

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(firefox50 affected, firefox51 verified)

VERIFIED FIXED
mozilla51
Tracking Status
firefox50 --- affected
firefox51 --- verified

People

(Reporter: robwu, Assigned: robwu)

References

Details

(Whiteboard: triaged)

Attachments

(3 files)

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).
Adding dependency on bug 1288276 because the unit tests for that bug helped with identifying the root cause of this bug.
Depends on: 1288276
Whiteboard: triaged
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 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/
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.
(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?
(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.
Review commit: https://reviewboard.mozilla.org/r/68784/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68784/
Attachment #8775372 - Attachment description: Bug 1288901 - Destroy ExtensionContext at DOMWindow destruction instead of unload → Bug 1288901 - Destroy ExtensionContext at inner window destruction instead of unload
Attachment #8777172 - Flags: review?(wmccloskey)
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/
(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 on attachment 8775372 [details]
Bug 1288901 - Destroy ExtensionContext at inner window destruction instead of unload

https://reviewboard.mozilla.org/r/67538/#review67830
Attachment #8775372 - Flags: review?(wmccloskey) → review+
Comment on attachment 8777172 [details]
Bug 1288901 - Create ExtensionContext at document-element-inserted

https://reviewboard.mozilla.org/r/68784/#review67832
Attachment #8777172 - Flags: review?(wmccloskey) → review+
Comment on attachment 8779140 [details]
Bug 1288901 - Hide inactive extension views from getViews

https://reviewboard.mozilla.org/r/70182/#review67836
Attachment #8779140 - Flags: review?(wmccloskey) → review+
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
Keywords: checkin-needed
Removing checkin-needed because I need to revise the pagehide logic because of the patch for bug 1292369.
Keywords: checkin-needed
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.
Keywords: checkin-needed
The Try push has browser_ext_getViews.js failures all over it.
Keywords: checkin-needed
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
Depends on: 1292369
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-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
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.