1373109 - [e10s] downloads in a new window or tab launched from a tab that itself was opened by a previous page automatically close the wrong tab

Status: VERIFIED FIXED

(Firefox :: File Handling, defect, P2)

Description

[kurt.garrett]

Intranet site with cross origin to open pdf from internal file share server. Worked up to version 53.x. Does not work in 54.0. The file downloads, but the tab with the link automatically closes.

Also, Acrobat Reader DC used as viewer.

window.open("../PDF_aspx/Scan.aspx?PATH=\\\\server001\\public\\folder\\somefile.pdf")

Comment 1

[:Paolo Amadini]

Can you provide more information so that the bug can be replicated?

Since the issue is with an Intranet site, it may be difficult for us to identify a solution. If this is not possible, at least a screen recording comparing Firefox 53 and 54 might help us understand the issue better.

Comment 2

[fgt.ciril]

Had a similar bug :

Page A open new Window with : window.open("B.html", "_blank");
Page B open new Window that end up with a file download : window.open("download.exe/doc.rtf", "_blank");

Firefox 53 : a new tab/window open, then is closed and a dialog to open or save file open.
Firefox 54 : a new tab/window open, then page B is closed and the new blank tab still open and no open or save file dialog.

Bug happen only if e10s is enable.
Content-type in download.exe reponse is "application/rtf"
Same problem happen with pdf with an external viewer, not if pdf is display in browser.

Comment 3

[fgt.ciril]

Uploaded a test case there : https://allanjdr.github.io/

Comment 5

[Kohei Yoshino [:kohei]]

[Tracking Requested - why for this release]: Annoying UX regression in 54.

Comment 6

[kurt.garrett]

As fgt.ciril noted, the parent closes when external pdf viewer is set as the application default (in my case PDF application is set to open in Acrobat Reader DC).

The server-side code behind for the aspx in my case is:

public partial class Scan : System.Web.UI.Page
    {
        protected void Page_Load(object sender, EventArgs e)
        {
            if (!Page.IsPostBack)
            {                
                ShowScan();
            }
           
        }
        private void ShowScan()
        {
            //opens and returns pdf to caller
            string filePath = Request.QueryString["PATH"];
            
            WebClient User = new WebClient();
            Byte[] FileBuffer = User.DownloadData(filePath);
            if (FileBuffer != null)
            {
                Response.ContentType = "application/pdf";
                Response.AddHeader("content-length", FileBuffer.Length.ToString());
                Response.BinaryWrite(FileBuffer);
            }
        }
    }

Comment 7

[Julien Cristau [:jcristau] (back April 22)]

I'm seeing the behaviour change between 52.2esr and current beta55/nightly56.  Tracking, though we'll need a regression window, if someone's up to running mozregression for this.

Comment 8

[Alice0775 White]

STR
1. Open https://allanjdr.github.io/
2. Click "Go to page B" link
3. Click "Download src" link
4. When helper dialog pops up, click OK  or cancel

Actual Results:
Tab bar is [Index][New Tab]

Expected Results:
Tab bar should be [Index][Page B]


Regression window(from [Index][Page B][New Tab] to [Index][New Tab]):
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=4746dc671cc275162fa3ffb425c9ed1f835a0e46&tochange=43cc8e497475488a494924132d9f610517081f62


Regressed by 43cc8e497475	Blake Kaplan — Bug 1301056 - Fix auto-closing of windows when they divert to an external app. r=mconley

Comment 10

[Kohei Yoshino [:kohei]]

Posted the site compatibility note for reference: https://www.fxsitecompat.com/en-CA/docs/2017/page-is-automatically-closed-when-window-open-triggers-file-download/

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Thanks for finding this. I have a patch to fix this now and I'll write a test so this doesn't happen again tomorrow.

This will only happen when window A opens window B (either via target or window.open) and window B has a link to a downloadable file that opens in a new window (window C).

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1373109 - Don't call MaybeCloseWindow twice when diverting.

nsExternalHelperAppService::OnStartRequest calls MaybeCloseWindow. I don't
know why DidDivertRequest also did, but it shouldn't have! Most of the time,
we bail out of MaybeCloseWindow because most windows don't have an opener when
we get there. In this case, of course we do and we close the opener instead.

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

Comment 13

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

That was fast! When this lands let's verify the fix. Do you think this is worth the risk to uplift to beta 55?

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1373109 - Add a mochitest for the auto-closing behavior.

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

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #13)
> That was fast! When this lands let's verify the fix. Do you think this is
> worth the risk to uplift to beta 55?

I think the patch to fix this bug is extremely low-risk and definitely worth uplifting to beta 55.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8880487 [details]
Bug 1373109 - Add a mochitest for the auto-closing behavior.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151822/diff/1-2/

Comment 17

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

