Can't start two downloads in parallel via <a download> anymore

RESOLVED FIXED in Firefox 67

Status

()

defect
RESOLVED FIXED
2 months ago
5 days ago

People

(Reporter: esnyder, Assigned: baku)

Tracking

(Regression, {regression, site-compat})

66 Branch
mozilla68
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox66- wontfix, firefox67+ fixed, firefox68+ fixed)

Details

Reporter

Description

2 months ago

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/73.0.3683.86 Safari/537.36

Steps to reproduce:

We have a page which performs two downloads using the click() function. Now, when the first click() happens, the page reloads and the rest of the javascript does not execute. A click() should not cause the page to reload or the rest of the code to not execute.

Actual results:

When the js click() happens, then the page reloads and the rest of the javascript after the click() no longer executes. Using click() effectively kills the rest of the javascript code by skipping it after the execution of the js click().

So, now a click() is like reloading the page...

Expected results:

The code after the js click() should have continued to execute after the click() happened. This started between Firefox 64 and Firefox 66. In 64 it worked as designed, now it's broken.

Comment 1

2 months ago

Hey esnyder,

Can you please provide that page so we can check this out?
On which Firefox version is this happening?

Flags: needinfo?(esnyder)
Reporter

Comment 2

2 months ago

It's an internal page, but I can provide an example of the code. It started happening with version 65 of Firefox. The source worked as intended under 64.

function downloadAll() {
  var urls = [
    'File.exe',
    'File2.xml'
  ];
  downloadFiles(urls);
}

function downloadFiles(urls){
  var link = document.createElement('a');
  link.setAttribute('download', null);
  link.style.display = 'none';

  document.body.appendChild(link);

  downloadFile(urls[0], link);
  downloadFile(urls[1], link);

  document.body.removeChild(link);
}

function downloadFile(url, link) {
  var filename = url.substring(url.lastIndexOf('/')+1);
  link.setAttribute('download', filename);
  link.setAttribute('href', url);
  link.click();
}

As soon as the first downloadFile() executes the link.click() the page reloads and the call to downloadFile(urls[1], link); never executes as the page is completely reloaded. I would not expect the click() function to cause the page to reload.

Flags: needinfo?(esnyder)
Reporter

Comment 3

2 months ago

Any update? This is causing us issues with our clients...

Comment 4

2 months ago

Hey ensyder, sorry for the late response!

Moving this over to it's component for the development team to take a look over it.

Component: Untriaged → JavaScript Engine
Product: Firefox → Core

Involves only DOM functionality, therefore moving into "DOM: Core & HTML" for further triaging.

Component: JavaScript Engine → DOM: Core & HTML

smaug, any ideas what may have changed here?

Component: DOM: Core & HTML → Document Navigation
Flags: needinfo?(bugs)
Summary: javascript click() now reloads page after execution → javascript click() on a link that should trigger download now reloads the page
Reporter

Comment 7

2 months ago

Just a clarification. The initial download happens, but since the page reloads after the first click(), no code after the click() executes.

Updated

2 months ago
Flags: needinfo?(bugs)

