1575799 - Download from a synology NAS is failing

Status: RESOLVED INCOMPLETE

(Core :: DOM: Core & HTML, defect, P2)

Description

[Daniel]

Attachment: Firefox.PNG

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

i'm triyng to download multiple files from Synology NAS File Station.
This is happening for all Firefox version above 66.0.5, also for developer versions 69 and 70 as tested now 22 of august 2019.

Actual results:

When i try to download multiple files from My Synology NAS File Station i get this error:

could not be saved, because the source file could not be read.
Try again later, or contact the server administrator.

Tried on several windows computer, diffrent Firefox installs, and diffrent Synology disk station's.

Expected results:

Download multiple files as one .zip

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Daniel, why do you think it isn't a problem with the Synology software?

Comment 2

[Daniel]

Hello Sylvestre Ledru, I incline to think it’s a Firefox problem mainly because on every other browser it’s working as it should, no errors, just downloading files, on top of that with previous Firefox builds (66.0.5 and before)it works like a charm, so maybe I’m missing something? Also I tested this on several different computers, and also on 6 different Synology NAS File Station with the same results.

Comment 3

[Florencia Di Ciocco, NI to vbalducci]

Hi Daniel,

Does this issue occur with a fresh profile? you can find the steps here: https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles?redirectlocale=en-US&redirectslug=Managing-profiles#w_starting-the-profile-manager

Can you please download Firefox Nightly from here: https://nightly.mozilla.org/ and retest the problem and see if the issue still occurs there as well?

If after doing this you can still reproduce the bug, could you send me a screenshot or a video of the bug? That always helps a lot.

Thanks in advance, Flor.

Comment 4

[Florencia Di Ciocco, NI to vbalducci]

Hi Daniel,

I'm marking this as Resolved-Incomplete due to lack of response. If the issue is still reproducible with the latest Firefox version, feel free to reopen the bug with more information.

Best regards, Flor.

Comment 5

[Daniel]

Hello @Florencia,

Sorry for the late reply, (i was on holidays), I tried on Firefox nightly, same error, it doesn't matter if i use a fresh profile or not the error is the same, this can be reproduced at any moment, like i said before, i tried this on several machines, some with a old profile, some with new profile/fresh install (windows + browser). This is not an intermitent bug, it's a permanent one that can be reproduced at any moment!

Comment 6

[Daniel]

Attachment: This is from Firefox Nightly

Comment 7

[Florencia Di Ciocco, NI to vbalducci]

Hi Daniel,

I've chosen a component for this bug so that the developers may look at it. We'll await their answer. If you consider that there's another component that's more proper for this case you may change it.

Regards, Flor.

Comment 8

[Junior [inactive]]

Hello Daniel,
Since it works for previous Firefox builds (66.0.5 and before).
It would be very helpful if you could find the exact regression range.
https://mozilla.github.io/mozregression/quickstart.html

Could you help us?
Thanks!

Comment 9

[Junior [inactive]]

I did the mozregression

2019-10-01T15:40:45: DEBUG : Starting merge handling...
2019-10-01T15:40:45: DEBUG : Using url: https://hg.mozilla.org/integration/autoland/json-pushes?changeset=a01586b62cf510bb165057e0bea9a45cc76e961e&full=1
2019-10-01T15:40:45: DEBUG : Found commit message:
Bug 1489308 part 10.  Remove some document.open handling in outer window that's no longer needed.  r=mccr8

Differential Revision: https://phabricator.services.mozilla.com/D18077

Comment 10

[Daniel]

