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

RESOLVED FIXED in mozilla18

Status

()

Core
Audio/Video
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ms2ger, Assigned: cpearce)

Tracking

({crash, intermittent-failure})

Trunk
mozilla18
x86_64
Linux
crash, intermittent-failure
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

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

https://tbpl.mozilla.org/php/getParsedLog.php?id=14906033&tree=Mozilla-Inbound

Updated

6 years ago
Keywords: crash
(Assignee)

Comment 1

6 years ago
Created attachment 657723 [details] [diff] [review]
Patch
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+
(Assignee)

Comment 3

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

6 years ago
Created attachment 657769 [details] [diff] [review]
Patch 2

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

6 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+
https://hg.mozilla.org/mozilla-central/rev/e28fd5ccdb40
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(Assignee)

Comment 12

6 years ago
Created attachment 657936 [details] [diff] [review]
Patch 3
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+
Keywords: intermittent-failure
Whiteboard: [orange]
You need to log in before you can comment on or make changes to this bug.