1289004 - Remove MediaDecoder::mShuttingDown

Status: RESOLVED FIXED

Description

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

It is confusing to have both mShuttingDown and PLAY_STATE_SHUTDOWN. We should remove mShuttingDown and use |mPlayState == PLAY_STATE_SHUTDOWN| instead.

Comment 1

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

Attachment: Bug 1289004. Part 1 - Constify and devirtualize some functions. .

Review commit: https://reviewboard.mozilla.org/r/66756/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66756/

Comment 2

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

Attachment: Bug 1289004. Part 2 - Add MediaDecoder::IsShutdown().

Review commit: https://reviewboard.mozilla.org/r/66758/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66758/

Comment 3

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

Attachment: Bug 1289004. Part 3 - Remove MediaDecoder::mShuttingDown.

Review commit: https://reviewboard.mozilla.org/r/66760/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66760/

Comment 4

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8774214 [details]
Bug 1289004. Part 1 - Constify and devirtualize some functions. .

https://reviewboard.mozilla.org/r/66756/#review63536

Comment 5

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8774215 [details]
Bug 1289004. Part 2 - Add MediaDecoder::IsShutdown().

https://reviewboard.mozilla.org/r/66758/#review63538

Comment 6

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8774216 [details]
Bug 1289004. Part 3 - Remove MediaDecoder::mShuttingDown.

https://reviewboard.mozilla.org/r/66760/#review63540

Comment 7

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

https://hg.mozilla.org/try/rev/f1671931bae24f1345a43eac38d4a5b82e9abde2#l1.105
This assertion fails in the mochitest.

https://treeherder.mozilla.org/logviewer.html#?job_id=24443345&repo=try#L32512
stack:
XUL!mozilla::MediaDecoder::ChangeState(mozilla::MediaDecoder::PlayState) [MediaDecoder.cpp:4bf5f73310db : 1355 + 0x0]
XUL!mozilla::dom::HTMLMediaElement::Pause(mozilla::ErrorResult&) [HTMLMediaElement.cpp:4bf5f73310db : 1935 + 0x9]
XUL!<name omitted> [HTMLMediaElement.cpp:4bf5f73310db : 1955 + 0x8]
XUL!mozilla::dom::HTMLMediaElement::UnbindFromTree(bool, bool) [HTMLMediaElement.cpp:4bf5f73310db : 3144 + 0xc]
XUL!mozilla::dom::Element::UnbindFromTree(bool, bool) [Element.cpp:4bf5f73310db : 1861 + 0x6]
XUL!nsGenericHTMLElement::UnbindFromTree(bool, bool) [nsGenericHTMLElement.cpp:4bf5f73310db : 583 + 0x10]
XUL!mozilla::dom::Element::UnbindFromTree(bool, bool) [Element.cpp:4bf5f73310db : 1861 + 0x6]
XUL!nsGenericHTMLElement::UnbindFromTree(bool, bool) [nsGenericHTMLElement.cpp:4bf5f73310db : 583 + 0x10]
XUL!mozilla::dom::Element::UnbindFromTree(bool, bool) [Element.cpp:4bf5f73310db : 1861 + 0x6]
XUL!nsGenericHTMLElement::UnbindFromTree(bool, bool) [nsGenericHTMLElement.cpp:4bf5f73310db : 583 + 0x10]
XUL!mozilla::dom::HTMLSharedElement::UnbindFromTree(bool, bool) [HTMLSharedElement.cpp:4bf5f73310db : 312 + 0xf]
XUL!nsDocument::cycleCollection::Unlink(void*) [nsDocument.cpp:4bf5f73310db : 1955 + 0x6]
XUL!nsHTMLDocument::cycleCollection::Unlink(void*) [nsHTMLDocument.cpp:4bf5f73310db : 189 + 0xb]
XUL!nsCycleCollector::CollectWhite() [nsCycleCollector.cpp:4bf5f73310db : 3343 + 0x3]
XUL!nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) [nsCycleCollector.cpp:4bf5f73310db : 3689 + 0x8]
XUL!nsCycleCollector::ShutdownCollect() [nsCycleCollector.cpp:4bf5f73310db : 3607 + 0x15]
XUL!nsCycleCollector_shutdown() [nsCycleCollector.cpp:4bf5f73310db : 4217 + 0x5]

This is because HTMLMediaElement::Pause() calls MediaDecoder::Pause() which calls ChangeState() after MediaShutdownManager::BlockShutdown() happens.

I will work around this problem by early return from MediaDecoder::Pause() when IsShutdown() is true.

However, we should fix this problem since HTMLMediaElement should not call into MediaDecoder functions once Shutdown() begins. Checking IsShutdown() everywhere makes the code fragile and logic unclear.

Comment 8

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

Comment on attachment 8774214 [details]
Bug 1289004. Part 1 - Constify and devirtualize some functions. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66756/diff/1-2/

Comment 9

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

Comment on attachment 8774215 [details]
Bug 1289004. Part 2 - Add MediaDecoder::IsShutdown().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66758/diff/1-2/

Comment 10

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

Comment on attachment 8774216 [details]
Bug 1289004. Part 3 - Remove MediaDecoder::mShuttingDown.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66760/diff/1-2/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/48c409dc9e93
Part 1 - Constify and devirtualize some functions. r=cpearce.
https://hg.mozilla.org/integration/autoland/rev/412d21e8fddc
Part 2 - Add MediaDecoder::IsShutdown(). r=cpearce
https://hg.mozilla.org/integration/autoland/rev/6bb275091417
Part 3 - Remove MediaDecoder::mShuttingDown. r=cpearce

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/48c409dc9e93
https://hg.mozilla.org/mozilla-central/rev/412d21e8fddc
https://hg.mozilla.org/mozilla-central/rev/6bb275091417