(In reply to Junior [:junior] from comment #9)

I did the mozregression

2019-10-01T15:40:45: DEBUG : Starting merge handling...
2019-10-01T15:40:45: DEBUG : Using url: https://hg.mozilla.org/integration/autoland/json-pushes?changeset=a01586b62cf510bb165057e0bea9a45cc76e961e&full=1
2019-10-01T15:40:45: DEBUG : Found commit message:
Bug 1489308 part 10.  Remove some document.open handling in outer window that's no longer needed.  r=mccr8

Differential Revision: https://phabricator.services.mozilla.com/D18077

Hey Junior,

I also did the Mozregression-with the same results:

2019-10-02T14:09:05: DEBUG : Starting merge handling...
2019-10-02T14:09:05: DEBUG : Using url: https://hg.mozilla.org/integration/autoland/json-pushes?changeset=a01586b62cf510bb165057e0bea9a45cc76e961e&full=1
2019-10-02T14:09:06: DEBUG : Found commit message:
Bug 1489308 part 10. Remove some document.open handling in outer window that's no longer needed. r=mccr8
Differential Revision: https://phabricator.services.mozilla.com/D18077
2019-10-02T14:09:06: DEBUG : Did not find a branch, checking all integration branches
2019-10-02T14:09:06: INFO : The bisection is done.
2019-10-02T14:09:07: INFO : Stopped

So...does that help finding the problem/bug?

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Hi Boris, mozregression is pointing at bug 1489308 as the regressor for this issue. Can you please take a look?

Comment 13

[Boris Zbarsky [:bzbarsky]]

I would really like to see the scripts involved. It sounds like the page is calling document.open() and that's canceling the downloads involved, but the details of why it's not happening in other browsers and didn't use to happen in Firefox (since document.open used to cancel things before my changes too, does it in all browsers, and is required to do it per spec) really depend on the exact script.

If someone has a setup that shows that bug that I can access, that would obviously be ideal, along with specific steps to reproduce. If not, at least copies of the scripts and HTML involved would be very helpful.

Comment 14

[Junior [inactive]]

I've pinged my friend who works for Synology. This will be fixed in their next release.

Here's the reason I understood:
iframe.contentDocument.open/write/close three calls in one shot.
An unexpected load event triggers their error handling process.

Maybe Bug 1489308 forced the load event even if the close is called or maybe changed some event timing .
Does it make sense?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Thank you for checking on that! There were some changes around the load event there, and there are some differences around the details from Chrome, and the spec is useless. See https://github.com/whatwg/html/issues/4291 and https://github.com/whatwg/html/issues/4292

I would still really like to know what their exact script looks like so it can feed back into our behavior and maybe the spec issues. Are they basically doing open/write/close and adding a load event listener after that, and in Chrome the load event has already fired (under close()) while we fire it async? Or something else?

Comment 16

[Junior [inactive]]

Here's the code snippet:

var iframe = document.createElement('iframe');
document.body.appendChild(iframe);
var fn = () => {
	iframe.remove();
	console.log('error');
};
var doc = iframe.contentDocument;
doc.open();
doc.close();
iframe.addEventListener('load', fn, true);
iframe.src = "https://www.seattlesymphony.org/~/media/files/notes/schubert-sonata-21-b-flat.pdf";

iframe.src can be replaced with any Content-Disposition: attachment

Comment 17

[Junior [inactive]]

IMO comment 16 is informative enough for :bz. Feel free to give more information.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Thank you. What's going on there is exactly my guess from comment 15. If you make the testcase look like so:

   var iframe = document.createElement('iframe');
   document.body.appendChild(iframe);
   iframe.addEventListener('load', () => {
     console.log('load fired');
   });

   var doc = iframe.contentDocument;
   doc.open();
   console.log("About to call close");
   doc.close();
   console.log("Called close");

you see that in Firefox the load event fires after the close() call returns (and hence after they add the load listener in the snippet in comment 16), but in Chrome it fires before close() returns.

That said, the behavior of my testcase in this comment is the same before the bug 1489308 changes as well: the load event fires after doc.close() returns. So the difference is that if a navigation is started after the document.close() call but before the load event fires, then the load event never happens at all before bug 1489308. That's a consequence of the document.open bits looking like an (unfinished) navigation when the src set happens before bug 1489308, and the src set canceling the navigation and hence causing it to never fire a load event...

Per spec, our behavior before bug 1489308 is definitely wrong. Or course per spec Chrome's behavior is also wrong: see https://bugs.chromium.org/p/chromium/issues/detail?id=878491

Maybe we could add a special case for firing the load event immediately during close if there was an open with no write after it, and get the spec changed accordingly somehow....

What I don't understand is why the page is doing the open/close dance here at all. Just taking all that out and making the script be like so:

var iframe = document.createElement('iframe');
document.body.appendChild(iframe);
var fn = () => {
	iframe.remove();
	console.log('error');
};
iframe.addEventListener('load', fn, true);
iframe.src = "https://www.seattlesymphony.org/~/media/files/notes/schubert-sonata-21-b-flat.pdf";

seems to work in browsers, though it also works only due to browser bugs. In particular, it only works in Chrome because it fires the load event sync under the append instead of async like https://html.spec.whatwg.org/multipage/iframe-embed-object.html#process-the-iframe-attributes says to do, and it only works in Firefox because the navigation starting cancels the (non-spec; see bug 1447406) about:blank load that would have fired the load event!

The spec-compliant way to do this is:

var iframe = document.createElement('iframe');
iframe.addEventListener("load", () => {
    var fn = () => {
      iframe.remove();
      console.log('error');
    };
    iframe.addEventListener('load', fn, true);
    iframe.src = "https://www.seattlesymphony.org/~/media/files/notes/schubert-sonata-21-b-flat.pdf";
  }, { once: true });
document.body.appendChild(iframe);

which is rather annoyingly complicated... :(

Comment 19

[Boris Zbarsky [:bzbarsky]]

Fwiw, I filed https://github.com/whatwg/html/issues/4965 on some of the spec issues here.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Junior, do you know when Synology's next release is? Do they generally get it rolled out to all their installs?

That is, do we need to do something on our end to either get this fixed for people sooner or to make sure all our users who also use Synology get the fix?

Comment 21

[Junior [inactive]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #20)

Junior, do you know when Synology's next release is? Do they generally get it rolled out to all their installs?

That is, do we need to do something on our end to either get this fixed for people sooner or to make sure all our users who also use Synology get the fix?

It's hard to say. Last release is Apr.
Next release, which is a dot release, should be Oct or Nov.
The default update policy is automatically update. I use the default as well.
They do have business users. I guess they could use ESR 60.x but it's going to end of life.
Given low risk, they should feel safe to upgrade.

What I don't understand is why the page is doing the open/close dance here at all.

In fact, they use multiple open/close pairs. The first one is to clear the doc, and the second is to write something from a clear plate.
The fix is simple, using dom operation directly instead of open/close trick.

The code snippet below shows the intension.
Hope it helps. Thanks for taking care of it and giving me a DOM lecture :) Really enjoy.

var iframe;

function prepareIframe()
{
	iframe = document.createElement('iframe');
	document.body.appendChild(iframe);
};

function clickButtonEvent() {
	var doc = iframe.contentDocument;

	var cleanDocContent = function() {
		doc.open();
		doc.close();
	};	
	var writeDocContect = function() {
		//Pass sensitive and too long parameters in HTTP body
		var html = '<html>' +
					'<head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head>' +
					'<body>' +
						'<form accept-charset="utf-8" name="form" action="https://www.seattlesymphony.org/~/media/files/notes/schubert-sonata-21-b-flat.pdf" method="POST">' +
							'<input type="hidden" name="password" value="**" />' +
							'<input type="hidden" name="path" value="/volume1/1,/volume1/2" />' +
						'</form>' +
				'</body></html>';
		doc.open('text/html');
		doc.write(html);
		doc.close();
	};

	cleanDocContent();
	writeDocContect();
	var count = 1;
	iframe.addEventListener('load', function() {
                // remove iframe triggers download error
		//iframe.remove();
		console.log('Load event times:', count);
		++count;
	}, { once: false });

	doc.form.submit();
};

prepareIframe();
setTimeout(clickButtonEvent, 2000);

Comment 22

[Boris Zbarsky [:bzbarsky]]

Next release, which is a dot release, should be Oct or Nov.

OK. We'd be unlikely to ship anything here before Firefox 71 ships in December, so sounds like they will get to this first.

The first one is to clear the doc, and the second is to write something from a clear plate.

Ah, I see. And they're relying on the fact that none of the content they write involved anything loading, because if they did their code would break in every browser (since the load event would have to fire async from close() if there were subresources)... Alright. That makes me worry a little less about someone else also being affected by this.

In particular, the "close after open with no write should fire load sync" approach would not help in their case, since they are writing out content...

Comment 23

[Boris Zbarsky [:bzbarsky]]

Oh, and even if they switch to DOM insertions, if we aligned with the spec by fixing bug 1447406 their code would still break. The spec on that may change, though; see https://github.com/whatwg/html/issues/4965

Comment 24

[Anne (:annevk)]

bz, per our conversation on IRC I got the impression that even if we managed to sort the specification issues, this wouldn't get fixed anytime soon due to the complexity of implementing the necessary changes. Does that basically mean we need to reach out to Synology somehow or are there other alternatives?

Comment 25

[Boris Zbarsky [:bzbarsky]]

It sounds like Synology is working on a fix on their end already. See comment 14.

Comment 26

[Pascal Chevrel:pascalc (PTO until April 26)]

Marking as wontfix for 71 as there is a patch coming on Synology's side.

Comment 27

[Daniel]

Hello, the november Synology "DSM 6.2.2-24922 Update 4" release is here, unfortunately this doesn't solve the download issue!

Comment 28

[Mike Taylor [:miketaylr]]

Thanks for the update Daniel. Junior, could you reach out to your Synology contacts and ask if the fix was intended to be in DSM 6.2.2-24922 Update 4? thanks.

Comment 29

[Junior [inactive]]

The update in comment 27 is for the NAS firmware.
The filestation update is deferred to January 2020, most likely.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Anne, can you follow up with Junior after he gets back to see whether the Synology people fixed this? I can't needinfo him right now...

This is the bug that led to me filing https://github.com/whatwg/html/issues/4965, fwiw.

Comment 32

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Junior, any ideas for comment 31? Thank you.

Comment 33

[Junior [inactive]]

AFAICT they fixed this already but the fix is not released yet.

Comment 34

[Junior [inactive]]

It's fixed in synology side with Version: 1.2.6-0281 (2020-04-28)

• Fixed the issue where downloading files on Firefox 67 might fail.
  https://www.synology.com/en-global/releaseNote/FileStation

Would you like to give a try to see if it's fixed at you side, Daniel?

Comment 35

[Daniel Holbert [:dholbert]]

Closing as incomplete given that we're waiting on confirmation per comment 34, though it seems likely this was fixed on Synology's end as noted there.