When merging multiple slices from an *original Blob*, the returned Blob is sometimes corrupted. If I take a File from an input[type=file], then slice it in multiple chunks with Blob.slice(start, end) method, before merging all these chunks in a single Blob using new Blob(chunks_array, {type: originalFile.type}) and try to load this merged Blob into a mediaElement using a blobURI, 3/4 of times, the mediaElement will fail to load the blobURI. I couldn't find any pattern as to why it fails: it may both fail and succeed with the same file and same chunk-size on two different tries. Also, converting this merged Blob to an ArrayBuffer gives the exact same result as an ArrayBuffer created from the originalFile. Moreover, even creating a new Blob from this ArrayBuffer only works sporadically... Note that I do not face this issue of 57 stable, only on 58 beta and 59 nightly.

Regression range: First step (Nightlies): Last good revision: 33b7b8e81b4befcba503c0e48cd5370aeb715085 (2017-09-25) First bad revision: 7d15bc419c6cd7e9f3b4d41370c3b0e5990c8d1b (2017-09-26) Second step (taskcluster - inbounds) Last good revision: 641bfddb87113b45ee3df9849345dbcf821ef3da First bad revision: e6b3498a39b94616ba36798fe0b71a3090b1b14c Final step (mozilla-inbound) Last good revision: e8d213d3265f8d6cb0401b2c2b127b4b5908baf5 First bad revision: db42206dd4498e112ffad99103b8ceaf0af3a0be

If I'm reading that regression range in comment 1 correctly, this was caused by bug 1382783. Are blob URIs not same on the image IO thread for some reason?

Tracking since this is a recent regression in 58.

The bug here is in nsMultiplexInputStream::Available. This method returns the number of available bytes for any substream. This is wrong when there are nsIAsyncInputStream because their ::Available() can return 0, and AsyncWait() must be called in order to have more data to process. If we return more, processing the following stream, the caller expects to have that amount of data available to be read. Another bug we currently have in nsMultiplexInputStream is that, if the IPCBlobInputStream is empty and it's a part of a nsMultiplexInputStream, ::Available() return CLOSED also when the following streams have data. A fix for this is: in nsMultiplexInputStream::Available(), if a substream returns CLOSED, we should considered CLOSED and move to the following one. This patch does: 0. nsMultiplexInputStream skips CLOSED stream in ::Available. 1. We have a mochitest for point 1. 2. ::Available() should not proceed following streams when the current one is async. 3. gtest for point 2. 4. Just because we do often do_QueryInterface() for nsIASyncInputStream and nsISeekableStream, I cache these values in a StreamData struct.

If you prefer, I'm happy to split the 5 points in multiple patches.

Comment on attachment 8933262 [details] [diff] [review] part 3 - implementation not green on try yet.

There was a broken test related to how TCPSocket uses nsIMultiplexInputStream: it append streams when reading. In this case, if we already in CLOSED status, we have to go back to NS_OK.

Comment on attachment 8933260 [details] [diff] [review] part 2 - gtests ok, this is just single threaded, so the unblock + available check is a bit weird, but I guess that is fine.

Comment on attachment 8933375 [details] [diff] [review] part 3 - implementation Please ensure TCPSocket gets fixed. The current setup where state may change after stream being already closed is weird.

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3ef41e230c5e nsMultiplexInputStream::Available() sanitize - mochitest - r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/83964057e1b2 nsMultiplexInputStream::Available() sanitize - gtests - r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/e5dd080198e6 nsMultiplexInputStream::Available() sanitize - r=smaug

Backed out for build bustages on xpcom/tests/gtest/TestMultiplexInputStream.cpp:266:3 https://treeherder.mozilla.org/logviewer.html#?job_id=148870586&repo=mozilla-inbound&lineNumber=13630 https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e5dd080198e6577eebdc0916745243d65f981db7&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable https://hg.mozilla.org/integration/mozilla-inbound/rev/64ec7529d9cfc5b4e8a35725e34045bf75b25872

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8654da5491ed nsMultiplexInputStream::Available() sanitize - mochitest - r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/a40cc53547cf nsMultiplexInputStream::Available() sanitize - gtests - r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/795993249baf nsMultiplexInputStream::Available() sanitize - r=smaug

https://hg.mozilla.org/mozilla-central/rev/8654da5491ed https://hg.mozilla.org/mozilla-central/rev/a40cc53547cf https://hg.mozilla.org/mozilla-central/rev/795993249baf

Is this safe enough for uplift to beta?

Has this been applied on Nightly branch? I still face an issue on 59.0a1 (2017-12-04) (64-bit) on osX. Now big files do appear for less than a second and then disappear along with an onerror event firing on the MediaElement. Smaller files raise the error faster.

Comment on attachment 8933259 [details] [diff] [review] part 1 - testing for empty IPCBlob Approval Request Comment [Feature/Bug causing the regression]: nsMultiplexInputStream [User impact if declined]: if a page ends up using nsMultiplexInputStream with IPCBlobInputStream, the consuming of data can fail. For instance, FileReader, or Media player. [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: low [Why is the change risky/not risky?]: This patch reimplements nsMultiplexInputStream::Available. The implementation is tested via gtest and mochitests and fully green on try. I think the risk level is low because I think I have covered all the current uses in our tree with tests. [String changes made/needed]: none

Hi :baku, Per comment #9, any thoughts?

(In reply to Gerry Chang [:gchang] from comment #21) > Hi :baku, > Per comment #9, any thoughts? This was when I canceled a review request. I submitted a new version of the patch and that has been reviewed and landed.

Comment on attachment 8933259 [details] [diff] [review] part 1 - testing for empty IPCBlob Take this to avoid failed on data consumption. Beta58+.

Please, push to beta all the 3 patches and not just the first one. Thanks!

Merge conflict when trying to uplift to beta. Please provide an updated patch.

All the other patches apply correctly on m-b.

https://hg.mozilla.org/releases/mozilla-beta/rev/019a43b61a5b https://hg.mozilla.org/releases/mozilla-beta/rev/0509c7874801 https://hg.mozilla.org/releases/mozilla-beta/rev/52b742260c84

Should I open a new bug report about the MediaElement error bug? I now see that the mochitest is only done with a FileReader, while commment #1 already stated that FileReader did produce correct output even for merged Blobs. And indeed, this mochitest does pass every time even on my machine. Nevertheless, the original issue is still affecting my Nightly: MediaElements fail to load resulting Blobs when served as blobURIs. Attachment 8932353 [details] still reproduces the issue.

> Should I open a new bug report about the MediaElement error bug? > I now see that the mochitest is only done with a FileReader, while commment > #1 already stated that FileReader did produce correct output even for merged > Blobs. your test works fine for me. Seeking on the middle of the stream is not allowed if the buffer has not been populated yet, but for the rest I can see the video correctly. Do you see exactly the same issue as before?

(In reply to Andrea Marchesini [:baku] from comment #29) > > Should I open a new bug report about the MediaElement error bug? > > I now see that the mochitest is only done with a FileReader, while commment > > #1 already stated that FileReader did produce correct output even for merged > > Blobs. > > your test works fine for me. Seeking on the middle of the stream is not > allowed if the buffer has not been populated yet, but for the rest I can see > the video correctly. > Do you see exactly the same issue as before? Not exactly the same no. Now, video elements will try to load the media, but will eventually fail to do so. And big images will display for less than a second before disappearing and raising an error event.

I filed a new bug to continue the work. bug 1424183.