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

VERIFIED FIXED in Firefox 58

Status

defect
P2
normal
VERIFIED FIXED
2 years ago
11 months ago

People

(Reporter: ke5trel, Assigned: rpl)

Tracking

(Blocks 1 bug)

unspecified
mozilla58
Dependency tree / graph

Firefox Tracking Flags

(firefox56- wontfix, firefox57- wontfix, firefox58 verified)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Reporter

Description

2 years ago
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
Reporter

Updated

2 years ago
Blocks: webext-oop
Status: UNCONFIRMED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1375490
Reporter

Comment 2

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

> window.gBrowser is undefined  RemotePrompt.jsm:41
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
See Also: → 1375490
Summary: Confirm/alert/prompt dialogs do not work from options/popup/sidebar with OOP enabled → Confirm/alert/prompt dialogs do not work from options_ui with OOP enabled

Updated

2 years ago
Priority: -- → P2

Comment 3

2 years ago
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

2 years ago
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

2 years ago
[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.
Reporter

Comment 6

2 years ago
[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.
Flags: needinfo?(amckay)

Comment 8

2 years ago
Will not be fixed for 56. Might be for 57 (its marked as a P2).
Flags: needinfo?(amckay)
Doesn't sound like we need to track this specifically.
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Assignee: nobody → lgreco
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee

Comment 14

2 years ago
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).
Attachment #8909881 - Flags: feedback?(kmaglione+bmo)

Comment 15

2 years ago
mozreview-review
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`
Attachment #8909881 - Flags: feedback?(kmaglione+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8909882 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8909883 - Attachment is obsolete: true
Assignee

Comment 18

2 years ago
mozreview-review-reply
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)
Assignee

Updated

2 years ago
Attachment #8909884 - Flags: review?(kmaglione+bmo)
Assignee

Updated

2 years ago
Attachment #8909881 - Flags: review?(kmaglione+bmo)

Comment 19

2 years ago
mozreview-review
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.
Attachment #8909881 - Flags: review?(kmaglione+bmo) → review+

Comment 20

2 years ago
mozreview-review
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.
Attachment #8909884 - Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 24

2 years ago
mozreview-review-reply
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

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 28

2 years ago
(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

2 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58

Comment 31

2 years ago
Posted file 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.

Updated

2 years ago
Status: RESOLVED → VERIFIED
Any chance to uplift this to Firefox 57?
Flags: needinfo?(amckay)

Comment 33

2 years ago
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.
Flags: needinfo?(amckay)

Updated

11 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.