1394520 - Links with target=_blank don't load with e10s

Status: RESOLVED FIXED

(GeckoView Graveyard :: Sandboxing, enhancement)

Description

[Eugen Sawin [:esawin]]

In bug 1377580 we've enabled handling of external links in GeckoView with in e10s disabled, in this bug we address the e10s case.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1394520-1.0-Extend-nsIBrowserDOMWindow-to-suppor.patch

This is based on the approach used in bug 1377580, we extend nsIBrowserDOMWindow to distinguish between opening an URI and creating a window while still passing the URI through.

This time I found it easier (requires fewer Gecko changes) to keep the URI optional for the openURI case.

Comment 2

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1394520-2.0-Add-support-for-external-URI-loading.patch

The implementation of createContentWindowInFrame for GeckoView is identical to its createContentWindow implementation (except for the arguments), but it might be better to keep them separated for future changes.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

In general, could you create diffs with -p -U 8

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8901996 [details] [diff] [review]
0001-Bug-1394520-1.0-Extend-nsIBrowserDOMWindow-to-suppor.patch


>+++ b/browser/base/content/browser.js
>@@ -5346,6 +5346,13 @@ nsBrowserAccess.prototype = {
>     return newWindow;
>   },
> 
>+  createContentWindowInFrame: function browser_createContentWindowInFrame(
>+                              aURI, aParams, aWhere, aFlags, aNextTabParentId,
>+                              aName) {
>+    return this.openURIInFrame(null, aParams, aWhere, aFlags, aNextTabParentId,
>+                               aName);
This definitely needs some comment why null is passed as uri to openURIInFrame.

>+++ b/dom/interfaces/base/nsIBrowserDOMWindow.idl
>@@ -26,7 +26,7 @@ interface nsIOpenURIInFrameParams : nsISupports
>   readonly attribute jsval openerOriginAttributes;
> };
> 
>-[scriptable, uuid(2a9bb880-5d73-40f3-8152-c60c8d137a14)]
>+[scriptable, uuid(b7f0dff6-75bf-4ae7-ae68-205f17b87180)]
no need for this change.

>     nsCOMPtr<nsIFrameLoaderOwner> frameLoaderOwner;
>-    aResult = browserDOMWin->OpenURIInFrame(aURIToLoad, params, openLocation,
>-                                            nsIBrowserDOMWindow::OPEN_NEW,
>-                                            aNextTabParentId, aName,
>-                                            getter_AddRefs(frameLoaderOwner));
>+    if (aURIToLoad && aLoadURI) {
>+      aResult = browserDOMWin->OpenURIInFrame(aURIToLoad,
>+                                              params, openLocation,
>+                                              nsIBrowserDOMWindow::OPEN_NEW,
>+                                              aNextTabParentId, aName,
>+                                              getter_AddRefs(frameLoaderOwner));
>+    } else {
>+      aResult = browserDOMWin->CreateContentWindowInFrame(aURIToLoad,
>+                                              params, openLocation,
>+                                              nsIBrowserDOMWindow::OPEN_NEW,
>+                                              aNextTabParentId, aName,
>+                                              getter_AddRefs(frameLoaderOwner));
>+    }
Why aURIToLoad needs to be non-null? Does this work with something like
window.open(null, null, "noopener"); 
(or looks like that crashes currenly, but it should work)


r- because I'd like to see some comments in the code and clarification how passing null url is supposed to work.

Comment 5

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1.1-Extend-nsIBrowserDOMWindow-to-support-conten.patch

Sorry for the delay.
I've addressed your comments - null-URIs should now be accepted (loading "about:blank").

Comment 6

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-2.1-Add-support-for-external-URI-loading-with-e1.patch

Rebased using the new load-URI handler.

Comment 7

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1394520-3.0-Translate-null-URI-to-about-blank-fo.patch

We should accept null-URIs and load "about:blank" same as the other platforms.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8913341 [details] [diff] [review]
0001-Bug-1.1-Extend-nsIBrowserDOMWindow-to-support-conten.patch

Passing aURIToLoad and aLoadURI is rather confusing.
Please document those CommonCreateWindow params.

Comment 9

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1.2-Extend-nsIBrowserDOMWindow-to-support-conten.patch

Added code comment.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f980b17c3b9
[1.2] Extend nsIBrowserDOMWindow to support content window creation without URI loading with e10s. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e176fee9dc7e
[2.1] Add support for external URI loading with e10s in GeckoView. r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bffa263522b
[3.0] Translate null-URI to "about:blank" for URI loading requests. r=snorp

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/7f980b17c3b9
https://hg.mozilla.org/mozilla-central/rev/e176fee9dc7e
https://hg.mozilla.org/mozilla-central/rev/2bffa263522b

Comment 12

[Chris Peterson [:cpeterson]]

Moving some e10s bugs to the new GeckoView::Sandboxing component.