Closed Bug 1289004 Opened 4 years ago Closed 4 years ago

Remove MediaDecoder::mShuttingDown

Categories

(Core :: Audio/Video: Playback, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

Details

Attachments

(3 files)

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.