Open Bug 1531289 Opened 8 months ago Updated 2 months ago

target=_blank (with either explicit noreferrer/noopener, or on builds that have dom.targetBlankNoOpener.enabled set to true) that results in a download opens a new tab and does not close

Categories

(Core :: Document Navigation, defect, P2)

65 Branch
defect

Tracking

()

Tracking Status
firefox-esr60 --- unaffected
firefox65 --- unaffected
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- fix-optional
firefox70 --- fix-optional

People

(Reporter: belsarerohit81, Assigned: ewright)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(2 files)

Attached file home.html

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:65.0) Gecko/20100101 Firefox/65.0

Steps to reproduce:

-Add the following anchor tag

<a href="https://github.com/Rohitbels/githubDB/archive/master.zip" 
	rel="noreferrer" 
	target="_blank" 
>Click</a>
  • Clicked on the Anchor tag

Actual results:

  • New tab was opened.
  • Firefox open or save pop up was displayed.
  • Irrespective of my selection the new tab opened did not close.
  • The blank tab had title as the URL we visited.

Expected results:

  • The tab should have closed or am I wrong?

I could reproduce this issue in Nightly67 /03.03.2019 ; Beta66.0b11 /02.25.2019 ; Release65.0.2 /02.25.2019 on Windows 10, Ubuntu 16.04, Osx 10.14.

IMO, there are two issues here:
1- the blank tab has the URL title of the href anchor; (regression? or intended fix)
2- the tab is not closed after the download selection.

for 1- Going back to Nightly55, the testcase result is that the _blank is shown, but still is not closed after the download option. Not sure if it's by design or not, the best mozregression could do is to narrow it to [2017-05-23, 2017-05-24] (1 days)- https://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2017-05-23&enddate=2017-05-24 from which bug 1319111 seems most likely candidate. Now that raises the question if this was an intended fix or not.

Honza, could you please take a look to this scenario? I'd rather have a confirmation on the expected result of this test case before splitting and triaging this issue further.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(honzab.moz)

You either want Honza Odvarko or someone completely else.

Flags: needinfo?(honzab.moz)

Ah! Missed the reference to bug 1319111. Then I should look at this, yes. But as this has slipped through so many releases, I think it's very low priority.

Assignee: nobody → honzab.moz
Blocks: 1319111
Component: Untriaged → DOM: Security
Product: Firefox → Core

Arguably what we're doing is correct: you've clicked on a link that explicitly says "open in new window" (target=_blank). We do that. The fact that the content is a type we can't render, therefore offer the download, is orthogonal to that.

Of course nothing in the spec says we can't close the window, and Chrome's behavior ("close popup if it's empty") is arguably nicer, but that's more of a feature change/enhancement than a "bug".

Alternatives for this page: don't use target=_blank and you'll get the download anyway. If you don't trust the link to remain a type that will cause a download (404 might navigate, for example) then you can use the download attribute on the link.

I don't believe the noreferrer has anything to do with this.

Component: DOM: Security → Document Navigation

Arguably what we're doing is correct

It's not. We have explicit code in the tree to close the window in this case, and it's not working properly anymore, while it used to. I'm investigating why that is, but I suspect based on what I've seen so far that it's something about how things are remoted for e10s.

I don't believe the noreferrer has anything to do with this.

Based on code inspection, I'm 99% sure that the noreferrer case would fail to close properly even if we fixed the non-noreferrer case, fwiw.

