Crash in TrackBuffersManager::RemoveFrames

NEW
Unassigned

Status

()

defect
P2
critical
2 years ago
8 months ago

People

(Reporter: jya, Unassigned)

Tracking

(Blocks 1 bug, {crash, stalled})

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

(Reporter)

Description

2 years ago
[@ mozilla::TrackBuffersManager::RemoveFrames ]rare crash, with no obvious explanation. 



https://crash-stats.mozilla.com/report/index/5a6ead0f-8aeb-4bf7-b7b7-764dd0171002
(Reporter)

Updated

2 years ago
See Also: → 1247189
I think it might also be related with the detached source buffer.

If the detached happen before resolving the promise [1], then we would get that situation.

Maybe we should check whether we have been detached or not every time when the promise is resolved.

[1] http://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/dom/media/mediasource/TrackBuffersManager.cpp#1281
Flags: needinfo?(jyavenard)
(Reporter)

Comment 2

2 years ago
(In reply to Alastor Wu [:alwu][please needinfo me][GMT+8] from comment #1)
> I think it might also be related with the detached source buffer.
>
> If the detached happen before resolving the promise [1], then we would get
> that situation.
> 
> Maybe we should check whether we have been detached or not every time when
> the promise is resolved.
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 1a4a26905f923458679a59a4be1e455ebc53c333/dom/media/mediasource/
> TrackBuffersManager.cpp#1281

No it won't, because the Detach task won't run until the appendBuffer task has fully completed.
A new task can't start while a current one is running.
http://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/dom/media/mediasource/TrackBuffersManager.cpp#179

mCurrentTask is only cleared once the appendTask completes and succeed.
http://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/dom/media/mediasource/TrackBuffersManager.cpp#796
Or is rejected.
http://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/dom/media/mediasource/TrackBuffersManager.cpp#808

Despite, the bug is about RemoveFrames, nothing to do with demuxing content.

RemoveFrames can only be called before inserting new frames or when processing a RangeRemoval task.

The code you linked to has nothing to do with either of those operations anyway.

When you call Detach, it queues a DetachTask, that task won't run until all other tasks have run and is always the last task to run. You even added assertions to make sure of that.

http://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/dom/media/mediasource/TrackBuffersManager.cpp#151
Flags: needinfo?(jyavenard)
I'm thinking about one situation (although I'm not sure it's possible or not).

First, notice that all crashes happened on old version, so the race between main thread and task queue is possible.

Assume the CC has happened, TBM got detached and then we might have a race, same reason as 1402681.

Then, we call TBM::ProcessTasks() on the same time and on the different threads. This would make us bypass the checking [1] and run the different tasks at the same time.

If one task is append task and other is reset task (which might be called from [2]), then we would clear |mQueuedSamples| during the appending process, it would cause the UAF.

Do you think is it possible? 

[1] http://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/dom/media/mediasource/TrackBuffersManager.cpp#179
[2] http://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/dom/media/mediasource/SourceBuffer.cpp#217
Flags: needinfo?(jyavenard)
(Reporter)

Comment 4

2 years ago
(In reply to Alastor Wu [:alwu][please needinfo me][GMT+8] from comment #3)
> I'm thinking about one situation (although I'm not sure it's possible or
> not).
> 
> First, notice that all crashes happened on old version, so the race between
> main thread and task queue is possible.

check the description of this commit.

I could have sworn that the crash report I put in the description was *after* bug 1402681 !


> 
> Assume the CC has happened, TBM got detached and then we might have a race,
> same reason as 1402681.
> [snip]
> Do you think is it possible? 

it certainly is possible, but only before bug 1402681.

(I was actually thinking of reports https://crash-stats.mozilla.com/report/index/e4795edc-c34d-40df-ba02-a7d8a0171005)
Flags: needinfo?(jyavenard)
(Reporter)

Comment 5

2 years ago
(In reply to Alastor Wu [:alwu][please needinfo me][GMT+8] from comment #3)
> 
> Then, we call TBM::ProcessTasks() on the same time and on the different
> threads. This would make us bypass the checking [1] and run the different
> tasks at the same time.

actually, on second thoughts...

no that's not possible.

When TrackBuffersManager::RemoveFrames() is running, it means that we either have a AppendBuffer or a RangeRemoval task running.

While those two tasks are running, the Detach task hasn't run.
http://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/dom/media/mediasource/TrackBuffersManager.cpp#179

bug 1247189 can occur due to the race that would have occurred *after* the Detach task being run (and having a second Detach task running again, this time on the main thread)

While the Detach task is pending, you can't have the race, as only the Detach task clear mTaskQueue

As such, this bug is different than bug 1247189
Assignee: nobody → alwu
Assignee: alastor0325 → nobody

Updated

8 months ago
Severity: normal → critical
Keywords: crash
(Reporter)

Comment 6

8 months ago
Don't see how that crash could happen normally, that is with proper ram, no bit flip of any kind etc.

such a rare incidence (a couple every month) i tend to blame cosmic rays
Keywords: stalled
You need to log in before you can comment on or make changes to this bug.