799234 - HTML5 audio tag can't play data from a Blob URL.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P1)

Description

[msanyi11]

Build ID: Mozilla/5.0 (X11; Linux i686; rv:18.0) Gecko/18.0 Firefox/18.0

Reproducible: Always
Steps to reproduce:
1.  Open a file with an <input> element.
2.  Use a FileReader object to read the file into an ArrayBuffer.
3.  Use the Blob constructor, and create a Blob object with the type "audio/ogg".
4.  Use the window.URL.createObjectURL function and create a Blob URL to the Blob object created in step 3.
5.  Open the URL.

Actual result:
About 1 second playback, then browser hangup (no crash).

Expected result:
Full playback of the given audio file, without hangup.

The given file can be played without problems if opened normally as a local file.

Works on: Mozilla/5.0 (X11; Linux i686; rv:16.0) Gecko/20100101 Firefox/16.0

Comment 1

[Loic]

Could you attach a small testcase, please.

Comment 2

[msanyi11]

Attachment: Test page with automated URL creation

Comment 3

[msanyi11]

Attachment: Example ogg audio file from the wikimedia commons.

Comment 4

[Loic]

m-c
good=2012-09-06
bad=2012-09-07
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0c4fa25f637b&tochange=36427d4b2cf6

Suspected bug:
Paul Adenot — Bug 730765 - Media cache shouldn't be used when loading from blob: urls. r=biesi

Comment 5

[Matthew Gregan [:kinetik]]

It looks like nsOggReader::GetBuffered ends up stuck in an infinite loop inside FileMediaResource::ReadFromCache where mInput->Read is returning NS_OK and zero bytes.  aOffset is 338013, aCount is 5000, bytesRead is 0, and the file size is 343013.

>	xul.dll!FileMediaResource::ReadFromCache(char * aBuffer, __int64 aOffset, unsigned int aCount)  Line 1429	C++
 	xul.dll!nsOggReader::RangeEndTime(__int64 aStartOffset, __int64 aEndOffset, bool aCachedDataOnly)  Line 887 + 0x23 bytes	C++
 	xul.dll!nsOggReader::GetBuffered(nsTimeRanges * aBuffered, __int64 aStartTime)  Line 1707 + 0x1d bytes	C++
 	xul.dll!nsBuiltinDecoderStateMachine::GetBuffered(nsTimeRanges * aBuffered)  Line 2459 + 0x2a bytes	C++
 	xul.dll!nsBuiltinDecoder::GetBuffered(nsTimeRanges * aBuffered)  Line 575 + 0x22 bytes	C++
 	xul.dll!nsHTMLMediaElement::GetBuffered(nsIDOMTimeRanges * * aBuffered)  Line 3592	C++
 	xul.dll!nsHTMLVideoElement::GetBuffered(nsIDOMTimeRanges * * aBuffered)  Line 40 + 0x10 bytes	C++
 	xul.dll!NS_InvokeByIndex_P(nsISupports * that, unsigned int methodIndex, unsigned int paramCount, nsXPTCVariant * params)  Line 71	C++
 	xul.dll!CallMethodHelper::Invoke()  Line 3108 + 0x1c bytes	C++
 	xul.dll!CallMethodHelper::Call()  Line 2442 + 0x8 bytes	C++
 	xul.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx, XPCWrappedNative::CallMode mode)  Line 2408 + 0x16 bytes	C++
 	xul.dll!XPCWrappedNative::GetAttribute(XPCCallContext & ccx)  Line 2855 + 0xe bytes	C++
 	xul.dll!XPC_WN_GetterSetter(JSContext * cx, unsigned int argc, JS::Value * vp)  Line 1517 + 0x9 bytes	C++

Comment 6

[Matthew Gregan [:kinetik]]

Attachment: test single underlying stream with nsIMultiplexInputStream

A debug build also asserts twice with:

###!!! ASSERTION: Bad URI: 'StringBeginsWith(aUri, NS_LITERAL_CSTRING(BLOBURI_SCHEME ":"))', file /home/kinetik/mozilla-central/content/base/src/nsBlobProtocolHandler.cpp, line 74

...which is bug 652114.

ReadFromCache is stuck in a loop because nsMultiplexInputStream::Seek doesn't handle SEEK_SET correctly when the stream has already advanced from zero iff there's only a single stream present in the multiplex, e.g. if the stream is at current offset 5000 and SEEK_SET seeks to new offset 15000 (and the file length is 20000), the seek lands at min(current_offset + new_offset, file length).

Attach patch modifies the existing tests to also run against a multiplexed stream with a single underlying stream, which fails and reveals this bug.

Comment 7

[Matthew Gregan [:kinetik]]

Attachment: patch v0