OK, so after a bit of digging into this:

  1. Comment 1 seems to be about the change in what url shows up in the url bar when the tab is (incorrectly) not closed. It's entirely possible that Honza's changes had to do with that, yes, since they would affect the "final channel URI". It's not at all clear to me what the right behavior there is. It should be spun off to a new bug, because it's not what this original bug is about. I filed bug 1532752 on this.

  2. In Firefox 54 (so before Honza's changes) the noreferrer case is broken while the "normal" case works, as I expected based on code inspection. That's also the case in Firefox 65, and is what this bug is about. This part is definitely Document Navigation, but is NOT a regression from Honza's patches, obviously. I'll look into this.

  3. In current nightly, the case without noreferrer is also broken. That's because target="_blank" now implies noopener (as of bug 1503681) and it's the noopener bits that are really relevant here (noreferrer also implies noopener). If I use a nonexistent named target and leave off the rel="noreferrer" things work as they should.

Assignee: honzab.moz → bzbarsky
No longer blocks: 1319111

OK, so there are at least two issues with the noopener codepath:

  1. https://searchfox.org/mozilla-central/rev/92d11a33250a8e368c8ca3e962e15ca67117f765/docshell/base/nsDocShell.cpp#8677-8709 does not set the INTERNAL_LOAD_FLAGS_FIRST_LOAD flag on the loadState. It should, maybe.

  2. Even if I fix that, the flag gets lost when we get to the SendCreateWindowInDifferentProcess call at https://searchfox.org/mozilla-central/rev/3e0f1d95fcf8832413457e3bec802113bdd1f8e8/dom/ipc/ContentChild.cpp#878 because that doesn't get passed either the loadstate or any info about its flags.

Kyle, I guess we could pass the loadstate through SendCreateWindowInDifferentProcess instead of deconstructing it into a bunch of separate args, and then use it on the other side? But also, it seems like all the code in ContentParent::CommonCreateWindow (which is where RecvCreateWindowInDifferentProcess lands) should be terminating in loads that set INTERNAL_LOAD_FLAGS_FIRST_LOAD somehow... If true, we should just fix that.

Flags: needinfo?(kyle)

So for example, it seems to me that ideally tabbrowsers's addTab would set INTERNAL_LOAD_FLAGS_FIRST_LOAD on the URI load it does. I got lost in the forest of loadURI calls that stuff does, though... and it's not obvious that it has a way to pass that flag in to start with.

Flags: needinfo?(gijskruitbosch+bugs)

Oh, and the "_blank implies noopener" thing is only on by default starting with 67, so we have a bit of time here to fix the major regression issue.... But we do want to fix it, I think.

Blocks: 1503681
Keywords: regression
Summary: target=_blank and rel=noreferrer opens a new tab and does not close. → target=_blank that results in a download opens a new tab and does not close

it seems to me that ideally tabbrowsers's addTab would set INTERNAL_LOAD_FLAGS_FIRST_LOAD on the URI load it does

And we'd need similar bits for Fennec and GeckoView, and for the "new window" vs "new tab" codepaths, probably....

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

So for example, it seems to me that ideally tabbrowsers's addTab would set INTERNAL_LOAD_FLAGS_FIRST_LOAD on the URI load it does. I got lost in the forest of loadURI calls that stuff does, though... and it's not obvious that it has a way to pass that flag in to start with.

It would [have a way to pass that], assuming there's a public XPCOM way of passing it. We already pass load flags to the relevant nsIWebNavigation. I assume, naively, that "all it would take" would be passing the relevant (non-internal) flag here:

https://searchfox.org/mozilla-central/rev/fbb251448feb7276f9b1d0a88f9c0cb1cd144ce4/browser/base/content/tabbrowser.js#2665-2688

(and slightly less easily for the case where we create the browser lazily, ie we'd need to somehow pass it through the session store entry we create so that session store passes it when it eventually calls loadURI - but that doesn't look like it's necessarily relevant for this bug as filed)

I've never been aware this flag existed, so I have no idea what it does and what might break if we started doing that...

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bzbarsky)

assuming there's a public XPCOM way of passing it

Hmm. I guess there is: nsIWebNavigation::LOAD_FLAGS_FIRST_LOAD.

I assume, naively, that "all it would take" would be passing the relevant (non-internal) flag here

One would think so.... I just tried it, and the flag makes its way to the channel and thence to the helper app service, but the tab-closing bit still doesn't happen, because there is no opener so the code at https://searchfox.org/mozilla-central/rev/b2d35912da5b2acecb0274eb113777d344ffb75e/docshell/base/nsDSURIContentListener.cpp#49-62 doesn't end up closing the window. I guess we need to figure out the "right" behavior here...

so I have no idea what it does

That part is pretty simple. All the flag does is cause the "docshell.newWindowTarget" property on the channel to be set to true. And all that does is that if the channel gets kicked over to the "helper app or download" code it sets mMaybeCloseWindowHelper->SetShouldCloseWindow(true), which lands in that code I linked above. The idea is to close the useless popup...

Now whether we want that behavior for all addTab consumers or not is an interesting question.

Flags: needinfo?(bzbarsky)

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

Now whether we want that behavior for all addTab consumers or not is an interesting question.

I think so.... or at least, I can't think of a case where we wouldn't want that.

I'm assuming from the opener stuff being in nsDSURIContentListener that you don't need more info from me; let me know if there's something else I can help with.

I can't think of a case where we wouldn't want that

We never get addTab called on a window with zero tabs?

I'm not really sure who to talk to about the opener stuff yet. The bugs where that stuff was added are bug 343921 and bug 355222 and none of those people are around anymore.

But what we are going to need to figure out is how to generally do this in a fission world, so maybe the right fix will fall out from that. Maybe. But there's a general UX issue here, if we're using a helper app dialog: where should we parent that dialog to?

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

I can't think of a case where we wouldn't want that

We never get addTab called on a window with zero tabs?

I'm not sure how that relates to this bug? We'd need to close the window, right? If the last tab in a window closes, the window will be closed. Frontend JS should be taking care of that.

The initial tab in a window is not created through addTab, it exists in markup and is attached to a browser by tabbrowser's init method.

Of course, this means that I don't know off-hand if the issue in comment #0 needs more additional work for the case where you right click the link and "open in new window" (or turn off the "open new windows as tabs" pref, to make _blank links open in new windows).

I'm not really sure who to talk to about the opener stuff yet. The bugs where that stuff was added are bug 343921 and bug 355222 and none of those people are around anymore.

:-(

But what we are going to need to figure out is how to generally do this in a fission world, so maybe the right fix will fall out from that. Maybe.

Yeah... would be nice to do something before then, but I know we're all busy.

But there's a general UX issue here, if we're using a helper app dialog: where should we parent that dialog to?

The opener, I guess? This is a known, somewhat-but-not-entirely-orthogonal issue, though (ie potential for spoofing when e.g. background tabs cause it to appear).

We'd need to close the window, right?

Well, that's the issue. We need to parent the new dialog to something, is my understanding... if we close the whole window, where do we parent to? We need to find a different window or something.

It's not clear to me whether we need to parent the helper app dialog to anything nowadays, btw. It used to be modal, I think, so had to be parented to stuff. But I'm pretty sure it's not modal anymore. So maybe we don't need to parent it at all?

if the issue in comment #0 needs more additional work for the case where you right click the link and "open in new window"

Or just where target="_blank" goes into a new window per user preferences. Yes, that needs separate work independent of what we do with addTab. :(

would be nice to do something before then

To be clear, I think we should fix this for 67. Which is bad, because there's not much time left. This needs someone to own it, and that someone can't be me, and should ideally be someone somewhat familiar with the various front-end codepaths involved and the helper app dialog and whatnot.

The opener, I guess?

The whole point is that we don't have access to the opener, if you mean the opener of the new tab that then needs a helper app dialog. It's in a different process.

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

To be clear, I think we should fix this for 67. Which is bad, because there's not much time left. This needs someone to own it, and that someone can't be me, and should ideally be someone somewhat familiar with the various front-end codepaths involved and the helper app dialog and whatnot.

I can see what you did there. :-)

I'm pretty busy, but I can try to help. The problem is I'm not sure what this bug is really about. As far as I can tell, we're talking about a number of different things (which perhaps just all need to happen, I'm just not sure anymore):

  1. make addTab pass this "first load" flag
  2. make the ipc-in-new-process thingymajig just always pass that flag given we know it's going to be the case
  3. allow the code in HelperAppDlg.jsm to not (by the looks of it) throw when there isn't a parent anymore, and/or find some way of telling it about the right opener.

I take it you want my help for (1) and (3). I'm a bit confused, because HelperAppDlg.jsm and the opening of unknownContentType.xul are definitely happening in the parent process, not the child, so I don't understand why it matters that the child is in a different process. SendCreateWindowInDifferentProcess knows which tab/contentchild requested the new window, so presumably we can still keep track of the link in the parent process, no? Heck, in the e10s case, all we really have are browser windows (we can't parent the dialog to a specific tab, at least not at present), so as long as we have an opener browser window we should be OK. I mean, that's speaking in generalities, the problem from my end is that I'm not familiar with the DOM/IPC side of the infrastructure we have for window.open...

Does that last paragraph make any sense? Do actions 1-3 make sense, and/or is there more work to do here, and/or am I wildly mistaken about any of 1/2/3? :-)

Flags: needinfo?(bzbarsky)

I can see what you did there. :-)

