1385548 - Confirm/alert/prompt dialogs do not work from options_ui with OOP enabled

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect, P2)

Description

[Kestrel]

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

Comment 2

[Kestrel]

Bug 1375490 fixed the popup and sidebar case but problem remains for options_ui.

> window.gBrowser is undefined  RemotePrompt.jsm:41

Comment 3

[mark]

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.

Comment 4

[mark]

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.

Comment 5

[mark]

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

Comment 6

[Kestrel]

[Tracking Requested - why for this release]:

Affects 56 and two of my extensions.

Comment 7

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

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.

Comment 8

[Andy McKay]

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

Comment 9

[Julien Cristau [:jcristau]]

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

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1385548 - Part 1: Support tab modals in WebExtensions options_ui pages.

This patch introduces a stub gBrowser object which allow a WebExtensions options_ui page
to open a tab modal using alert/prompt/confirm.

The about:addons page is defined at toolkit level but the TabModalPromptBox is defined
at browser level, and so to be able to provide a TabMobalPromptBox from the about:addons
page this patch uses the implementation provided by the window that contains the
about:addons tab, if any.

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

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1385548 - Part 2: Fix "tabEvent is undefined" exception for modals opened from a WebExtensions options_ui page.

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

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1385548 - Part 3: Fix exception raised by calling target.contains on the gBrowser stub defined in "about:addons".

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

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1385548 - Part 2: Add new test for tab modals created from a WebExtensions options_ui page.

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

Comment 14

[Luca Greco [:rpl] [:luca] [:lgreco]]

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 15

[Kris Maglione [:kmag]]

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 16

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8909881 [details]
Bug 1385548 - Part 1: Support tab modals in WebExtensions options_ui pages.

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

Comment 17

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8909884 [details]
Bug 1385548 - Part 2: Add new test for tab modals created from a WebExtensions options_ui page.

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

Comment 18

[Luca Greco [:rpl] [:luca] [:lgreco]]

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 19

[Kris Maglione [:kmag]]

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 20

[Kris Maglione [:kmag]]

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 = "[e-mail]";

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 21

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8909881 [details]
Bug 1385548 - Part 1: Support tab modals in WebExtensions options_ui pages.

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

Comment 22

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8909884 [details]
Bug 1385548 - Part 2: Add new test for tab modals created from a WebExtensions options_ui page.

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

Comment 23

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8909884 [details]
Bug 1385548 - Part 2: Add new test for tab modals created from a WebExtensions options_ui page.

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

Comment 24

[Luca Greco [:rpl] [:luca] [:lgreco]]

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 25

[Kris Maglione [:kmag]]

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.

Comment 26

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8909881 [details]
Bug 1385548 - Part 1: Support tab modals in WebExtensions options_ui pages.

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

Comment 27

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8909884 [details]
Bug 1385548 - Part 2: Add new test for tab modals created from a WebExtensions options_ui page.

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

Comment 28

[Luca Greco [:rpl] [:luca] [:lgreco]]

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

Comment 29

[Pulsebot]

Pushed by [e-mail]:
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

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 31

[CosminB]

Attachment: FixedIssue.zip

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.

Comment 32

[Geoffrey De Belie (Smile4ever)]

Any chance to uplift this to Firefox 57?

Comment 33

[Andy McKay]

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.

Comment 34

[Sylvestre Ledru [:Sylvestre]]

Just like Andy said.