(In reply to Matthew Gregan [:kinetik] from comment #6)
> nsMultiplexInputStream::Seek
> doesn't handle SEEK_SET correctly when the stream has already advanced from
> zero iff there's only a single stream present in the multiplex

Turns out this is true even with multiple streams (so long as the current offset and the new offset remain within the same internal stream), the existing tests didn't cover this case.  I extended the existing tests to test this case, and another bug I introduced while trying to fix this.  The updated test_seek_multiplex.js fails without these changes, and passes with them.  The testcase in this bug also works correctly with the patch applied.

The SeekableStreamAtBeginning assertion might be overkill; if so let me know and I'll remove it.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=3f51043c6486

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 669866 [details] [diff] [review]
patch v0

The assertions seems valuable to me, please keep it :)

+                    int64_t newPos = streamPos + avail;
+                    if (remaining < newPos) {
+                        // Final seek target is this stream.
+                        newPos = remaining;
+                    }

Hmm, I wonder if it would be better to write this as:

  int64 newPos = NS_MIN(remaining, streamPos + avail);

either way - r=biesi

Comment 9

[Matthew Gregan [:kinetik]]

Attachment: patch v1

Bug 797871 tried to fix the same issue, but test_seek_multiplex fails once I apply my changes to the test, so I'm going to call that fix broken, back it out, and apply my one (which does pass the new test added in bug 797871).

Also, I have no idea why these tests are catching and swallowing all exceptions, but the end result is that any real test failure ends up hidden and the tests always pass.  I'll remove the try/catch in question.

Requesting review from dougt since he reviewed bug 797871 and biesi has already okayed this change.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=37832a557665

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Is this a problem for Gaia?

Comment 11

[Andrea Marchesini [:baku]]

I don't see any test failed applying the patch for bug 797871.
Are you sure that this bug is still valid after applying the patch for bug 797871?

Comment 12

[Matthew Gregan [:kinetik]]

(In reply to Andrea Marchesini (:baku) from comment #11)
> I don't see any test failed applying the patch for bug 797871.
> Are you sure that this bug is still valid after applying the patch for bug
> 797871?

Yes, apply my test changes from patch v0 in this bug. They fail because newPos is computed incorrectly.

Comment 13

[Andrea Marchesini [:baku]]

ok, but if you apply the patch bug 797871 without your patch, is this bug reproducible?

Comment 14

[Matthew Gregan [:kinetik]]

baku and I talked on IRC to clarify this, but to close the loop in the bug:

I didn't realize this problem was being worked on in parallel in another bug until I got review and went to apply my patch and hit patch rejects.  I decided to keep baku's changes from bug 797871 and just include the new tests and assertions from my patch on top.  After I applied those changes, test_multiplex_seek.js failed because it tests a path that was previously untested and isn't fixed by the patch from bug 797871.  When seeking to position 35 (immediately after the new tests I added), the seek of the underlying stream fails because the computed newPos is wrong -- it has computed a newPos of 9, but the streamPos is already 10, so with SEEK_CUR the final position will be 19, which is invalid for a stream of length 10.

The simplest solution (in terms of code changes, and testing) seems to be to revert those changes and apply mine, which pass the tests I added and the tests baku added in bug 797871.

Comment 15

[Doug Turner (:dougt)]

Comment on attachment 671695 [details] [diff] [review]
patch v1

Review of attachment 671695 [details] [diff] [review]:
-----------------------------------------------------------------

bako just was in this code.  He can review.

Comment 16

[Andrea Marchesini [:baku]]

Comment on attachment 671695 [details] [diff] [review]
patch v1

Review of attachment 671695 [details] [diff] [review]:
-----------------------------------------------------------------

looks good. I have already read and try this code and it works.

Comment 17

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad953d3aaebe

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ad953d3aaebe

Comment 19

[Ryan VanderMeulen [:RyanVM]]

This bug is marked blocking-basecamp+ but the patch does not apply cleanly to Aurora. Please post a branch-specific patch for landing.

Comment 20

[Matthew Gregan [:kinetik]]

Attachment: patch v1 for aurora

Ah, patch v1 applies on top of bug 797871 (by reverting part of it), which hasn't landed on aurora.  Here's a patch for aurora.  I created this by qimporting bug 797871's patch, the patch v1 from this bug, and qfolding them.  The end result is the same code change as patch v0 on this bug, plus the test changes from bug 797871 and patch v1.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8c6befc1efaa

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

This patch added main thread IO in debug builds :(

Comment 23

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (X11; Linux i686; rv:18.0) Gecko/20100101 Firefox/18.0

Verified on Ubuntu 12.04 and F18 beta 3.

Comment 24

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/20100101 Firefox/19.0

All good when using the attached testcase in F19 beta.

Comment 25

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old