Also to be clear, I'm not saying you should be the one to fix this...

Other options, by the way, are to delay bug 1503681 by a release or to ship the regression.

The problem is I'm not sure what this bug is really about.

The user-perceived behavior is "I clicked a link, which opened a useless new tab and that tab didn't autoclose like it used to". Fixing that involves several different things, though.

make addTab pass this "first load" flag

I think this should happen, yes.

make the ipc-in-new-process thingymajig just always pass that flag given we know it's going to be the case

Not needed for the new-tab case. For the new-window case, I think we're looking at ContentParent::CommonCreateWindow with aOpenLocation == nsIBrowserDOMWindow::OPEN_NEWWINDOW, which ends up calling nsIBrowserDOMWindow::OpenURI with the OPEN_NEW flag. I don't know whether that flag should eventually result in the LOAD_FLAGS_FIRST_LOAD flag or not (as in, what the other OPEN_NEW consumers are doing). In any case, I'm not quite sure what the right behavior here is for the new-window case to start with.

allow the code in HelperAppDlg.jsm to not (by the looks of it) throw when there isn't a parent anymore, and/or find some way of telling it about the right opener.

That seems plausible...

I take it you want my help for (1) and (3).

Yes. I mean, ideally I want someone else (Andrea?) to take this off my plate completely... ;)

