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

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[esnyder]

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

[Timea Cernea [:tbabos][inactive]]

Hey esnyder,

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

Comment 2

[esnyder]

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.

Comment 3

[esnyder]

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

Comment 4

[Timea Cernea [:tbabos][inactive]]

Hey ensyder, sorry for the late response!

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

Comment 5

[André Bargull [:anba]]

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

Comment 6

[Henri Sivonen (:hsivonen)]

smaug, any ideas what may have changed here?

Comment 7

[esnyder]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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?

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

[Julien Cristau [:jcristau]]

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

Comment 12

[esnyder]

Thanks so much!

Comment 13

[esnyder]

@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

Comment 14

[Pascal Chevrel:pascalc]

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

Comment 15

[esnyder]

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

Comment 16

[Pascal Chevrel:pascalc]

(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

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

Comment 20

[Boris Zbarsky [:bzbarsky]]

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.

Comment 21

[Nika Layzell [:nika] (ni? for response)]

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.

Comment 22

[Pascal Chevrel:pascalc]

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

Comment 23

[Andrea Marchesini [:baku]]

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.

Comment 24

[Pascal Chevrel:pascalc]

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

Comment 25

[Julien Cristau [:jcristau]]

(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

Comment 26

[Boris Zbarsky [:bzbarsky]]

That's what tracking flags are for

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

Comment 27

[Pascal Chevrel:pascalc]

Fixed for 67 by the backout in bug 1557797

Comment 28

[Maria Berlinger [:mberlinger], Services QA]

Hi Andrea,
Could you please help me with a test page in order to confirm the fix or could you verify it on latest beta and nightly?