With e10s enabled, can't open https URLs from anchor tags in moz-extension iframe

RESOLVED FIXED in Firefox 57

Status

P1
normal
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: _6a68, Assigned: kmag)

Tracking

unspecified
mozilla57

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

a year ago
Firefox Screenshots, a new system addon / embedded webextension, shows an onboarding page when it's first used. That page is loaded into an iframe at the URL moz-extension://b1b72830-2aba-a842-875b-98578724cb48/blank.html.

The onboarding tour contains regular anchor tags that point to the privacy and terms pages, which are https://mozilla.org URLs.

With e10s enabled in Nightly, clicking those tags opens blank new tabs.
With e10s disabled in Nightly, clicking those tags opens new tabs at the correct URLs.

Kris's comments in IRC: "I think something screwy is going on with E10sUtils trying to change the process based on the frame URL..."

Brief steps to reproduce:
- Open Nightly in a fresh profile
- Go to about:config and set 'extensions.screenshots.system-disabled' to false
- Open example.com in a new tab
- Click the Screenshots toolbar button, triggering the onboarding overlay
- Click the 'terms' or 'privacy' links at the bottom of the first screen of the onboarding flow
- A new tab is opened with no URL
- Switch e10s off and try again, and the links will work

Related to https://github.com/mozilla-services/screenshots/issues/2699.

Updated

a year ago
Assignee: nobody → kmaglione+bmo
Priority: -- → P1
Whiteboard: triaged

Comment 1

a year ago
We are having a similar issue with our extension and it seems to be introduced in Firefox 53.
First we could only reproduce it on 32 bit Firefox but since this afternoon 64 bit is also affected at least on windows 10.

https://github.com/markus-hartung-avira/extension-iframe-bug

It's a little example extension, originally used for a chrome bug so don't mind the readme.

It will inject an extension hosted iframe into every page, following the link in the iframe will lead to opening a blank page as already described by Jared.

Updated

