jimmywarting / StreamSaver.js

StreamSaver writes stream to the filesystem directly asynchronous
https://jimmywarting.github.io/StreamSaver.js/example.html
MIT License
4.04k stars 418 forks source link

Transferable Streams + HTTP on Chrome = infinite loading #94

Closed TexKiller closed 5 years ago

TexKiller commented 5 years ago

For some strange reason, since the last changes made on the compatibility overhaul, if Chrome has Transferable Streams available (on version 73 with the flag) and we use StreamSaver.js on HTTP pages, the popup with the download link stays loading forever, and the download never starts.

This doesn't happen on HTTPS, nor does it happen on Web Extensions. And if we disable Transferable Streams or use an older version of Chrome that doesn't support it, the download is started correctly.

jimmywarting commented 5 years ago

tested and confirmed

better turn off transform stream on chrome then when running http 😞

tried keeping the popup open all the time, as long as it was open it worked just fine maybe chrome has a bug where it dose not know where it original came from?

the transform stream is passed from main thread to the popup and then transfered to sw.js guess chrome will GC the stream when the owner close the window. What i think is happening is when a readableStream is transfered it will flag the readableStream's producer as the current window. And when the producer closes the window it will close the stream. So when the popup forwards the readableStream to sw.js it will override the producer as the popup - guessing it should not override that flag as it still belongs to the main thread


Have a theory that is semi optional. that involves keeping transferable stream so that the popup can be closed

using messageChannel to transfer the readableStream

...so instead of transferring the readableStream from main thread -> popup -> sw.js we transfer a MessageChannel from main thread -> popup -> sw.js once main thread and sw.js has establish a connection we can transfer the readableStream from main thread -> messageChannel -> sw.js

jimmywarting commented 5 years ago

Wow, it actually worked, lol a fix is coming up

TexKiller commented 5 years ago

Nice! :D

jimmywarting commented 5 years ago

Wondering if we should send out a bug report to chromium 😜

TexKiller commented 5 years ago

I don't know exactly what the expected behaviour is there. Is it supposed to be GC'ed when the Service Worker is stopped?

jimmywarting commented 5 years ago

i don't think it's the service worker fault. And I'm not sure exactly what the expected behaviour should be b/c i don't know the in or outs of the chromium code and the spec.

But when you create a TransformStream you get back a WritableStream and a ReadableStream that are very similar to a MessageChannel and a port1 & port2 where the port1 is the Writable channel and the port2 is the Readable channel - and i happen to know a bit about them as i have experimented with it before - it has also been more tested

And with MessageChannel you should be able to forward the port2 to another worker and be able to close any man in the middle (proxy) without the port2 getting destroyed, cuz the producer is still from the the original thread it was created from.

jimmywarting commented 5 years ago

the expected behavior is that a transfered readable stream should work as MessageChannel dose. Especially if it's a TransformStream, but it's also possible to get a readableStream without creating a producer (similar to port1) if you do (await fetch(url)).body so maybe they didn't thought of my senario and it has some different mechanism

either way it feels like a browser bug to me

jimmywarting commented 5 years ago

Kind of impress to have find a solution that works just after 10 minutes of a unknown issue by guessing how chrome operates

jimmywarting commented 5 years ago

think it works now :) hade to make a small change in sw.js to detect if stream is going to be transferred from MessageChannel

TexKiller commented 5 years ago

Good job fixing the problem!

I can see another problem now, though. The way it is now, StreamSaver.js will never know when to remove the <iframe> with the download link, because you closed the communication port and are no longer sending the "Download started" message.

By the way: the <iframe> onload event never triggers on downloads, and if you remove the <iframe> before the download starts, it never starts.

jimmywarting commented 5 years ago

When I debugged I also tested to comment out the close() method and gave it plenty of time to transfer the stream and all port messages that had to be sent where sent. Then I was downloading the torrent that takes a while to finish... The download happened and then I closed the pop-up manually. Then the stream would just stop (then i was not even touching the "intercept me" iframe)

TexKiller commented 5 years ago

Well, maybe that had something to do with the pinging. It shouldn't matter if the popup is closed or not, since the communication with the Service Worker is done directly through the message port.

From what you are saying, maybe Chrome's bug of keeping the Service Worker alive forever stops working if the popup is closed by the user instead of by JavaScript.

Anyway, in sw.js you explicitly don't store the port on the metadata if Transferable Streams are supported, so the "Download started" message can never be sent. And you also remove the onmessage event on StreamSaver.js.

TexKiller commented 5 years ago

Or did you mean you closed the popup with the download link? I didn't know that affected the download.

I can't right now, but I'll do some more tests later.

jimmywarting commented 5 years ago

@MattiasBuelens explained the problem and what's going on very well in https://github.com/whatwg/streams/issues/276#issuecomment-482749288, he was kind to even write a minimal example to reproduce the problem also

If it helps, don't think streams as being transferable (think of it more like a bunch of pipes put together) when the popup closes then you are breaking the pipe in the middle.

TexKiller commented 5 years ago

Oh, I see. That makes sense. :)

So the issue is resolved already? I still think we should send the message saying that the "Download started", so we can remove the <iframe> with the download link.

jimmywarting commented 5 years ago

So the issue is resolved already?

Yep, i did it in 28fbafe

still think we should send the message saying that the "Download started", so we can remove the <iframe> with the download link.

We can do that, still wondering about #95 doe. and how we should go about closing them... Don't you think it's just easier to remove the download iframe after a second or so instead of listening for a postMessage? or closing the iframe on onload event

TexKiller commented 5 years ago

The onload event never triggers for downloads, and if we have a random timeout, there is a chance we remove it before the download begins.

Do you want to remove that message so we don't need to keep a message port open between the Service Worker and the page? I was thinking of keeping a permanent port in the future to create new streams without the need to open mitm.html again (in the case of popups, at least).

jimmywarting commented 5 years ago

The onload event never triggers for downloads

Good to know. didn't know that

and if we have a random timeout, there is a chance we remove it before the download begins.

We could safely set it to something like 10s
The "download started" happens almost immediately (in the "nextTick") when you open up the download link

I was thinking of keeping a permanent port in the future to create new streams without the need to open mitm.html again (in the case of popups, at least).

I have been thinking about that as well before, but might be worried that the service worker would be forced to reload or go idle and sometime close the MessagePort. i don't want to keep the popup open for what is longer then necessary. people will just see a blank screen - wonder what da heck it's and just close it.

TexKiller commented 5 years ago

Well, so far the library has no way (as far as I can tell) of letting go of the Service Worker and allowing it to die after the download ends, so it shouldn't matter if we keep a permanent message port. We could do something to stop the pinging, or remove the iframe that is pinging, after the stream is closed. But I don't think that is implemented, right?

About the popup, that makes sense, but I think it should still be better than opening two popups, especially if the download is started quickly.