Closed Bug 1373109 Opened 4 years ago Closed 4 years ago

[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

Categories

(Firefox :: File Handling, defect, P2)

54 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 56
Tracking Status
firefox-esr52 --- unaffected
firefox54 + verified
firefox55 + fixed
firefox56 + fixed

People

(Reporter: kurt.garrett, Assigned: mrbkap)

References

Details

(4 keywords)

Attachments

(2 files)

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")
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.
Flags: needinfo?(kurt.garrett)
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.
Uploaded a test case there : https://allanjdr.github.io/
Duplicate of this bug: 1373637
[Tracking Requested - why for this release]: Annoying UX regression in 54.
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);
            }
        }
    }
Flags: needinfo?(kurt.garrett)
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.
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
Blocks: 1301056
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: window.open closes source page automatically after → [e10s] window.open closes source page automatically after
Flags: needinfo?(mrbkap)
Priority: -- → P2
Duplicate of this bug: 1374723
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).
Flags: needinfo?(mrbkap)
Assignee: nobody → mrbkap
That was fast! When this lands let's verify the fix. Do you think this is worth the risk to uplift to beta 55?
Flags: qe-verify+
(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.
Summary: [e10s] window.open closes source page automatically after → [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
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?
Attachment #8879760 - Flags: review?(mconley) → review+
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!
Attachment #8880487 - Flags: review?(mconley) → review+
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?
Pushed by mrbkap@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/d14dde1a1332
https://hg.mozilla.org/mozilla-central/rev/135ec7faeede
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
[Tracking Requested - why for this release]: Given 2 duplicate reports, we should probably uplift the patch to the upcoming 54.0.1 release.
Duplicate of this bug: 1376196
Duplicate of this bug: 1376257
More dups coming in. The patch needs an uplift request.
Flags: needinfo?(mrbkap)
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 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]:
Flags: needinfo?(mrbkap)
Attachment #8879760 - Flags: approval-mozilla-beta?
Attachment #8880487 - Flags: approval-mozilla-beta?
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.
Attachment #8879760 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Flags: needinfo?(mrbkap)
Flags: needinfo?(mrbkap)
Attachment #8879760 - Flags: approval-mozilla-release?
Attachment #8880487 - Flags: approval-mozilla-release?
Comment on attachment 8880487 [details]
Bug 1373109 - Add a mochitest for the auto-closing behavior.

already landed test
Attachment #8880487 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Track 54+ as new regression.
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.
Attachment #8879760 - Flags: approval-mozilla-release? → approval-mozilla-release+
Attachment #8880487 - Flags: approval-mozilla-release? → approval-mozilla-release+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
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....
Duplicate of this bug: 1377123
You need to log in before you can comment on or make changes to this bug.