a year ago
webextensions: --- → ?
I wonder if this is a similar problem to blob handling in web-ext tabs, bug 1331176.  I traced into this issue a while back, and it seemed to have something to do with transitioning the current frame to a different origin (I didn't verify that is indeed the case).   I found that opening the blob (from anchor tag) into a new tab worked fine, but trying to open in the current tab resulted in a blank page.

Comment 3

a year ago
Here is a reduced case that might make testing easier:

https://gist.github.com/kentbrew/190a0ad83f037567eb2393c235e78043

Thanks for your work on this!

Comment 4

a year ago
I've just installed Firefox 54.0 and am seeing the same thing.
(Assignee)

Updated

a year ago
Duplicate of this bug: 1376308
(Assignee)

Updated

a year ago
Duplicate of this bug: 1376607
See Also: → bug 1331176
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

a year ago
mozreview-review
Comment on attachment 8882684 [details]
Bug 1357589: Part 2 - Test that opening web URLs from extension iframes works correctly.

https://reviewboard.mozilla.org/r/153758/#review158968

r+ w/comment addressed

::: toolkit/components/extensions/test/mochitest/test_ext_new_tab_processType.html:70
(Diff revision 1)
> +
> +  {
> +    synthesizeMouseAtCenter(link, {}, iframe.contentWindow);
> +    let {subject: doc} = await promiseObserved("document-element-inserted", doc => doc.documentURI === linkURL);
> +    info("Link opened");
> +    doc.defaultView.close();

await BrowserTestUtils.closeWindow(win); 
// or similar

Window closing can cause intermittent issues if we don't wait for the window to close.  Ditto for second close.
Attachment #8882684 - Flags: review?(mixedpuppy) → review+
(Assignee)

Comment 10

a year ago
mozreview-review-reply
Comment on attachment 8882684 [details]
Bug 1357589: Part 2 - Test that opening web URLs from extension iframes works correctly.

https://reviewboard.mozilla.org/r/153758/#review158968

> await BrowserTestUtils.closeWindow(win); 
> // or similar
> 
> Window closing can cause intermittent issues if we don't wait for the window to close.  Ditto for second close.

This isn't a browser test, so that's not an option.

Comment 11

a year ago
mozreview-review
Comment on attachment 8882683 [details]
Bug 1357589: Part 1 - Use the correct default remote type for content-created tabs/windows.

https://reviewboard.mozilla.org/r/153756/#review159358

This seems okay, but I'm requesting an additional review from mystor - specifically because of the work he did in bug 1303196, which seems relevant.
Attachment #8882683 - Flags: review?(mconley) → review+
Attachment #8882683 - Flags: review?(michael)

Comment 12

a year ago
mozreview-review
Comment on attachment 8882683 [details]
Bug 1357589: Part 1 - Use the correct default remote type for content-created tabs/windows.

https://reviewboard.mozilla.org/r/153756/#review159368

I think this is the wrong approach for 2 reasons:
1. The information that we need to choose the process to load into is already present in another parameter, aNextTabParentId, and
2. re-using the aOpener parameter for a completely different codepath which is almost completely unrelated is gross.

I would instead suggest doing something different. From reading the code it looks like we already do most of the right thing (namely we override any provided remoteType if the next tab parent ID is set), but we don't update the remoteType attribute, so the frontend code gets confused and does the wrong thing. I think we should try to change that and see if it fixes the problem.

I think this should be doable by setting the remoteType attribute in this if to match the remote type of the selected TabParent's ContentParent: http://searchfox.org/mozilla-central/rev/e8f4f51cd543f203e9cb861cecb7545ac43c836c/dom/ipc/ContentParent.cpp#1188-1196, as we do this check before we read the remoteType, it should override any remoteType decisions which the frontend code has made.

Let me know if this doesn't work, and I can help you look into other ways to avoid duplicating this information.
Attachment #8882683 - Flags: review?(michael) → review-
I also feel the need to note that another workaround to this issue right now would be to add `rel="noopener"` to your outbound links, which should also fix this problem for the webextension. Not exactly a great solution for the future, but I figured I should mention it.
(Assignee)

Comment 14

a year ago
I went this route rather than updating the attribute from the frameloader mainly because we actually need the opener browser for other reasons (for bug 1238314, and other cases where we don't set the correct owner for windows opened from background tabs).

And I'm also not sure we should override the remoteType returned by E10SUtils even if we do have a nextTabParentId that doesn't match. Though I suppose we could leave out the remoteType attribute when we have a nextTabParentID, and then set the remoteType attribute based on the type of the TabParent.

Either way, though, I need access to the opener browser at this point in tabbrowser.xml.
(Assignee)

Comment 15

a year ago
(In reply to Michael Layzell [:mystor] from comment #13)
> I also feel the need to note that another workaround to this issue right now
> would be to add `rel="noopener"` to your outbound links, which should also
> fix this problem for the webextension. Not exactly a great solution for the
> future, but I figured I should mention it.

That was one of the first things we tried, but it didn't work. I don't remember the exact reasons. But either way, we need a real fix.
(In reply to Kris Maglione [:kmag] from comment #14)
> I went this route rather than updating the attribute from the frameloader
> mainly because we actually need the opener browser for other reasons (for
> bug 1238314, and other cases where we don't set the correct owner for
> windows opened from background tabs).

When noopener is set, I imagine that we still want to have the browser.tabs.openerTabId property set for the originating window, but we don't want to require that they are loaded in the same content process, which makes me think that we should keep the two responsibilities separate.

> And I'm also not sure we should override the remoteType returned by
> E10SUtils even if we do have a nextTabParentId that doesn't match. Though I
> suppose we could leave out the remoteType attribute when we have a
> nextTabParentID, and then set the remoteType attribute based on the type of
> the TabParent.

My understanding from having worked with some of this code in the past is that our code often assumes that the remoteType attribute is up to date, and will cause problems if it is not. We should probably at least assert that they match.

> Either way, though, I need access to the opener browser at this point in
> tabbrowser.xml.

If we need it for this case, perhaps we could standardize on always passing around a nsIFrameLoader or nsIFrameLoaderOwner, and simply fetch the opener window in the non-e10s case from the opener nsIFrameLoader's .docShell property. I don't really like that aOpener is sometimes a nsIFrameLoaderOpener and sometimes a nsIDOMWindow.
(In reply to Kris Maglione [:kmag] from comment #15)
> That was one of the first things we tried, but it didn't work. I don't
> remember the exact reasons. But either way, we need a real fix.

The change which made this work only landed a couple of weeks ago, so I'm not surprised.
(Assignee)

Comment 18

a year ago
(In reply to Michael Layzell [:mystor] from comment #16)
> (In reply to Kris Maglione [:kmag] from comment #14)
> > I went this route rather than updating the attribute from the frameloader
> > mainly because we actually need the opener browser for other reasons (for
> > bug 1238314, and other cases where we don't set the correct owner for
> > windows opened from background tabs).
>
> When noopener is set, I imagine that we still want to have the
> browser.tabs.openerTabId property set for the originating window, but we
> don't want to require that they are loaded in the same content process,
> which makes me think that we should keep the two responsibilities separate.

This patch won't require that they be loaded into the same content process, it
just makes the preferred process type the process type of the opener. I
suspect that's what we want in most cases, and it's definitely what we want in
the case of moz-extension: URLs opened from web content processes, since they
have a more limited set of privileges when running in web content processes
than they do when running in extension processes.

> > And I'm also not sure we should override the remoteType returned by
> > E10SUtils even if we do have a nextTabParentId that doesn't match. Though I
> > suppose we could leave out the remoteType attribute when we have a
> > nextTabParentID, and then set the remoteType attribute based on the type of
> > the TabParent.
>
> My understanding from having worked with some of this code in the past is
> that our code often assumes that the remoteType attribute is up to date, and
> will cause problems if it is not. We should probably at least assert that
> they match.

That probably makes sense.

> > Either way, though, I need access to the opener browser at this point in
> > tabbrowser.xml.
>
> If we need it for this case, perhaps we could standardize on always passing
> around a nsIFrameLoader or nsIFrameLoaderOwner, and simply fetch the opener
> window in the non-e10s case from the opener nsIFrameLoader's .docShell
> property. I don't really like that aOpener is sometimes a
> nsIFrameLoaderOpener and sometimes a nsIDOMWindow.

I don't particularly like it either, but we a higher degree of granularity
when the opener is in the same process, since it can be a frame rather than a
top-level window, and remote frameloaders (almost) always point to top-level
windows.

I guess we could add a separate topLevelOpener attribute, or something like
that, that's always a nsIFrameLoaderOwner, though...
> I guess we could add a separate topLevelOpener attribute, or something like
> that, that's always a nsIFrameLoaderOwner, though...

Perhaps we could call that aOpeningTab?
(Assignee)

Updated

a year ago
Duplicate of this bug: 1385732
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 23

a year ago
mozreview-review
Comment on attachment 8882683 [details]
Bug 1357589: Part 1 - Use the correct default remote type for content-created tabs/windows.

https://reviewboard.mozilla.org/r/153756/#review171238

::: dom/interfaces/base/nsIBrowserDOMWindow.idl:23
(Diff revision 2)
>    readonly attribute boolean isPrivate;
>    attribute nsIPrincipal triggeringPrincipal;
>  
> +  // The browser or frame element in the parent process which holds the
> +  // opener window in the content process. May be null.
> +  readonly attribute nsIFrameLoaderOwner opener;

Please call this openerBrowser rather than opener. I think that clarifying which of the two we're talking about is important here.

(also change mOpener -> mOpenerBrowser on nsOpenURIInFrameParams :-))

::: dom/ipc/ContentParent.cpp:4550
(Diff revision 2)
>        return IPC_OK();
>      }
>  
> +    nsCOMPtr<nsIFrameLoaderOwner> opener;
> +    if (frame) {
> +      opener = do_QueryInterface(frame);

nit: You can pass nullptr to do_QueryInterface and we handle it correctly, so we don't need this if statement.
Attachment #8882683 - Flags: review?(michael) → review+
(Assignee)

Comment 24

a year ago
mozreview-review
Comment on attachment 8882683 [details]
Bug 1357589: Part 1 - Use the correct default remote type for content-created tabs/windows.

https://reviewboard.mozilla.org/r/153756/#review174732

::: dom/ipc/ContentParent.cpp:1122
(Diff revision 2)
>      if (TabParent* parent =
>            sNextTabParents.GetAndRemove(aNextTabParentId).valueOr(nullptr)) {
>        MOZ_ASSERT(!parent->GetOwnerElement(),
>                   "Shouldn't have an owner elemnt before");
> +      MOZ_ASSERT(remoteType == static_cast<ContentParent*>(parent->Manager())->GetRemoteType(),
> +                 "Browser remote type must match tab parent remote type");

I had to remove this, since it triggered failed assertions in tests for URLs opened from file processes in a new window rather than a new tab.

I'll re-add it in a follow-up, since that puts us in a pretty bad position where the remoteType attribute of the browser doesn't match the actual remote type of the process.
(Assignee)

Comment 25

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5f70ddc8d2ee80a129f868eab9ae5be62478653
Bug 1357589: Part 1 - Use the correct default remote type for content-created tabs/windows. r=mconley r=mystor

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5143747b59da0a16e1833f134ee2aab10b85e81
Bug 1357589: Part 2 - Test that opening web URLs from extension iframes works correctly. r=mixedpuppy

Comment 26

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b5f70ddc8d2e
https://hg.mozilla.org/mozilla-central/rev/d5143747b59d
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Updated

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