Calling window.confirm() with OOP enabled fails with an error. From options_ui: > window.gBrowser is undefined RemotePrompt.jsm:41 From popup: > TypeError: this.Dialog is undefined tabprompts.xml:200:17 From sidebar: > window.gBrowser is null RemotePrompt.jsm:41

Bug 1375490 fixed the popup and sidebar case but problem remains for options_ui. > window.gBrowser is undefined RemotePrompt.jsm:41

I can confirm that I've also encountered this issue and I tried developer edition and nightly. I am using window.confirm in my options_ui page and as soon as the it hits the confirm call, it stops. No modal appears and the page is unresponsive.

It looks like 57 will release with OOP web extensions enabled by default for Windows. Is this bug expected to be closed before 57 release? If not, my addon will be impacted since I use alerts/confirm in my options_ui page.

[Tracking Requested - why for this release]: Out-of-process web extensions is enabled by default on Windows in 56 since https://bugzilla.mozilla.org/show_bug.cgi?id=1357486. This is already impacting users of Foxy Gestures. I use window.confirm() and window.alert() quite a lot in my options_ui page.

[Tracking Requested - why for this release]: Affects 56 and two of my extensions.

Andy, is this something that might be fixed in 57? Or is that going to depend on other priorities? I don't think we can fix this in time for 56 release. But, in case we can and it's simple, we can still take a patch this week for the last beta build.

Will not be fixed for 56. Might be for 57 (its marked as a P2).

Doesn't sound like we need to track this specifically.

