Closed
Bug 787805
Opened 12 years ago
Closed 12 years ago
Intermittent 752784-1.html | crash @ FileMediaResource::EnsureLengthInitialized
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: Ms2ger, Assigned: cpearce)
References
Details
(Keywords: crash, intermittent-failure)
Attachments
(2 files, 1 obsolete file)
828 bytes,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
6.34 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
Because FileMediaResource::EnsureLengthInitialized() dereferences mInput without null-checking it: https://tbpl.mozilla.org/php/getParsedLog.php?id=14906033&tree=Mozilla-Inbound
Assignee | ||
Comment 1•12 years ago
|
||
Comment on attachment 657723 [details] [diff] [review] Patch Review of attachment 657723 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/MediaResource.cpp @@ +1030,5 @@ > > void FileMediaResource::EnsureLengthInitialized() > { > mLock.AssertCurrentThreadOwns(); > + if (mSizeInitialized || !mInput) { But this means EnsureLengthInitialized does not, in fact, ensure the length is initialized. Seems to me the caller shouldn't be getting to call EnsureLengthInitialized if mInput is null.
Attachment #657723 -
Flags: review?(roc) → review+
Assignee | ||
Comment 3•12 years ago
|
||
Usually we null check mInput and bail out from the methods that need to call EnsureLengthInitialized(), but we don't in at least two places: FileMediaResource::GetCachedRanges() and FileMediaResource::GetLength(), so if we remove the mInput null check from EnsureLengthInitialized() it needs to be duplicated in at least those two places, which leaks the abstraction EnsureLengthInitialized creates.
Assignee | ||
Comment 4•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e28fd5ccdb40
Oops, I think I meant to r- that. Sorry. (In reply to Chris Pearce (:cpearce) from comment #3) > Usually we null check mInput and bail out from the methods that need to call > EnsureLengthInitialized(), but we don't in at least two places: > FileMediaResource::GetCachedRanges() and FileMediaResource::GetLength(), so > if we remove the mInput null check from EnsureLengthInitialized() it needs > to be duplicated in at least those two places, which leaks the abstraction > EnsureLengthInitialized creates. I don't understand what you mean by "leaks the abstraction". mInput being non-null should be a precondition for calling EnsureLengthInitialized. So either GetCachedRanges should also have a precondition of mInput being non-null, or it should return no ranges when mInput is null. Similar for GetLength.
Assignee | ||
Comment 6•12 years ago
|
||
Let's try that again, but enforcing that mInput is non null in the callers. I think we should be taking the lock before reading mSize in GetCachedDataEnd and GetNextCachedData too.
Attachment #657769 -
Flags: review?(roc)
Comment on attachment 657769 [details] [diff] [review] Patch 2 Review of attachment 657769 [details] [diff] [review]: ----------------------------------------------------------------- Why are we calling these methods when mInput is null, anyway? ::: content/media/MediaResource.cpp @@ +959,5 @@ > } > virtual int64_t GetLength() { > MutexAutoLock lock(mLock); > + if (!mInput) { > + return -1; Wouldn't zero be safer here? @@ +976,5 @@ > } > + virtual int64_t GetCachedDataEnd(int64_t aOffset) { > + MutexAutoLock lock(mLock); > + if (!mInput) { > + return -1; Wouldn't aOffset be a safer return value here?
Assignee | ||
Comment 8•12 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7) > Comment on attachment 657769 [details] [diff] [review] > Patch 2 > > Review of attachment 657769 [details] [diff] [review]: > ----------------------------------------------------------------- > > Why are we calling these methods when mInput is null, anyway? Because mInput is annulled when the decoder is being shutdown, and that can happen while we're in the middle of seeking or calculating buffered ranges (where we call these methods).
(In reply to Chris Pearce (:cpearce) from comment #8) > Because mInput is annulled when the decoder is being shutdown, and that can > happen while we're in the middle of seeking or calculating buffered ranges > (where we call these methods). Aha! Please document that where mInput is declared.
Comment on attachment 657769 [details] [diff] [review] Patch 2 Review of attachment 657769 [details] [diff] [review]: ----------------------------------------------------------------- r+ with that. ::: content/media/MediaResource.cpp @@ +959,5 @@ > } > virtual int64_t GetLength() { > MutexAutoLock lock(mLock); > + if (!mInput) { > + return -1; Actually I think it would be best to return mSize if we've got it, otherwise return 0.
Attachment #657769 -
Flags: review?(roc) → review+
Comment 11•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e28fd5ccdb40
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Assignee | ||
Comment 12•12 years ago
|
||
Attachment #657769 -
Attachment is obsolete: true
Attachment #657936 -
Flags: review?(roc)
Comment on attachment 657936 [details] [diff] [review] Patch 3 Review of attachment 657936 [details] [diff] [review]: ----------------------------------------------------------------- r+ with that fixed. ::: content/media/MediaResource.cpp @@ +967,5 @@ > virtual int64_t GetNextCachedData(int64_t aOffset) > { > + MutexAutoLock lock(mLock); > + if (!mInput) { > + return aOffset; Should return -1. It's not correct to say that aOffset is the start of a cached data block.
Attachment #657936 -
Flags: review?(roc) → review+
Assignee | ||
Comment 14•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/482edf1460dc
Updated•12 years ago
|
Keywords: intermittent-failure
Updated•12 years ago
|
Whiteboard: [orange]
You need to log in
before you can comment on or make changes to this bug.
Description
•