Self-support, and tests for webextensions' webRequest and UITour, all rely on HiddenFrame.jsm, which creates frames in the hidden DOM window. It should use windowless browsers instead.

Comment on attachment 8852021 [details] Bug 1351300 - stop using the hidden DOM window from HiddenFrame.jsm, https://reviewboard.mozilla.org/r/124268/#review126930 r=me, but I'd rather we use a simple event listener rather than a webprogress observer unless there's a particularly good reason not to. ::: browser/modules/HiddenFrame.jsm:51 (Diff revision 1) > - if (this._frame) { > - if (!Cu.isDeadWrapper(this._frame)) { > - this._frame.removeEventListener("load", this, true); > + this._webProgress.removeProgressListener(this._listener); > + this._listener = null; > + this._webProgress = null; > - this._frame.remove(); > } > - > + if (this._frame) { No need for the `if`, I think. ::: browser/modules/HiddenFrame.jsm:55 (Diff revision 1) > } > - > + if (this._frame) { > - this._frame = null; > + this._frame = null; > + } > this._deferred = null; > + if (this._browser) { Already checked this in the outer `if`. ::: browser/modules/HiddenFrame.jsm:85 (Diff revision 1) > + // Get the window reference via the document. > + this._frame = this._browser.document.ownerGlobal; > + this._deferred.resolve(this._frame); > - } > + } > + }; > + this._webProgress.addProgressListener(this._listener, Ci.nsIWebProgress.NOTIFY_STATE_ALL); Nit: Just pass `NOTIFY_STATE_NETWORK` rather than `NOTIFY_STATE_ALL` here, and skip the `STATE_IS_NETWORK` check in the callback. Although, is there a particular reason you're not just waiting for the DOMContentLoaded or load even here? Or even DOMDocElementInserted, I guess. ::: browser/modules/test/browser/browser_SelfSupportBackend.js (Diff revision 1) > -/** > - * Test that SelfSupportBackend only allows HTTPS. > - */ > -add_task(function* test_selfSupport_noHTTPS() { > - Preferences.set(PREF_SELFSUPPORT_URL, TEST_PAGE_URL); > - > - SelfSupportBackend.init(); > - > - // SelfSupportBackend waits for "sessionstore-windows-restored" to start loading. Send it. > - info("Sending sessionstore-windows-restored"); > - sendSessionRestoredNotification(); > - > - // Find the SelfSupport browser. We don't expect to find it since we are not using https. > - let selfSupportBrowser = findSelfSupportBrowser(TEST_PAGE_URL); > - Assert.ok(!selfSupportBrowser, "SelfSupport browser must not exist."); > - > - // We shouldn't need this, but let's keep it to make sure closing SelfSupport twice > - // doesn't create any problem. > - SelfSupportBackend.uninit(); > -}) Any particular reason we don't still need this?

(In reply to Kris Maglione [:kmag] from comment #3) > ::: browser/modules/HiddenFrame.jsm:85 > (Diff revision 1) > > + // Get the window reference via the document. > > + this._frame = this._browser.document.ownerGlobal; > > + this._deferred.resolve(this._frame); > > - } > > + } > > + }; > > + this._webProgress.addProgressListener(this._listener, Ci.nsIWebProgress.NOTIFY_STATE_ALL); > > Nit: Just pass `NOTIFY_STATE_NETWORK` rather than `NOTIFY_STATE_ALL` here, > and skip the `STATE_IS_NETWORK` check in the callback. > > Although, is there a particular reason you're not just waiting for the > DOMContentLoaded or load even here? Or even DOMDocElementInserted, I guess. I haven't found a way to do that. Windowless browsers don't have `addEventListener`, and the docShell object, once you get hold of it, has a null chromeEventListener. Listening on the initial window object has also not worked for me. If you've got a suggestion that works I'm all ears. > ::: browser/modules/test/browser/browser_SelfSupportBackend.js > (Diff revision 1) > > -/** > > - * Test that SelfSupportBackend only allows HTTPS. > > - */ > > -add_task(function* test_selfSupport_noHTTPS() { > > - Preferences.set(PREF_SELFSUPPORT_URL, TEST_PAGE_URL); > > - > > - SelfSupportBackend.init(); > > - > > - // SelfSupportBackend waits for "sessionstore-windows-restored" to start loading. Send it. > > - info("Sending sessionstore-windows-restored"); > > - sendSessionRestoredNotification(); > > - > > - // Find the SelfSupport browser. We don't expect to find it since we are not using https. > > - let selfSupportBrowser = findSelfSupportBrowser(TEST_PAGE_URL); > > - Assert.ok(!selfSupportBrowser, "SelfSupport browser must not exist."); > > - > > - // We shouldn't need this, but let's keep it to make sure closing SelfSupport twice > > - // doesn't create any problem. > > - SelfSupportBackend.uninit(); > > -}) > > Any particular reason we don't still need this? Well, all of this is going to go away when we ship SHIELD, but more practically, there isn't a good way to assert that this doesn't happen - the existing test relies on no iframes being created in the hidden DOM window, and now that we're not using that and there's no way to enumerate windowless browsers, we can't do that. But this really also doesn't strike me as an important thing to test in this way. Does that make sense?

(In reply to :Gijs from comment #4) > I haven't found a way to do that. Windowless browsers don't have > `addEventListener`, and the docShell object, once you get hold of it, has a > null chromeEventListener. Listening on the initial window object has also > not worked for me. If you've got a suggestion that works I'm all ears. For extension background pages, we use window.document.addEventListener("DOMContentLoaded", ...) We call createAboutBlankContentViewer to synchronously initialize the docShell, though.

(In reply to Kris Maglione [:kmag] from comment #5) > (In reply to :Gijs from comment #4) > > I haven't found a way to do that. Windowless browsers don't have > > `addEventListener`, and the docShell object, once you get hold of it, has a > > null chromeEventListener. Listening on the initial window object has also > > not worked for me. If you've got a suggestion that works I'm all ears. > > For extension background pages, we use > window.document.addEventListener("DOMContentLoaded", ...) > > We call createAboutBlankContentViewer to synchronously initialize the > docShell, though. Do you mean code similar to what's in ExtensionParent.jsm which force-inits the docShell as you said, then uses a "chrome-document-global-created" observer which checks that it's being called for the 'right' document, followed by a load event listener-turned-into-promise after a bunch of QIs... which doesn't seem to really make things less complex - can I leave it like this? :-) (There's already similar code in the backgroundpagethumbs code, from bug 1288030.)

(In reply to :Gijs from comment #6) > Do you mean code similar to what's in ExtensionParent.jsm which force-inits > the docShell as you said, then uses a "chrome-document-global-created" > observer which checks that it's being called for the 'right' document, > followed by a load event listener-turned-into-promise after a bunch of > QIs... which doesn't seem to really make things less complex - can I leave > it like this? :-) Hm. You're right. I guess it looks simpler when you have those helpers. Anyway, I'm fine with your approach. But maybe you should switch to NOTIFY_STATE_DOCUMENT rather than NETWORK.

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/80d9492ea03f stop using the hidden DOM window from HiddenFrame.jsm, r=kmag

I filed bug 1351989 about being able to just add event listeners for loads in windowless browsers.