Comment on attachment 8909881 [details] Bug 1385548 - Part 1: Support tab modals in WebExtensions options_ui pages. Hi Kris, here is a draft patch which provides the gBrowser stub needed to allow the tab modals to work for the options_ui remote browsers. The approach is (unfortunately) different from the one used by the sidebar, the main reason is that TabModalPromptBox is defined at browser level, but the "about:addons" page is defined at toolkit level. The approach used in this patch is basically the following: it tries to retrieve the parent of the "about:addons" page, if the parent has gBrowser, it uses the parent window's gBrowser to create a TabModalPromptBox associated to the "about:addons" tab (so that the "about:addons" tab is visually marked to signal the presence of the modal in the tab and the tab modal is going to cover the entire tab, as it does when the options_ui is not remote). I'd like to get a feedback on the general approach (or if there are other alternative approaches that you think that can be a better fit to fix this issue) (side note: part 4 contains a new test for this, the other two patches fix issues that have been found by running all the tests, but I'm not setting f?/r? on them until we confirm the general approach).

Comment on attachment 8909881 [details] Bug 1385548 - Part 1: Support tab modals in WebExtensions options_ui pages. https://reviewboard.mozilla.org/r/181372/#review187228 ::: toolkit/mozapps/extensions/content/extensions.js:4164 (Diff revision 1) > + const aboutAddonsBrowser = parentWindow.gBrowser.getBrowserForDocument(window.document); > + return parentWindow.gBrowser.getTabModalPromptBox(aboutAddonsBrowser); I'd rather we make this modal to the options browser rather than to the entire tab. ::: toolkit/mozapps/extensions/content/extensions.js:4173 (Diff revision 1) > + return null; > + }, > +}; > + > +// Lazily retrieve the window that contains the about:addons page. > +XPCOMUtils.defineLazyGetter(gBrowser, "parentWindow", () => { Please don't cache this. Tabs can be moved between windows. ::: toolkit/mozapps/extensions/content/extensions.js:4174 (Diff revision 1) > + const {parent} = window.QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDocShell); > + > + if (parent) { > + return parent.QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDOMWindow); > + } This won't get us the parent window. It will just give us the about:addons window again. Anyway, this should be something like `document.docShell.chromeEventHandler.ownerGlobal`

Comment on attachment 8909881 [details] Bug 1385548 - Part 1: Support tab modals in WebExtensions options_ui pages. https://reviewboard.mozilla.org/r/181372/#review187228 > This won't get us the parent window. It will just give us the about:addons window again. > > Anyway, this should be something like `document.docShell.chromeEventHandler.ownerGlobal` Sounds great, I like the alternative path that you suggested much more ;-) Change applied to the updated version of this patch. (Just to be precise, the previous version was also returning the actual parent window: in the other patch there is the new test, and if the previous version was returning the about:addons page itself, then `parentWindow.gBrowser.getTabModalPromptBox` would be a recursive call, which would definitely lead the test to fail and for sure it would have never opened any modal)

Comment on attachment 8909881 [details] Bug 1385548 - Part 1: Support tab modals in WebExtensions options_ui pages. https://reviewboard.mozilla.org/r/181372/#review187688 r=me with issues fixed ::: browser/base/content/tabbrowser.xml:6001 (Diff revision 2) > + const window = event.target.ownerGlobal; > + const principal = window.document.nodePrincipal; > + > + // The add-on manager is a special case, since it contains extension > + // options pages in <browser> frames. > + if (Services.scriptSecurityManager.isSystemPrincipal(principal) && > + window.location.href === "about:addons" && > + event.target.tagName === "browser" && > + event.target.getAttribute("id") === "addon-options") { > + // Focus the about:addons tab. > + tabForEvent = this._getTabForContentWindow(window); > + } No need for all of this. Just `tabForEvent = this._getTabForContentWindow(event.target.ownerGlobal)` should be enough, and bailing out if it's null. But we really don't need this logic at all. We don't want the alert to be modal to the entire tab, only to the options panel, so we can ignore everything we'd normally do for a tab-modal alert, and just make sure we handle this case cleanly. ::: toolkit/components/addoncompat/RemoteAddonsParent.jsm:441 (Diff revision 2) > return target; > } else if (target.localName == "tab") { > return target.linkedBrowser; > } > > // Check if |target| is somewhere on the patch from the Please s/patch/path/ while you're here. ::: toolkit/components/addoncompat/RemoteAddonsParent.jsm:445 (Diff revision 2) > + // NOTE: some privileged window defines its own gBrowser stub > + // (e.g. the about:addons page and the WebExtensions sidebars), > + // which is not a XUL element and so calling `target.contains` > + // on it will raise an exception. No need for so much detail, or all-caps. Just something like "Some non-browser windows define gBrowser globals which are not elements and can't be passed to target.contains()" should suffice. ::: toolkit/mozapps/extensions/content/extensions.js:3676 (Diff revision 2) > try { > if (this._addon.optionsType == AddonManager.OPTIONS_TYPE_INLINE_BROWSER) { > whenViewLoaded(async () => { > await this._addon.startupPromise; > > - let browser = await this.createOptionsBrowser(rows); > + const optionsBrowserContainer = await this.createOptionsBrowser(rows); s/optionsBrowserContainer/browserContainer/ please. The smaller a variable's scope, the shorter its name should be, as a rule. ::: toolkit/mozapps/extensions/content/extensions.js:4168 (Diff revision 2) > +// (See Bug 1385548 for rationale). > +var gBrowser = { > + getTabModalPromptBox(browser) { > + const parentWindow = document.docShell.chromeEventHandler.ownerGlobal; > + > + if (parentWindow && parentWindow.gBrowser instanceof Ci.nsIDOMXULElement) { No need for the `instanceof` check or null check. If we have a parent window with a gBrowser global, it had better be what we're expecting. And Boris is trying to get rid of `nsIDOM*` interfaces. Just `if (parentWindow.gBrowser)` should suffice.

Comment on attachment 8909884 [details] Bug 1385548 - Part 2: Add new test for tab modals created from a WebExtensions options_ui page. https://reviewboard.mozilla.org/r/181378/#review187692 r=me with issues addressed. ::: browser/components/extensions/test/browser/browser_ext_optionsPage_modals.js:29 (Diff revision 2) > + }); > + > + browser.test.sendMessage("options-ui-loaded"); > + } > + > + const ID = "options_modals@tests.mozilla.org"; ID shouldn't be necessary. ::: browser/components/extensions/test/browser/browser_ext_optionsPage_modals.js:57 (Diff revision 2) > + > + let webpageTab = await BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.com/"); > + > + await extension.startup(); > + > + await extension.awaitMessage("options-ui-loaded"); No need for this message or "options-ui-alert". Just have the options page open the alert as soon as it loads. ::: browser/components/extensions/test/browser/browser_ext_optionsPage_modals.js:59 (Diff revision 2) > + let aboutAddonsBrowser = gBrowser.selectedBrowser; > + let aboutAddonsTab = gBrowser.selectedTab; No need to store these. ::: browser/components/extensions/test/browser/browser_ext_optionsPage_modals.js:62 (Diff revision 2) > + await extension.awaitMessage("options-ui-loaded"); > + > + let aboutAddonsBrowser = gBrowser.selectedBrowser; > + let aboutAddonsTab = gBrowser.selectedTab; > + > + Assert.equal(aboutAddonsBrowser.currentURI.spec, "about:addons", Nit: s/Assert.// ::: browser/components/extensions/test/browser/browser_ext_optionsPage_modals.js:78 (Diff revision 2) > + aboutAddonsBrowser.removeEventListener("DOMWillOpenModalDialog", onModalDialog, true); > + resolve(); > + }, true); > + }); > + > + info("Select the previous tab"); This doesn't seem accurate. In any case, please don't bother testing tab focus. All we care about is that alerts marginally work. ::: browser/components/extensions/test/browser/browser_ext_optionsPage_modals.js:85 (Diff revision 2) > + extension.sendMessage("options-ui-alert"); > + > + await onceTabModalOpened; > + > + let stack = aboutAddonsBrowser.parentNode; > + let dialogs = stack.getElementsByTagName("tabmodalprompt"); Nit: `dialog = stack.querySelector("tabmodalprompt")` ::: browser/components/extensions/test/browser/browser_ext_optionsPage_modals.js:89 (Diff revision 2) > + let stack = aboutAddonsBrowser.parentNode; > + let dialogs = stack.getElementsByTagName("tabmodalprompt"); > + > + // For remote extensions, the stack that contains the tabmodalprompt elements > + // is the parent of the extensions options_ui browser element. > + if (Services.prefs.getBoolPref("extensions.webextensions.remote")) { `if (!WebExtensionPolicy.isExtensionProcess)` ::: browser/components/extensions/test/browser/browser_ext_optionsPage_modals.js:94 (Diff revision 2) > + if (Services.prefs.getBoolPref("extensions.webextensions.remote")) { > + const optionsBrowser = aboutAddonsBrowser.contentDocument.getElementById("addon-options"); > + dialogs = optionsBrowser.parentNode.getElementsByTagName("tabmodalprompt"); > + } > + > + Assert.equal(dialogs.length, 1, "Expect a tab modal opened for the about addons tab"); s/Assert.// Same below.

Comment on attachment 8909884 [details] Bug 1385548 - Part 2: Add new test for tab modals created from a WebExtensions options_ui page. https://reviewboard.mozilla.org/r/181378/#review187692 > `if (!WebExtensionPolicy.isExtensionProcess)` I opted to use `WebExtensionPolicy.useRemoteWebExtensions` to replace the previous `Services.prefs.getBoolPref("extensions.webextensions.remote")` here. > s/Assert.// > > Same below. `equal` (without `Assert.`) seems to be available in the xpcshell tests, but it doesn't seem to be currently available in the mochitest browser tests. I looked at the other mochitest browser tests and it looks that we are using `Assert.equal` in everyone of them (the ones that use the `equal` assertion at least) and so I'm currently leaving it as it is, so that it is consistent with the other mochitest browser tests.

Comment on attachment 8909884 [details] Bug 1385548 - Part 2: Add new test for tab modals created from a WebExtensions options_ui page. https://reviewboard.mozilla.org/r/181378/#review187692 > I opted to use `WebExtensionPolicy.useRemoteWebExtensions` to replace the previous `Services.prefs.getBoolPref("extensions.webextensions.remote")` here. Please use `!isExtensionProcess` instead. What we care about here is whether the extension loads in *this* process, not whether remote extensions are enabled.

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #25) > Comment on attachment 8909884 [details] > Bug 1385548 - Part 2: Add new test for tab modals created from a > WebExtensions options_ui page. > > https://reviewboard.mozilla.org/r/181378/#review187692 > > > I opted to use `WebExtensionPolicy.useRemoteWebExtensions` to replace the previous `Services.prefs.getBoolPref("extensions.webextensions.remote")` here. > > Please use `!isExtensionProcess` instead. What we care about here is whether > the extension loads in *this* process, not whether remote extensions are > enabled. I've just updated the patch, I opted to check the isRemoteBrowser property on the options_ui browser element (which is going to be true if the options_ui extension page is running in the extension process) and then retrieve the stack XUL element where the modals are supposed to be added accordingly.

Pushed by luca.greco@alcacoop.it: https://hg.mozilla.org/integration/autoland/rev/f42d24d8aa41 Part 1: Support tab modals in WebExtensions options_ui pages. r=kmag https://hg.mozilla.org/integration/autoland/rev/a4de1a9d965a Part 2: Add new test for tab modals created from a WebExtensions options_ui page. r=kmag

https://hg.mozilla.org/mozilla-central/rev/f42d24d8aa41 https://hg.mozilla.org/mozilla-central/rev/a4de1a9d965a

This issue is verified as fixed on Firefox 58.0a1 (20171011220113) under Wind 7 64-bit and Ubuntu 16.04 32-bit. The error message window.gBrowser is undefined RemotePrompt.jsm:41 is no longer displayed in the browser console. Please see the attached files.

Any chance to uplift this to Firefox 57?

Unfortunately I don't think this meets the requirements to uplift into 57, which are very tight at this time and should be something that's a serious regression. This is annoying, but there are work arounds, like loading options in its own page.

Just like Andy said.