1301056 - [e10s] Link with target="_blank" to download a file leaves a about:blank window/tab open after the download

Status: VERIFIED FIXED

(Firefox :: File Handling, defect, P2)

Description

[Jessica DeFusco]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160823121617

Steps to reproduce:

With 48.0.2 click a link, for example to a PDF file or .JNLP file, that has target="_blank" set and PDF attachments set to open automatically and it will open a new tab (or window depending on the browser.link.open_newwindow option) and then open in the PDF client but that tab/window will remain open. In the past that tab/window would instantly closed.

Setting browser.link.open_newwindow to 1 so it forces use of the current tab is a workaround but may cause unexpected issues with other links.

This isn't consistent behavior however as we have ~250 users (Windows 7 32-bit and 64-bit) and it's not happening to everyone but it only started when they upgraded to 48.0.8.


Actual results:

The tab/window for the file that downloads and opens wasn't closed


Expected results:

The tab/window for the file that downloads and opens should've closed

Comment 1

[Loic]

Attachment: 1301056.html

I can't reproduce it with the testcase I attached.
If I select "Save File" for PDF in Firefox options, the PDF is downloaded and the extra about:blank tab disappears just after the download.

Comment 2

[Loic]

Ok, I understand the issue. The issue appears only with e10s enabled (which is the case for a small group of FF48 users, for testing purpose).

I'm pretty sure all your affected users have e10s enabled, you can check that in the page about:support, there is a line called "Multiprocess Windows" in the " Application Basics" table.

Comment 3

[Loic]

I tested with Nightly 40 and e10s enabled, same issue. It's probably here since e10s has been implemented.

STR:
1) Be sure e10s is on.
2) about:preferences#applications > Portable Document Format (PDF): Safe File
3) Click on the link of the testcase

Comment 4

[Mike Conley (:mconley) (:⚙️)]

The DOMWindowClose event doesn't appear to be being fired here... we don't seem to enter this block: http://searchfox.org/mozilla-central/rev/591354c5e5521cf845bf6b6ef44e8b3b0aeda17d/docshell/base/nsDocShell.cpp#11093

Which means we don't set docshell.newWindowTarget to true on the channel properties, which means we don't set mShouldCloseWindow here: http://searchfox.org/mozilla-central/rev/591354c5e5521cf845bf6b6ef44e8b3b0aeda17d/uriloader/exthandler/nsExternalHelperAppService.cpp#1639

Which means we don't queue this timer which fires the event which removes the tab: http://searchfox.org/mozilla-central/rev/591354c5e5521cf845bf6b6ef44e8b3b0aeda17d/uriloader/exthandler/nsExternalHelperAppService.cpp#2569

That's as far as I've gotten with my initial pokings. We'll triage this on Thursday.

Comment 8

[mail]

Hi, any news on when the devs are planning to fix this? Thanks.

Comment 9

[:Paolo Amadini]

Hey Erin, just want to make sure this is still on the radar of the e10s team.

If this is what I believe, it's pretty bad as we leave users staring at a blank page in the middle of navigation, and with no "back" button enabled, when the site doesn't actually intend to open a popup window. Some of these users may have never used tabs. I haven't tested this though, maybe one of the bug reporters can comment on whether this is actually what they're observing.

Comment 10

[mail]

Hey Paolo, I confirm what you say, Yes, that is what I am observing. Still an issue on recent versions.

Comment 11

[akalinyunus98]

My experience was, that if you would start a download it will open a new blank tab, but in previous versions (48/49) the tab would close after the download had started, whereas in Version 51 the tab will stay openned.

Comment 12

[Erin Lancaster [:elan]]

Thank you for reporting and raising this. I will escalate.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1301056 - Fix auto-closing of windows when they divert to an external app.

It appears that we always meant to call
nsExternalHelperAppService::OnStartRequest even before we diverted (there's a
check for XRE_IsChildProcess in there) but forgot to. With this patch, we call
nsExternalHelperAppService::MaybeCloseWindow in the child and restore the old
behavior.

Review commit: https://reviewboard.mozilla.org/r/118224/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118224/

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8844968 [details]
Bug 1301056 - Fix auto-closing of windows when they divert to an external app.

https://reviewboard.mozilla.org/r/118224/#review120606

D'oh, good find. We should probably consider uplifting this.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/43cc8e497475
Fix auto-closing of windows when they divert to an external app. r=mconley

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/43cc8e497475

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Looks like a trivial fix? Shall we nominate for Beta/ESR52 approval?

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Don't forget bug 1350058. I'd like to get a mochitest in the tree before requesting approval. I'll try as hard as I can to get to that today or tomorrow (leaving the NI on me for that).

Comment 20

[Marcia Knous [:marcia]]

Tracking 54/55 and esr52+ for this e10s regression.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8844968 [details]
Bug 1301056 - Fix auto-closing of windows when they divert to an external app.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: New tabs and windows opened for content types we don't handle won't be automatically closed.
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): Medium risk, given that there was one regression (bug 1350058).
String or UUID changes made by this patch: n/a

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: e10s
[Is this code covered by automated tests?]: Yes, see bug 1359651
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: bug 1350058 bug 1359651

Comment 22

[Gerry Chang [:gchang]]

Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!

Comment 23

[ovidiu boca[:Ovidiu]]

I tested this issue on Windows 10 x64 with FF Nightly 55.0a1(2017-04-27) and I can confirm the fix.

Comment 24

[Gerry Chang [:gchang]]

Comment on attachment 8844968 [details]
Bug 1301056 - Fix auto-closing of windows when they divert to an external app.

Fix a regression related to e10s and was verified. Beta54+. Should be in 54 beta 4.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c9ea5b27b818

Comment 26

[Paul Silaghi, QA [:pauly]]

Verified fixed Fx 54b4, Win 10.

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8844968 [details]
Bug 1301056 - Fix auto-closing of windows when they divert to an external app.

Given that the risk to uplift this is medium and that it led to at least one regression, I am leaning towards not uplifting this to ESR52. Tabs not closing automatically is not ideal but it's not as severe as a data loss/crashy situation.