so I don't understand why it matters that the child is in a different process

It might not. I just don't know how these pieces fit together nowadays.

Flags: needinfo?(bzbarsky) → needinfo?(amarchesini)

Just a crazy idea: what about to wait with opening the new window until we know the response content type (or better said - the response handler)? We would not have to carry the "close me" flag. We could parent the save-as dialog to the current window. UX would worsen, tho - when the response comes with a significant delay user may get confused by the sudden pop-up.

(In reply to Honza Bambas (:mayhemer) from comment #19)

UX would worsen,

Yeah, I don't think this would be an acceptable trade-off. Especially on slow connections, it may then take many seconds for windows/tabs to open when opening links in a new tab/window. Downloads are ultimately the minority of uses of links, so we can't really penalize the majority case for that.

Also to be clear, I'm not saying you should be the one to fix this...

If we want to have an extra cycle to fix this issue, we can postpone bug 1503681. It's not a big deal, I guess.
But I have not time to work a 'real' fix.

Flags: needinfo?(amarchesini)

OK. I feel like bug 1503681 is going to lead to noticeable usability regressions without this fixed. We should try to find someone who has time to work on the real fix...

Assignee: bzbarsky → nobody

Dolske/Wennie, can you help find someone to work on this and resolve this for 67/68 (one way or another)?

Issue: when opening download some links, we leave an empty useless tab around (this bug) and the URL bar value is incorrect (bug 1532752).

Context: as part of bug 1503681 we added an option where <a> and <area> links with target=_blank imply rel=noopener. This is a security feature. It landed on 65 but was preffed off. It was preffed on by default in bug 1522083, which is targeting 67.

One of the side effects of this (noopener/_blank) change is that download links with target=_blank, which are reasonably common, now get treated as having rel=noopener by default. That is, where previously this bug wouldn't happen except if the website explicitly specified rel=noopener or rel=noreferrer, this same bug will now hit more people.

The same (more or less) applies to bug 1532752.

Therefore, one possible mitigation we could apply is holding back the pref flip from 67 so target=_blank links continue to have noopener - but that's a security feature so it deserves at least some sign-off. Even then, we'd need a fix for the 68 cycle still. So we need a plan for both 67/68, to avoid usability regressions (see also comment 22).

There should be enough context in this bug and bug 1532752 to enable someone with the relevant skills to fix both. It shouldn't, in theory, take more than a week, I think, assuming familiarity with frontend and/or docshell. What's missing is someone who has cycles to work on this. I'm happy to help answer questions if things are unclear on the frontend side.

Flags: needinfo?(wleung)
Flags: needinfo?(dolske)

Not sure if security engineering have engineers to work on this by 67/68. We are rather overloaded these days. Let's have Johann and Christoph to take a look at it and get an estimate first.

Flags: needinfo?(wleung)
Flags: needinfo?(jhofmann)
Flags: needinfo?(ckerschb)
See Also: → 1532752

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

Flags: needinfo?(ckerschb)

[Tracking Requested - why for this release]: Usability regression

assuming we go through that code path

For downloads we do not.

Not sure if security engineering have engineers to work on this by 67/68

Then we should back out bug 1522083 until we do, imo. I don't think we should be shipping the usability regression.

Blocks: 1522083
Flags: needinfo?(wleung)

Actually, I guess Neha is the right person to needinfo for the bug 1522083 interaction...

Flags: needinfo?(wleung) → needinfo?(nkochar)

Note that bug 1522083 only landed in 67, so arguably we should track for that, and probably mark 66 as wontfix at this point.

Summary: target=_blank that results in a download opens a new tab and does not close → target=_blank (with either explicit noreferrer/noopener, or on builds that have dom.targetBlankNoOpener.enabled set to true) that results in a download opens a new tab and does not close

Sorry for the delay.

I think we should pref off the default rel=noopener feature (back out bug 1522083) until this bug is fixed. I am completely overloaded (and mostly stuff that I can't just drop, like onboarding new folks), but I know that Erica might be interested in taking this.

Erica, we need someone with some time at their hands to really own this problem and find a solution, ideally for an uplift to 67. Do you think you could do that?

Flags: needinfo?(jhofmann)
Flags: needinfo?(ewright)

we should track for that, and probably mark 66 as wontfix at this point

Er, yes, that is what I meant to do, but got the wrong dropdown.

Fwiw, I really doubt we can come up with a solution here that is upliftable to 67 at this date, but I would be happy to be proven wrong.

I'm interested and have time, so I'll give it a shot.

Flags: needinfo?(ewright)

I'd been busy with some fission work and hadn't had a chance to check this until now. If implementing Comment 7 for passing DocShellLoadState through the SendCreateWindowInDifferentProcess chain would help, feel free to file a new issue dependent on this one and assign it to me.

Flags: needinfo?(kyle)

Thank you, Erica for taking this up. We'll wait till the end of this week to take the decision on whether we should back out bug 1522083 (to pref it off by default) in 67 or not, depending on Erica's estimate for the work involved.

Assignee: nobody → ewright
Flags: needinfo?(nkochar)
Priority: -- → P2

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

Thank you, Erica for taking this up. We'll wait till the end of this week to take the decision on whether we should back out bug 1522083 (to pref it off by default) in 67 or not, depending on Erica's estimate for the work involved.

Neha, did you reach to a conclusion last week?

Flags: needinfo?(nkochar)

(In reply to Pascal Chevrel:pascalc from comment #34)

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

Thank you, Erica for taking this up. We'll wait till the end of this week to take the decision on whether we should back out bug 1522083 (to pref it off by default) in 67 or not, depending on Erica's estimate for the work involved.

Neha, did you reach to a conclusion last week?

Yes, talked to Erica yesterday and it does not look like we have clarity on the possible solution so I think we will need to backout bug 1522083, to pref it off by default and also not track this for 67 and instead work on a solution for 68.

Flags: needinfo?(nkochar)

Just got the update from Erica that Ehsan has sent her some ideas last night (thank you Ehsan), so she'll try those today. Let's give this 1-2 more days, please. :)

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

Just got the update from Erica that Ehsan has sent her some ideas last night (thank you Ehsan), so she'll try those today. Let's give this 1-2 more days, please. :)

Sure :)

