Jessica DeFusco Reporter
	Description • 8 years ago

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

	Loic
	Comment 1 • 8 years ago

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.

	Loic
	Comment 2 • 8 years ago

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.

	Loic
	Comment 3 • 8 years ago

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

	Mike Conley (:mconley) (:⚙️)
	Comment 4 • 8 years ago

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.

	mail
	Comment 8 • 7 years ago

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

	:Paolo Amadini
	Comment 9 • 7 years ago

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.

	mail
	Comment 10 • 7 years ago

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

	akalinyunus98
	Comment 11 • 7 years ago

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.

	Erin Lancaster [:elan]
	Comment 12 • 7 years ago

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

	Mike Conley (:mconley) (:⚙️)
	Comment 14 • 7 years ago
mozreview-review

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.

	Pulsebot
	Comment 15 • 7 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 16 • 7 years ago
bugherder

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

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 7 years ago

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

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 19 • 7 years ago

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).

	Marcia Knous [:marcia]
	Comment 20 • 7 years ago

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

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 21 • 7 years ago

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

	Gerry Chang [:gchang]
	Comment 22 • 7 years ago

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

	ovidiu boca[:Ovidiu]
	Comment 23 • 7 years ago

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

	Gerry Chang [:gchang]
	Comment 24 • 7 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 7 years ago
bugherder uplift

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

	Paul Silaghi, QA [:pauly]
	Comment 26 • 7 years ago

Verified fixed Fx 54b4, Win 10.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 27 • 7 years ago

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.