Remove MediaDecoder::mShuttingDown

RESOLVED FIXED in Firefox 50

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(3 attachments)

It is confusing to have both mShuttingDown and PLAY_STATE_SHUTDOWN. We should remove mShuttingDown and use |mPlayState == PLAY_STATE_SHUTDOWN| instead.
Assignee: nobody → jwwang
Priority: -- → P3
Comment on attachment 8774214 [details]
Bug 1289004. Part 1 - Constify and devirtualize some functions. .

https://reviewboard.mozilla.org/r/66756/#review63536
Attachment #8774214 - Flags: review?(cpearce) → review+
Comment on attachment 8774215 [details]
Bug 1289004. Part 2 - Add MediaDecoder::IsShutdown().

https://reviewboard.mozilla.org/r/66758/#review63538
Attachment #8774215 - Flags: review?(cpearce) → review+
Comment on attachment 8774216 [details]
Bug 1289004. Part 3 - Remove MediaDecoder::mShuttingDown.

https://reviewboard.mozilla.org/r/66760/#review63540
Attachment #8774216 - Flags: review?(cpearce) → review+
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 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 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 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/
Pushed by jwwang@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.