1183394 - "ASSERTION: Must have state machine to start state machine thread" with duplicate createObjectURL use

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jesse Ruderman]

Attachment: testcase

1. Load the testcase
2. Quit Firefox

###!!! ASSERTION: Must have state machine to start state machine thread: 'mDecoderStateMachine', file dom/media/MediaDecoder.cpp, line 536

> #01: mozilla::MediaDecoder::ChangeState(mozilla::MediaDecoder::PlayState) [dom/media/MediaDecoder.cpp:280]
> #02: mozilla::MediaDecoder::Shutdown() [dom/media/MediaDecoder.cpp:450]
> #03: mozilla::MediaSourceDecoder::Shutdown() [dom/media/mediasource/MediaSourceDecoder.cpp:152]
> #04: mozilla::MediaShutdownManager::Shutdown() [xpcom/glue/nsTHashtable.h:222]
> #05: mozilla::MediaShutdownManager::Observe(nsISupports*, char const*, char16_t const*) [dom/media/MediaShutdownManager.cpp:98]
> #06: nsObserverList::NotifyObservers(nsISupports*, char const*, char16_t const*) [xpcom/glue/nsTArray.h:361]
> #07: nsObserverService::NotifyObservers(nsISupports*, char const*, char16_t const*) [xpcom/glue/nsTHashtable.h:138]
> #08: mozilla::ShutdownXPCOM(nsIServiceManager*) [xpcom/glue/nsCOMPtr.h:389]
> #09: ScopedXPCOMStartup::~ScopedXPCOMStartup() [toolkit/xre/nsAppRunner.cpp:1498]
> #10: XREMain::XRE_main(int, char**, nsXREAppData const*) [memory/mozalloc/mozalloc.h:210]
> #11: XRE_main [toolkit/xre/nsAppRunner.cpp:4463]
> #12: main [browser/app/nsBrowserApp.cpp:212]

Comment 1

[Jesse Ruderman]

Why does this only happen at shutdown? Is something leaking here?

(In most of my shutdown bugs, I can replace the "Quit Firefox" step with "Close the tab and bonk the Memory Pressure button".)

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Hi Benjamin,
Please check this issue.

Comment 3

[Benjamin Chen [:bechen]]

(In reply to JW Wang [:jwwang] from comment #2)
> Hi Benjamin,
> Please check this issue.

Seems a good use case for rr, I'll try reproduce it tomorrow.

Comment 4

[Benjamin Chen [:bechen]]

I have check the bug with rr :)

mDecoderStateMachine is a data member of MediaDecoder and it still keep in the initail value 0 (I guess it is because the MediaSource doesn't send in any data). Then MediaDecoder::Shutdown is called.

Comment 5

[Benjamin Chen [:bechen]]

Attachment: bug-1183394.v01.patch

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

When I change

document.createElement('audio').src = sourceURL;
document.createElement('audio').src = sourceURL;

to

document.createElement('audio').src = sourceURL;

in the test case, it doesn't hit the assertion.

Can you figure out the root cause of the problem before we conclude the patch makes sense?

Comment 7

[Benjamin Chen [:bechen]]

(In reply to JW Wang [:jwwang] from comment #6)
> When I change
> 
> document.createElement('audio').src = sourceURL;
> document.createElement('audio').src = sourceURL;
> 
> to
> 
> document.createElement('audio').src = sourceURL;
> 
> in the test case, it doesn't hit the assertion.
> 
> Can you figure out the root cause of the problem before we conclude the
> patch makes sense?

|MediaSource::Attach| returns different value. The first one returns TRUE, but the second one returns FALSE due to the |mReadyState != MediaSourceReadyState::Closed|.

Comment 8

[Benjamin Chen [:bechen]]

The 2 MediaElement holds the same MediaSource, so their mMediaSource member point to the same object.

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

It appears to me that MediaDecoder::Shutdown should do nothing if Load() is not called at all. We should replace more |if (mDecoderStateMachine)| with |MOZ_ASSERT(mDecoderStateMachine)| when we can identify which functions should be called after Load() and which could be called before Load(). Those assertions will help us catch bugs when things go unexpected.

Comment 10

[Benjamin Chen [:bechen]]

(In reply to JW Wang [:jwwang] from comment #9)
> It appears to me that MediaDecoder::Shutdown should do nothing if Load() is
> not called at all. We should replace more |if (mDecoderStateMachine)| with
> |MOZ_ASSERT(mDecoderStateMachine)| when we can identify which functions
> should be called after Load() and which could be called before Load(). Those
> assertions will help us catch bugs when things go unexpected.

How about destroyed the MediaDecoder directly in this testcase ? (The MediaShutdownManager hold a reference to MediaDecoder)
if (!mMediaSource->Attach(decoder)) {
  decoder->ShutDown();
  return NS_ERROR_FAILURE;
}

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

I wonder if that would work.

Shutdown -> ChangeState -> ScheduleStateMachine -> assertion!

Comment 12

[Benjamin Chen [:bechen]]

(In reply to JW Wang [:jwwang] from comment #11)
> I wonder if that would work.
> 
> Shutdown -> ChangeState -> ScheduleStateMachine -> assertion!

I know the code path. I mean in this testcase, should we destroy the MediaDecoder earlier instead of leave it until the browser shutdown?

Comment 13

[JW Wang [:jwwang] [:jw_wang]]

Early cleanup is good in my opinion.

Comment 14

[Benjamin Chen [:bechen]]

Attachment: bug-1183394.v02.patch

Early clean up the MediaDecoder in HTMLMediaElement.cpp.
Add assertion in MediaDecoder::ScheduleStateMachine.

Bug 1185931 will address comment 9.

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=07d5e3e8a72d

Comment 15

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8636943 [details] [diff] [review]
bug-1183394.v02.patch

Review of attachment 8636943 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. I wonder if we need to call ScheduleStateMachine() in MediaDecoder::ChangeState for changes in the play state is already handled by the mirror of MDSM. Please file a new bug to investigate it.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a75d02e97bc

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5a75d02e97bc