I just tested this. To reproduce (at least from file://) you need to use filenames that actually exist.

I do see a behavior change at some point where instead of showing download dialogs for both downloads we only show one for the last download. That change happened somewhere in https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e56cc5e7b57a5d18ab72207f7f246a9b8c610c1c&tochange=24f969298ec5c1a7cbc444bfc8f3d77b07f46bbe which seems like it would first show up in Firefox 66; that matches comment 0.

I expect this is a regression from bug 1495363. Before that change, starting the second download would NOT cancel the first one, because we "knew" it was going to be a download; what happened if we then decided to not download (e.g. due to it going cross-site is an interesting question). I expect it triggered the assertion that bug 1495363 removed, but still verifying that.

Anyway, if it did use to trigger that assertion, then the change in bug 1495363 would make us cancel the first download attempt when we start the second download.

Reporter, is this bug report really about the fact that only one of the downloads happens, or do you actually see pages being reloaded? How are you identifying that the page is being reloaded, in the latter case?

Blocks: 1495363
Flags: needinfo?(nika)
Flags: needinfo?(esnyder)
Flags: needinfo?(amarchesini)

I expect it triggered the assertion that bug 1495363 removed, but still verifying that

Confirmed. If I just build locally from the changeset right before bug 1495363 landed and run the testcase from this bug, I get:

[Child 32679, Main Thread] ###!!! ASSERTION: Overwriting an existing document channel!: '(loadFlags & nsIChannel::LOAD_REPLACE) || !(mDocumentRequest.get())', file uriloader/base/nsDocLoader.cpp, line 402
[Child 32679, Main Thread] ###!!! ASSERTION: Overwriting an existing document channel!: '(loadFlags & nsIChannel::LOAD_REPLACE) || !(mDocumentRequest.get())', file uriloader/base/nsDocLoader.cpp, line 402

Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: javascript click() on a link that should trigger download now reloads the page → Can't start two downloads in parallel via <a download> anymore
No longer blocks: 1495363
Regressed by: 1495363

Probably too late for 66 given we're about 2 weeks away from 67.

Reporter

Comment 12

2 months ago

Thanks so much!

Reporter

Comment 13

2 months ago

@boris This is really about the fact that a single download takes place and once that download begins the rest of the code after the javascript click() doesn't execute seemingly due to the page reloading.

sorry for the late needinfo reply

Flags: needinfo?(esnyder)

Too late for 67, if a patch can be uplifted in a dot release we might consider it, so fix-optional for 67.

Reporter

Comment 15

Last month

@Pascal: If it's not going to make it into 67, how long before version 68 is due to be released?

(In reply to esnyder from comment #15)

@Pascal: If it's not going to make it into 67, how long before version 68 is due to be released?

Here is the calendar for our upcoming releases (68 is July 9) https://wiki.mozilla.org/Release_Management/Calendar

One big question here is whether this bug is a bigger problem than bug 1495363 or not. If it is, we should just back out bug 1495363 in 67...

Flags: needinfo?(pascalc)
Duplicate of this bug: 1528457
Duplicate of this bug: 1529846

One big question here is whether this bug is a bigger problem than bug 1495363 or not.

Given that we have had three independent reports of it already (two of which were ignored and marked as duplicate of something that has nothing to do with any of this!), I think it is.

Had a conversation with :bz, and it looks like, while there may be long term solutions, the best short term solution is probably to back out bug 1495363.

Flags: needinfo?(nika)

(In reply to :Nika Layzell (ni? for response) from comment #21)

Had a conversation with :bz, and it looks like, while there may be long term solutions, the best short term solution is probably to back out bug 1495363.

I would propose that we first back out on pre-release channels and then that we consider an uplift into a 67 dot release once we have made sure that the fix is verified in nightly and beta without causing new regressions. We have shipped bug 1495363 in the last two releases already, backing it out a few days before shipping 67.0 feels like unnecessary risk to me.

Flags: needinfo?(pascalc)
Depends on: 1552138
Assignee

Comment 23

Last month

bug 1495363 has been backed out. Is this bug still valid? I'm planning to work again on bug 1495363, but in the meantime, probably we can close this bug.

Flags: needinfo?(amarchesini)

(In reply to Andrea Marchesini [:baku] from comment #23)

bug 1495363 has been backed out. Is this bug still valid? I'm planning to work again on bug 1495363, but in the meantime, probably we can close this bug.

This is still valid until we uplift the backout to a dot release after we have made sure on early 68 betas that we didn't regress something else.

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

(In reply to Andrea Marchesini [:baku] from comment #23)

bug 1495363 has been backed out. Is this bug still valid? I'm planning to work again on bug 1495363, but in the meantime, probably we can close this bug.

This is still valid until we uplift the backout to a dot release after we have made sure on early 68 betas that we didn't regress something else.

That's what tracking flags are for, I think we can close this.

Fixed by the backout in https://hg.mozilla.org/mozilla-central/rev/7aeb3ef01176fc25e7b51d6f32b4093661800084

Status: NEW → RESOLVED
Closed: Last month
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

That's what tracking flags are for

OK, but shouldn't we then track either this bug or bug 1552138 for 67?

Flags: needinfo?(jcristau)
Assignee: nobody → amarchesini
Flags: qe-verify+
Depends on: 1557797

Fixed for 67 by the backout in bug 1557797

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