1421176 - merged Blob from sliced one produce corrupted Blob

Status: RESOLVED FIXED

(Core :: DOM: File, defect, P1)

Description

[Kaiido]

Attachment: merge_blob_slices.html

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.

Comment 1

[Kaiido]

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

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

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?

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this is a recent regression in 58.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: media.patch

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.

Comment 5

[Andrea Marchesini [:baku]]

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

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 1 - testing for empty IPCBlob

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 2 - gtests

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 3 - implementation

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 8933262 [details] [diff] [review]
part 3 - implementation

not green on try yet.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 3 - implementation

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 3 - implementation

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 12

[Olli Pettay [:smaug][[e-mail]]]

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 13

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Cosmin Sabou [:CosminS]]

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 18

[Julien Cristau [:jcristau] (back April 22)]

Is this safe enough for uplift to beta?

Comment 19

[Kaiido]

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 20

[Andrea Marchesini [:baku]]

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

Comment 21

[Gerry Chang [:gchang]]

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

Comment 22

[Andrea Marchesini [:baku]]

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

[Gerry Chang [:gchang]]

Comment on attachment 8933259 [details] [diff] [review]
part 1 - testing for empty IPCBlob

Take this to avoid failed on data consumption. Beta58+.

Comment 24

[Andrea Marchesini [:baku]]

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

Comment 25

[Tiberius Oros[:tiberius_oros]]

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

Comment 26

[Andrea Marchesini [:baku]]

Attachment: part 1 - m-b

All the other patches apply correctly on m-b.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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

Comment 28

[Kaiido]

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.

Comment 29

[Andrea Marchesini [:baku]]

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

Comment 30

[Kaiido]

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

Comment 31

[Andrea Marchesini [:baku]]

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