1532752 - URL shown in URL bar when download fails to close a new window changed

Status: NEW

(Core :: DOM: Navigation, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE:

1. Create an HTML page like so:

  <a href="https://github.com/Rohitbels/githubDB/archive/master.zip"
     rel="noreferrer"
     target="_blank">Click me</a>

2. Load it.
3. Click the link.

The tab should close when the helper app dialog comes up, but does not due to bug 1531289. But that's not what this bug is about. This bug is about what URL shows up in the URL bar. It used to be "about:blank", but became the URL of the thing being downloaded somewhere in this commit range: https://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2017-05-23&enddate=2017-05-24

Note that https://github.com/Rohitbels/githubDB/archive/master.zip does an HTTP redirect to https://codeload.github.com/Rohitbels/githubDB/zip/master so it's pretty plausible that bug 1319111 could have changed behavior here.

It's not clear to me whether the new behavior is better or worse... Probably somewhat worse, because I expect the actual page being shown here is still about:blank.

I don't know whether that could be used for spoofing somehow...

Comment 1

[:Gijs (he/him)]

I bet that because the page that's loaded is the initial about:blank, and some other code that tries to ensure we update the URL bar (a) quickly and (b) don't make it flicker / revert to about:blank and then back again.

I created a quick glitch for the example in comment #0: https://peaceful-parakeet.glitch.me/ .

The nodePrincipal of the resulting about:blank window is a null principal, so I don't believe this is exploitable, in the sense that I don't think the opener can write to the document without navigating, which presumably trips location bar updates. I guess it's possible that there's a way to trip the same thing with window.open() that doesn't have that problem; I haven't dug very deeply.

Marco, do you have cycles to look at this? I'm swamped atm. I'll keep my needinfo for a bit in case you're also really busy.

Comment 2

[Boris Zbarsky [:bzbarsky]]

The nodePrincipal of the resulting about:blank window is a null principal

Because we opened it in a new process. I wonder what happens if we remove the noreferrer bit but then make the site in question return an HTTP 204 response...

Comment 3

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #2)

The nodePrincipal of the resulting about:blank window is a null principal

Because we opened it in a new process. I wonder what happens if we remove the noreferrer bit but then make the site in question return an HTTP 204 response...

Not sure I follow. You mean the link just returns a 204? Or also returns content + a mimetype that trips the external handler? Would it still need the redirect?

Comment 4

[Boris Zbarsky [:bzbarsky]]

I mean the link returns a 204; a 204 doesn't have content. Probably best to still have the redirect to have a close basis for comparison.

Comment 5

[Honza Bambas (:mayhemer)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #0)

It's not clear to me whether the new behavior is better or worse... Probably somewhat worse, because I expect the actual page being shown here is still about:blank.

I don't know whether that could be used for spoofing somehow...

Checked that the redirect has no effect on this bug's manifestation, if I have the same setup <a> directing to the end url, it's shown in the URL bar as well.

I don't know where the URL bar is fed the URL. If you point me there, I can try to hunt its source.

Note that doing a test using the same <a> setup redirecting to a URL that responses with 302 after a delay (php usleep) we render the source URL immediately in the address bar.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I don't know where the URL bar is fed the URL

I don't know offhand, for cases that aren't resulting in actual navigation in the docshell...

Comment 7

[Honza Bambas (:mayhemer)]

I don't know how to move on with this w/o either investing some significant time into finding the address bar URL source or w/o someone navigating me to it.

Comment 8

[:Gijs (he/him)]

The value is set at https://searchfox.org/mozilla-central/rev/7abb9117c8500ed20833746c9f8e800fce3a4688/browser/base/content/tabbrowser.js#2644-2649 .

Re-reading, I don't really understand why this is treated as a new thing... the same thing happens without rel="noreferrer", right?

Put differently... it seems to me we have always done this, and I don't know how to trace why we don't remove the URL because I don't know a "good" case - ie, how can I see the "correct" behavior here?

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK, Gijs and I talked offline, and tested a 2017-05-25 nightly on Mac, and that shows "about:blank" too. So bug 1319111 is definitely off the hook.

Adrian, could you please double-check the regression range you ended up with in bug 1531289 comment 1?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Alright, that regression range is definitely wrong. Gijs checked, and the actual behavior change is from bug 1370971. And in fact, flipping that pref ("dom.noopener.newprocess.enabled") back to false changes the behavior back to about:blank.

So I think there's still kinda a bug here, due to the inconsistency between same-process and different process.

In the different-process case, ContentChild::ProvideWindowCommon sends CreateWindowInDifferentProcess, which ends up doing ContentParent::CommonCreateWindow with aLoadURI == true. That then calls nsIBrowserDOMWindow::OpenURIInFrame.

In the same-process case, ContentChild::ProvideWindowCommon sends CreateWindow, which ends up doing ContentParent::CommonCreateWindow with aLoadURI == false. That calls nsIBrowserDOMWindow::CreateContentWindowInFrame.

Now if you compare https://searchfox.org/mozilla-central/source/browser/base/content/browser.js#5721-5725 to https://searchfox.org/mozilla-central/source/browser/base/content/browser.js#5713-5719 (the browser.js implementations of openURIInFrame and createContentWindowInFrame), the latter passes a null URI to getContentWindowOrOpenURIInFrame. So it causes addTab to be called with no URI and the URL bar does not get set to anything interesting. But openURIInFrame passes the URI through, so ends up setting the URL bar to that URI.

It's still not 100% clear to me what behavior we really want here, but we should definitely have it be consistent for the same-process vs different-process cases!

Comment 11

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #10)

Alright, that regression range is definitely wrong. Gijs checked, and the actual behavior change is from bug 1370971. And in fact, flipping that pref ("dom.noopener.newprocess.enabled") back to false changes the behavior back to about:blank.

So I think there's still kinda a bug here, due to the inconsistency between same-process and different process.

In the different-process case, ContentChild::ProvideWindowCommon sends CreateWindowInDifferentProcess, which ends up doing ContentParent::CommonCreateWindow with aLoadURI == true. That then calls nsIBrowserDOMWindow::OpenURIInFrame.

In the same-process case, ContentChild::ProvideWindowCommon sends CreateWindow, which ends up doing ContentParent::CommonCreateWindow with aLoadURI == false. That calls nsIBrowserDOMWindow::CreateContentWindowInFrame.

Now if you compare https://searchfox.org/mozilla-central/source/browser/base/content/browser.js#5721-5725 to https://searchfox.org/mozilla-central/source/browser/base/content/browser.js#5713-5719 (the browser.js implementations of openURIInFrame and createContentWindowInFrame), the latter passes a null URI to getContentWindowOrOpenURIInFrame. So it causes addTab to be called with no URI and the URL bar does not get set to anything interesting. But openURIInFrame passes the URI through, so ends up setting the URL bar to that URI.

It's still not 100% clear to me what behavior we really want here, but we should definitely have it be consistent for the same-process vs different-process cases!

Going to be slightly controversial: I think we want the URL to show up (in the general case), per bug 610357 and the mess of already-fixed bugs in that department. Basically, we want the URL bar to show the user where they're heading especially in an otherwise empty/clean tab, which is what the tabbrowser.js code does.

If/when the load is redirected to the unknownContentType dialog and its friends and/or doing a download, we want it to be cleared. But the URL should show up so the user has feedback about the fact that they opened a URL.

So I think we need to do 2 things:

1. fix the non-OOP case to also send the URL (and/or figure out if there was a conscious decision that led to it not doing so)
2. fix the case where we don't load a document to force a URL bar update so we show about:blank.

I'm a little bit confused about where to do the latter, because casual source trawling would suggest somewhere around https://searchfox.org/mozilla-central/rev/7abb9117c8500ed20833746c9f8e800fce3a4688/uriloader/exthandler/nsExternalHelperAppService.cpp#1569 ... which is clearly not working atm, viz. bug 1531289.

Comment 12

[Boris Zbarsky [:bzbarsky]]

I think we want the URL to show up (in the general case)

Fwiw, that seems reasonable to me.

fix the non-OOP case to also send the URL (and/or figure out if there was a conscious decision that led to it not doing so)

It's already doing that as far as getting to createContentWindowInFrame. So this would need to be done on the UI side...

I'm a little bit confused about where to do the latter

We could do it in the place where we currently aim to close the window. The "reset to about:blank" could be done independently of the opener situation, so should not have the problems bug 1531289 has, right? That said, it would definitely be related to bug 1531289 in the sense that right now we don't even propagate the "load in a new window" state there...

Comment 13

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #12)

I think we want the URL to show up (in the general case)

Fwiw, that seems reasonable to me.

fix the non-OOP case to also send the URL (and/or figure out if there was a conscious decision that led to it not doing so)

It's already doing that as far as getting to createContentWindowInFrame. So this would need to be done on the UI side...

Huh, yeah. The comment at https://searchfox.org/mozilla-central/rev/7abb9117c8500ed20833746c9f8e800fce3a4688/browser/base/content/browser.js#5716 indicates not passing it through is intentional, but from the bug I don't understand why this is done this way.

Eugen, can you clarify (as you wrote this in bug 1394520)? Why don't we just pass the URL? The documentation for https://searchfox.org/mozilla-central/rev/7abb9117c8500ed20833746c9f8e800fce3a4688/dom/interfaces/base/nsIBrowserDOMWindow.idl#117-126 doesn't really imply that we must "only" create the content window without loading the URL...

Comment 14

[Eugen Sawin [:esawin]]

(In reply to :Gijs (he/him) from comment #13)

Eugen, can you clarify (as you wrote this in bug 1394520)? Why don't we just pass the URL? The documentation for https://searchfox.org/mozilla-central/rev/7abb9117c8500ed20833746c9f8e800fce3a4688/dom/interfaces/base/nsIBrowserDOMWindow.idl#117-126 doesn't really imply that we must "only" create the content window without loading the URL...

Can you clarify what you mean by "Why don't we just pass the URL"?

nsIBrowserDOMWindow::createContentWindow{InFrame} should only create the content window without loading the URI. If the documentation does not reflect that properly, it needs to be fixed.

Bug 1377580 provides the broader context for the change, in short, for GeckoView, we need to split window creation and URI loading because GeckoView needs to delegate the window creation behavior/decision to the app and therefore cannot process that request synchronously.

Comment 15

[:Gijs (he/him)]

(In reply to Eugen Sawin [:esawin] from comment #14)

(In reply to :Gijs (he/him) from comment #13)

Eugen, can you clarify (as you wrote this in bug 1394520)? Why don't we just pass the URL? The documentation for https://searchfox.org/mozilla-central/rev/7abb9117c8500ed20833746c9f8e800fce3a4688/dom/interfaces/base/nsIBrowserDOMWindow.idl#117-126 doesn't really imply that we must "only" create the content window without loading the URL...

Can you clarify what you mean by "Why don't we just pass the URL"?

nsIBrowserDOMWindow::createContentWindow{InFrame} should only create the content window without loading the URI. If the documentation does not reflect that properly, it needs to be fixed.

Is that really also the case for desktop? Why? You changed desktop as well...

Bug 1377580 provides the broader context for the change, in short, for GeckoView, we need to split window creation and URI loading because GeckoView needs to delegate the window creation behavior/decision to the app and therefore cannot process that request synchronously.

This makes sense in geckoview, but I don't see why this is necessary in desktop.

Comment 16

[:Gijs (he/him)]

To be clear, I'm asking about https://searchfox.org/mozilla-central/rev/7abb9117c8500ed20833746c9f8e800fce3a4688/browser/base/content/browser.js#5716 , which won't run in geckoview, but adopted the same behavior, ie not passing along the URL when opening tabs in certain ways. It's unclear why that was done.

Comment 17

[Eugen Sawin [:esawin]]

(In reply to :Gijs (he/him) from comment #16)

To be clear, I'm asking about https://searchfox.org/mozilla-central/rev/7abb9117c8500ed20833746c9f8e800fce3a4688/browser/base/content/browser.js#5716 , which won't run in geckoview, but adopted the same behavior, ie not passing along the URL when opening tabs in certain ways. It's unclear why that was done.

Looking at the relevant diff, the behavior has not been changed on desktop, it was simply adapted to the split interface.
The null URI case had been used to indicate window creation before.

Comment 18

[:Gijs (he/him)]

(In reply to Eugen Sawin [:esawin] from comment #17)

(In reply to :Gijs (he/him) from comment #16)

To be clear, I'm asking about https://searchfox.org/mozilla-central/rev/7abb9117c8500ed20833746c9f8e800fce3a4688/browser/base/content/browser.js#5716 , which won't run in geckoview, but adopted the same behavior, ie not passing along the URL when opening tabs in certain ways. It's unclear why that was done.

Looking at the relevant diff, the behavior has not been changed on desktop, it was simply adapted to the split interface.
The null URI case had been used to indicate window creation before.

OK, this is pretty confusing, because you're linking to the diff in browser.js, but I think what you're saying is that the previous DOM-based callers of openURIInFrame used to pass null for the URI parameter prior to the patch, and now they don't anymore but, in the cases where that used to happen, they now call createContentWindowInFrame and we should treat that as if no URI was passed (despite a URI being present), in terms of not executing a load ?

Do we actually need DOM to be executing the load and setting openers for _blank links? :-(

Note that we don't currently have an abstraction in addTab that lets us pass a URL that we then don't load. I guess we could add one, though it feels pretty strange. Or we could add a hack to browser.js to just set the URL after tabbrowser gives us a tab, which might be more reasonable in the circumstances.

Comment 19

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #18)

Do we actually need DOM to be executing the load and setting openers for _blank links? :-(

:nika ?

Comment 20

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #19)

(In reply to :Gijs (he/him) from comment #18)

Do we actually need DOM to be executing the load and setting openers for _blank links? :-(

:nika ?

Oh, hm, I guess this is being fixed by the whole "implied noopener for _blank" stuff that we are shipping in 67...

Comment 21

[Neha Kochar [:neha]]

Gijs, is it bug 1522083 that you're referring to? If yes, then that's shipping in 67 - yes. What are the next steps for this bug?

Comment 22

[:Gijs (he/him)]

(In reply to Neha Kochar [:neha] from comment #21)

Gijs, is it bug 1522083 that you're referring to? If yes, then that's shipping in 67 - yes. What are the next steps for this bug?

It needs an engineer. See bug 1531289 comment 23, which is closely related.

Comment 23

[Neha Kochar [:neha]]

Adding ni for Johann and Christoph (see bug 1531289 comment 23).

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Neha Kochar [:neha] from comment #23)

Adding ni for Johann and Christoph (see bug 1531289 comment 23).

Sorry I can't be of any help here, I added info to:
https://bugzilla.mozilla.org/show_bug.cgi?id=1531289#c25 which I will also quote here which could be a potential path forward:

Unfortunately I don't have much to add here, and it seems there is not a straight forward solution.

One thing that is worth noting however, a while ago we started to block top-level data URI navigations and we had a similar problem with leaving open windows around where the data: URI was actually blocked from loading. Within Bug 1412824 we added some code [1] which actually closes the window after it was already openend. So maybe adding some special code to nsDSURIContentListener::DoContent() might be a possibility to consider for this bug as well - assuming we go through that code path and assuming we know it's a download at that point.

[1] https://searchfox.org/mozilla-central/source/docshell/base/nsDSURIContentListener.cpp#151

Comment 25

[Johann Hofmann [:johannh]]

As I said in bug 1531289, I don't have time for this, but Erica is looking into it and I'll try to assist her.

Comment 26

[Patricia Lawless]

Hi Erica, are you working on this one? (tracking for 68). Thanks!

Comment 27

[Erica Wright [:ewright]]

I haven't been working on this one, I was hoping it might reveal itself during work on Bug 1531289, but it has not.

Comment 28

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

Too late for 68 but we could still potentially take a patch for 69/70.

Comment 29

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

Happy to take a patch for 70 or beyond.
Since we are getting close to the end of the 69 beta cycle and this is set to P3, I'm marking it fix-optional for 69 and 70 to remove it from weekly triage.