Closed Bug 799234 Opened 8 years ago Closed 8 years ago

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

Categories

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

18 Branch
x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox17 --- unaffected
firefox18 + verified
firefox19 + verified

People

(Reporter: msanyi11, Assigned: kinetik)

References

Details

(Keywords: hang, regression, testcase)

Attachments

(4 files, 2 obsolete files)

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
Severity: blocker → normal
Could you attach a small testcase, please.
Attachment #669430 - Attachment mime type: text/plain → text/html
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
Keywords: hang
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++
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.
Attached patch patch v0 (obsolete) — Splinter Review
(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
Attachment #669844 - Attachment is obsolete: true
Attachment #669866 - Flags: review?(cbiesinger)
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
Attachment #669866 - Flags: review?(cbiesinger) → review+
Depends on: 797871
Attached patch patch v1Splinter Review
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
Attachment #669866 - Attachment is obsolete: true
Attachment #671695 - Flags: review?(doug.turner)
Is this a problem for Gaia?
blocking-basecamp: --- → ?
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?
(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.
ok, but if you apply the patch bug 797871 without your patch, is this bug reproducible?
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.
blocking-basecamp: ? → +
Priority: -- → P1
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.
Attachment #671695 - Flags: review?(doug.turner) → review?(amarchesini)
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.
Attachment #671695 - Flags: review?(amarchesini) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad953d3aaebe
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla19
https://hg.mozilla.org/mozilla-central/rev/ad953d3aaebe
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
This bug is marked blocking-basecamp+ but the patch does not apply cleanly to Aurora. Please post a branch-specific patch for landing.
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.
This patch added main thread IO in debug builds :(
Mozilla/5.0 (X11; Linux i686; rv:18.0) Gecko/20100101 Firefox/18.0

Verified on Ubuntu 12.04 and F18 beta 3.
Keywords: verifyme
QA Contact: virgil.dicu
Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/20100101 Firefox/19.0

All good when using the attached testcase in F19 beta.
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.