Closed Bug 787805 Opened 7 years ago Closed 7 years ago

Intermittent 752784-1.html | crash @ FileMediaResource::EnsureLengthInitialized

Categories

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

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: Ms2ger, Assigned: cpearce)

References

Details

(Keywords: crash, intermittent-failure)

Attachments

(2 files, 1 obsolete file)

Because FileMediaResource::EnsureLengthInitialized() dereferences mInput without null-checking it:

https://tbpl.mozilla.org/php/getParsedLog.php?id=14906033&tree=Mozilla-Inbound
Keywords: crash
Attached patch PatchSplinter Review
Assignee: nobody → cpearce
Status: NEW → ASSIGNED
Attachment #657723 - Flags: review?(roc)
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+
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.
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.
Attached patch Patch 2 (obsolete) — Splinter Review
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?
(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+
https://hg.mozilla.org/mozilla-central/rev/e28fd5ccdb40
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Attached patch Patch 3Splinter Review
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+
Whiteboard: [orange]
You need to log in before you can comment on or make changes to this bug.