Comment on attachment 8879760 [details]
Bug 1373109 - Don't call MaybeCloseWindow twice when diverting.

https://reviewboard.mozilla.org/r/151118/#review156862

::: commit-message-464b2:6
(Diff revision 1)
> +Bug 1373109 - Don't call MaybeCloseWindow twice when diverting. r=mconley
> +
> +nsExternalHelperAppService::OnStartRequest calls MaybeCloseWindow. I don't
> +know why DidDivertRequest also did, but it shouldn't have! Most of the time,
> +we bail out of MaybeCloseWindow because most windows don't have an opener when
> +we get there. In this case, of course we do and we close the opener instead.

Can you clarify what case you're referring to here?

Comment 18

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

Comment on attachment 8880487 [details]
Bug 1373109 - Add a mochitest for the auto-closing behavior.

https://reviewboard.mozilla.org/r/151822/#review156866

Slick test, thanks!

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8880487 [details]
Bug 1373109 - Add a mochitest for the auto-closing behavior.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151822/diff/2-3/

Comment 20

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

Comment on attachment 8880487 [details]
Bug 1373109 - Add a mochitest for the auto-closing behavior.

https://reviewboard.mozilla.org/r/151822/#review156878

Ah, that's some great de-duplication, thanks!

::: uriloader/exthandler/tests/mochitest/browser_auto_close_window.js:60
(Diff revisions 2 - 3)
>      is(windowContext.gBrowser.selectedBrowser.currentURI.spec, URL,
>         "got the right windowContext");
>    });
>  });
>  
> -add_task(async function target_blank() {
> +async function testNewTab(browser, url) {

Can you please add some documentation to this helper function describing what it does?

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8879760 [details]
Bug 1373109 - Don't call MaybeCloseWindow twice when diverting.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151118/diff/1-2/

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8880487 [details]
Bug 1373109 - Add a mochitest for the auto-closing behavior.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151822/diff/3-4/

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d14dde1a1332
Don't call MaybeCloseWindow twice when diverting. r=mconley
https://hg.mozilla.org/integration/autoland/rev/135ec7faeede
Add a mochitest for the auto-closing behavior. r=mconley

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d14dde1a1332
https://hg.mozilla.org/mozilla-central/rev/135ec7faeede

Comment 25

[Kohei Yoshino [:kohei]]

[Tracking Requested - why for this release]: Given 2 duplicate reports, we should probably uplift the patch to the upcoming 54.0.1 release.

Comment 28

[Kohei Yoshino [:kohei]]

More dups coming in. The patch needs an uplift request.

Comment 29

[Klaus Plate]

Let me add a comment to the importance: This behaviour is quite common for online banking. Your are on page A and go to page B (post box) where you see your documents. Then you try to download one after the other. In the moment, the current TAB is closed and you loose your banking session. You need a new login for the next action e.g. next download. Really cumbersome. In so far at least for me, the priority is high. Regards, Klaus

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8879760 [details]
Bug 1373109 - Don't call MaybeCloseWindow twice when diverting.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1301056
[User impact if declined]: We could close the wrong tab if a download is triggered in a new tab.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes (in the automated test).
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: No.
[Why is the change risky/not risky?]: The bug is easily explained by reading the code and the fix is obvious.
[String changes made/needed]:

Comment 31

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

Comment on attachment 8879760 [details]
Bug 1373109 - Don't call MaybeCloseWindow twice when diverting.

Let's bring this fix to beta (for either beta 5 or 6).
We can then verify it on beta before uplifting to release.

Comment 32

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

Can you request uplift to m-r also? That way Gerry will see it when considering what we may need in a 54.0.1 release.  Thanks.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/34751b9bda2b
https://hg.mozilla.org/releases/mozilla-beta/rev/9e6af0875ed6

Comment 34

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8880487 [details]
Bug 1373109 - Add a mochitest for the auto-closing behavior.

already landed test

Comment 35

[Gerry Chang [:gchang]]

Track 54+ as new regression.

Comment 36

[Gerry Chang [:gchang]]

Comment on attachment 8879760 [details]
Bug 1373109 - Don't call MaybeCloseWindow twice when diverting.

Fix a regression issue. Release54+. Should be in 54.0.1.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/aa921d444aae
https://hg.mozilla.org/releases/mozilla-release/rev/d5cdee570edd

Comment 38

[Mihai Boldan, Desktop QA [:mboldan]]

I managed to reproduce this issue on Firefox 54.0 under Mac OS X 10.12.5.
The issue is no longer reproducible on Firefox 54.0.1.
Tests were performed under Mac OS X 10.12.5, Windows 10x64, Ubuntu 16.04x64.

Comment 39

[Jan Morgils]

Great job. Solved also 'our' problem as described in 1376404
Very pleased it was upliftet as a patch to version 54.0. Thanks. Open Source rules....