FWIW the fix here may not end up being suitable for a backport, depending on what it ends up looking like, so we shouldn't assume that after waiting Erica's fix will necessarily be something that would be safe to backport to beta. I still don't even have 100% confidence that the parts of the fixes Erica and I have looked at would be sufficient for a full fix. So we shouldn't yet rule out a backout-on-beta-as-a-regression-fix-for-67 solution yet. :-)

Flags: needinfo?(dolske)
No longer blocks: 1503681
Regressed by: 1503681
Blocks: 1546415

Bug 1546415 disables the pref (enabled in bug 1522083) by default for 67 so removing the 67 tracking flag in this bug. This can ride the trains with 68.

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:ewright, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(ewright)

Thank you, I've written an appropriate test for the patch and will land it shortly after try completes.

Flags: needinfo?(ewright)

Erica, ping for potentially landing this in 69 - how was the try push?

Flags: needinfo?(ewright)

Hi, so sorry for dropping this for so long. I've been distracted with other work. The try push failed and I've been looking into the failing tests when I can, I hope to be able to get back to focus on this soon.

Flags: needinfo?(ewright)

This is P2 and assigned, so I'm removing it from weekly regression triage by marking it fix-optional.
Happy to take a patch for 70 or in future.

You need to log in before you can comment on or make changes to this bug.