Closed Bug 1471800 Opened 6 years ago Closed 6 years ago

HTMLMediaElement can start playing its MediaDecoder while its docshell is being destroyed

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

Details

Attachments

(1 file)

Bug 1471800 - Ensure HTMLMediaElement doesn't play its MediaDecoder in a readyState update after it's OwnerDoc has been removed from its DocShell.
59 bytes, text/x-review-board-request
jya
: review+
Details 
In bug 1463919 I added assertions that we're allowed to autoplay whenever HTMLMediaElement calls MediaDecoder::Play(). Turns out, one of those assertions was failing with this call stack:


0  0x0000517260236e7d in mozilla::WrapNotNull<mozilla::dom::HTMLMediaElement*>(mozilla::dom::HTMLMediaElement*) (aBasePtr=0x32f462fe5000)
    at /home/cpearce/src/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/NotNull.h:156
#1  0x0000517260236e7d in mozilla::dom::HTMLMediaElement::IsAllowedToPlay() (this=0x32f462fe5000) at /home/cpearce/src/firefox/dom/html/HTMLMediaElement.cpp:7033
#2  0x000051726023536b in mozilla::dom::HTMLMediaElement::ChangeReadyState(unsigned short) (this=0x32f462fe5000, aState=<optimized out>) at /home/cpearce/src/firefox/dom/html/HTMLMediaElement.cpp:6127
#3  0x0000517260239bcb in mozilla::dom::HTMLMediaElement::UpdateReadyStateInternal() (this=0x32f462fe5000) at /home/cpearce/src/firefox/dom/html/HTMLMediaElement.cpp:6033
#4  0x00005172602f7a70 in mozilla::MediaDecoder::UpdateReadyState() (this=<optimized out>) at /home/cpearce/src/firefox/dom/media/MediaDecoder.h:386
#5  0x00005172602f7a70 in mozilla::MediaDecoder::OnNextFrameStatus(mozilla::MediaDecoderOwner::NextFrameStatus) (this=0x56bf33060a00, aStatus=mozilla::MediaDecoderOwner::NEXT_FRAME_AVAILABLE)
    at /home/cpearce/src/firefox/dom/media/MediaDecoder.cpp:448
#6  0x000051726033550e in mozilla::detail::RunnableMethodArguments<mozilla::MediaDecoderOwner::NextFrameStatus&&>::applyImpl<mozilla::detail::Listener<mozilla::MediaDecoderOwner::NextFrameStatus>, void (mozilla::detail::Listener<mozilla::MediaDecoderOwner::NextFrameStatus>::*)(mozilla::MediaDecoderOwner::NextFrameStatus&&), StoreCopyPassByRRef<mozilla::MediaDecoderOwner::NextFrameStatus>, 0ul>(mozilla::detail::Listener<mozilla::MediaDecoderOwner::NextFrameStatus>*, void (mozilla::detail::Listener<mozilla::MediaDecoderOwner::NextFrameStatus>::*)(mozilla::MediaDecoderOwner::NextFrameStatus&&), mozilla::Tuple<StoreCopyPassByRRef<mozilla::MediaDecoderOwner::NextFrameStatus> >&, std::integer_sequence<unsigned long, 0ul>) (o=<optimized out>, m=<optimized out>, args=...)
    at /home/cpearce/src/firefox/obj-x86_64-pc-linux-gnu/dist/include/nsThreadUtils.h:1165

Debugging this a bit with RR, I discovered that a loading HTMLMediaElement had been in a non-paused state, and then the test had ended and the HTMLMediaElement's OwnerDoc was removed from the docshell, and then the MediaDecoder updated its frame status to HAVE_FUTURE_DATA, and so the HTMLMediaElement updated its readyState accordingly and tried to play the MediaDecoder since it wasn't paused.

IsAllowedToPlay() is returning false because the document's inner window is null, because the document has been removed from the docshell, which is why IsAllowedToPlay() disagrees with mPaused.

So basically, we reached readyState>=HAVE_FUTURE_DATA just after the document was detached from its window/docshell, and we started playing because we were in a non-paused state.

Over the years we've had phantom playbacks where we started playing after the tab was closed. I bet this was the cause!

For the record, how the HTMLMediaElement finds out its document was removed from the docshell happens on this call path:

#0  0x0000517260239340 in mozilla::dom::HTMLMediaElement::NotifyOwnerDocumentActivityChanged() (this=0x32f462fe5000) at /home/cpearce/src/firefox/dom/html/HTMLMediaElement.cpp:6646
#1  0x000051725f5de075 in NotifyActivityChanged(nsISupports*, void*) (aSupports=0x32f462fe5000, aUnused=<optimized out>) at /home/cpearce/src/firefox/dom/base/nsDocument.cpp:4566
#2  0x000051725f5e764f in nsIDocument::EnumerateActivityObservers(void (*)(nsISupports*, void*), void*) (aData=0x517264240f44 <nsIContent::COMTypeInfo<nsIContent, void>::kIID>, this=<optimized out>, aEnumerator=<optimized out>) at /home/cpearce/src/firefox/dom/base/nsDocument.cpp:9445
#3  0x000051725f5e764f in nsDocument::RemovedFromDocShell() (this=0x97f42192000) at /home/cpearce/src/firefox/dom/base/nsDocument.cpp:8117
#4  0x0000517260b1c132 in nsDocumentViewer::Close(nsISHEntry*) (this=0x5605079b6ce0, aSHEntry=<optimized out>) at /home/cpearce/src/firefox/layout/base/nsDocumentViewer.cpp:1700
#5  0x00005172617f5457 in nsDocShell::Destroy() (this=0x32f462fe0000) at /home/cpearce/src/firefox/docshell/base/nsDocShell.cpp:5325
#6  0x000051725f60b166 in nsFrameLoader::DestroyDocShell() (this=0x53f676fd6d00) at /home/cpearce/src/firefox/dom/base/nsFrameLoader.cpp:1835
#7  0x000051725f60b0a3 in nsFrameLoaderDestroyRunnable::Run() (this=0x62280250da90) at /home/cpearce/src/firefox/dom/base/nsFrameLoader.cpp:1773
#8  0x000051725f5df37a in nsIDocument::MaybeInitializeFinalizeFrameLoaders() (this=<optimized out>) at /home/cpearce/src/firefox/dom/base/nsDocument.cpp:6553
#9  0x000051725f623307 in mozilla::detail::RunnableMethodArguments<>::applyImpl<nsIDocument, void (nsIDocument::*)()>(nsIDocument*, void (nsIDocument::*)(), mozilla::Tuple<>&, std::integer_sequence<unsigned long>) (o=<optimized out>, m=<optimized out>, args=...) at /home/cpearce/src/firefox/obj-x86_64-pc-linux-gnu/dist/include/nsThreadUtils.h:1165
#10 0x000051725f623307 in _ZN7mozilla6detail23RunnableMethodArgumentsIJEE5applyI11nsIDocumentMS4_FvvEEEDTcl9applyImplfp_fp0_dtdefpT10mArgumentstlSt16integer_sequenceImJEEEEEPT_T0_ (o=<optimized out>, m=<optimized out>, this=<optimized out>) at /home/cpearce/src/firefox/obj-x86_64-pc-linux-gnu/dist/include/nsThreadUtils.h:1171
#11 0x000051725f623307 in mozilla::detail::RunnableMethodImpl<nsIDocument*, void (nsIDocument::*)(), true, (mozilla::RunnableKind)0>::Run() (this=<optimized out>)
    at /home/cpearce/src/firefox/obj-x86_64-pc-linux-gnu/dist/include/nsThreadUtils.h:1216
#12 0x000051725f4ceae9 in nsContentUtils::RemoveScriptBlocker() () at /home/cpearce/src/firefox/dom/base/nsContentUtils.cpp:5637
#13 0x0000517260b1c62f in nsAutoScriptBlocker::~nsAutoScriptBlocker() (this=<optimized out>) at /home/cpearce/src/firefox/obj-x86_64-pc-linux-gnu/dist/include/nsContentUtils.h:3528
#14 0x0000517260b1c62f in nsDocumentViewer::Destroy() (this=0x5172656a0340) at /home/cpearce/src/firefox/layout/base/nsDocumentViewer.cpp:1938
#15 0x00005172617f5467 in nsDocShell::Destroy() (this=0x33f85c1b8000) at /home/cpearce/src/firefox/docshell/base/nsDocShell.cpp:5326
#16 0x000051725f60b166 in nsFrameLoader::DestroyDocShell() (this=0x4479180f9d00) at /home/cpearce/src/firefox/dom/base/nsFrameLoader.cpp:1835
#17 0x000051725f60b0a3 in nsFrameLoaderDestroyRunnable::Run() (this=0x33f85c158b80) at /home/cpearce/src/firefox/dom/base/nsFrameLoader.cpp:1773
#18 0x000051725f5df37a in nsIDocument::MaybeInitializeFinalizeFrameLoaders() (this=<optimized out>) at /home/cpearce/src/firefox/dom/base/nsDocument.cpp:6553
#19 0x000051725f5df0ec in nsDocument::EndUpdate() (this=0x32f462f71000) at /home/cpearce/src/firefox/dom/base/nsDocument.cpp:4948
#20 0x00005172608265fe in mozilla::dom::XULDocument::EndUpdate() (this=0x32f462f71000) at /home/cpearce/src/firefox/dom/xul/XULDocument.cpp:2832
#21 0x000051725f5dc893 in mozAutoDocUpdate::~mozAutoDocUpdate() (this=<optimized out>) at /home/cpearce/src/firefox/dom/base/mozAutoDocUpdate.h:37
#22 0x000051725f5dc893 in nsINode::doRemoveChildAt(unsigned int, bool, nsIContent*, nsAttrAndChildArray&) (this=0x517265648200, aIndex=<optimized out>, aNotify=<optimized out>, aKid=0x50fa72720780, aChildArray=...) at /home/cpearce/src/firefox/dom/base/nsINode.cpp:1734
#23 0x000051725f565ca5 in mozilla::dom::FragmentOrElement::RemoveChildNode(nsIContent*, bool) (this=0x517265648200, aKid=0x50fa72720780, aNotify=true)
    at /home/cpearce/src/firefox/dom/base/FragmentOrElement.cpp:1233
#24 0x00005172608317e1 in nsXULElement::RemoveChildNode(nsIContent*, bool) (this=0x517265648200, aKid=0x50fa72720780, aNotify=<optimized out>) at /home/cpearce/src/firefox/dom/xul/nsXULElement.cpp:889
#25 0x000051725f614858 in nsINode::RemoveChild(nsINode&, mozilla::ErrorResult&) (this=0x517265648200, aOldChild=..., aError=...) at /home/cpearce/src/firefox/dom/base/nsINode.cpp:555
#26 0x000051725f614858 in nsINode::Remove() (this=0x50fa72720780) at /home/cpearce/src/firefox/dom/base/nsINode.cpp:1643
#27 0x000051725fd5e063 in mozilla::dom::ElementBinding::remove(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitMethodCallArgs const&) (cx=0x38d774721000, obj=..., self=0x50fa72720780, args=...) at /home/cpearce/src/firefox/obj-x86_64-pc-linux-gnu/dom/bindings/ElementBinding.cpp:4517
#28 0x000051725ff2fc9c in mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) (cx=0x38d774721000, argc=<optimized out>, vp=<optimized out>) at /home/cpearce/src/firefox/dom/bindings/BindingUtils.cpp:3285
#29 0x00002606c2975510 in  ()
#30 0x00007fff1f809ad8 in  ()
#31 0x00007fff1f809a10 in  ()
Comment on attachment 8988383 [details]
Bug 1471800 - Ensure HTMLMediaElement doesn't play its MediaDecoder in a readyState update after it's OwnerDoc has been removed from its DocShell.

https://reviewboard.mozilla.org/r/253662/#review260300

::: dom/html/HTMLMediaElement.cpp:6126
(Diff revision 1)
>      mLoadedDataFired = true;
>    }
>  
>    if (oldState < HAVE_FUTURE_DATA && mReadyState >= HAVE_FUTURE_DATA) {
>      DispatchAsyncEvent(NS_LITERAL_STRING("canplay"));
> -    if (!mPaused) {
> +    if (!mPaused && !mPausedForInactiveDocumentOrChannel) {

What if the document becomes active again?
the state would have already been modified and now the event would no longer be dispatched nor play() called.

Shouldn't we exit earlier such that mReadyState doesn't get modified at all, so in the event we enter this code again, and the document is now attached it can resume from where it left off?
Attachment #8988383 - Flags: review?(jyavenard) → review+
Comment on attachment 8988383 [details]
Bug 1471800 - Ensure HTMLMediaElement doesn't play its MediaDecoder in a readyState update after it's OwnerDoc has been removed from its DocShell.

https://reviewboard.mozilla.org/r/253662/#review260300

> What if the document becomes active again?
> the state would have already been modified and now the event would no longer be dispatched nor play() called.
> 
> Shouldn't we exit earlier such that mReadyState doesn't get modified at all, so in the event we enter this code again, and the document is now attached it can resume from where it left off?

While we're suspended, any events we try to dispatch are enqueued in mPendingEvents (see the 'if (mEventDeliveryPaused)...' block in HTMLMediaElement::DispatchEvent). When the owning document becomes active again, HTMLMediaElement::SuspendOrResumeElement() is called again by NotifyOwnerDocumentActivityChanged() and the decoder is resumed and any pending events are dispatched. So the media element can continue where it left off.
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/97499b2f5612
Ensure HTMLMediaElement doesn't play its MediaDecoder in a readyState update after it's OwnerDoc has been removed from its DocShell. r=jya
Comment on attachment 8988383 [details]
Bug 1471800 - Ensure HTMLMediaElement doesn't play its MediaDecoder in a readyState update after it's OwnerDoc has been removed from its DocShell.

https://reviewboard.mozilla.org/r/253662/#review260300

> While we're suspended, any events we try to dispatch are enqueued in mPendingEvents (see the 'if (mEventDeliveryPaused)...' block in HTMLMediaElement::DispatchEvent). When the owning document becomes active again, HTMLMediaElement::SuspendOrResumeElement() is called again by NotifyOwnerDocumentActivityChanged() and the decoder is resumed and any pending events are dispatched. So the media element can continue where it left off.

Yes but here we change the ready State, and exit early. The event is never q
q
queued
Comment on attachment 8988383 [details]
Bug 1471800 - Ensure HTMLMediaElement doesn't play its MediaDecoder in a readyState update after it's OwnerDoc has been removed from its DocShell.

https://reviewboard.mozilla.org/r/253662/#review260300

> Yes but here we change the ready State, and exit early. The event is never q
> q
> queued

Did you mean NotifyAboutPlaying() doesn't get the chance to enqueue a playing event?
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/63ee095b5f31
Backed out changeset 97499b2f5612 as requested by cpearce.
(In reply to Chris Pearce (:cpearce) from comment #8)
> Comment on attachment 8988383 [details]
> Bug 1471800 - Ensure HTMLMediaElement doesn't play its MediaDecoder in a
> readyState update after it's OwnerDoc has been removed from its DocShell.
> 
> https://reviewboard.mozilla.org/r/253662/#review260300
> 
> > Yes but here we change the ready State, and exit early. The event is never q
> > q
> > queued
> 
> Did you mean NotifyAboutPlaying() doesn't get the chance to enqueue a
> playing event?

We should be enqueuing a "playing" event here, as there is nothing in the resume code to fire a "playing" event. So if we don't call NotifyAboutPlaying() here, there won't be a "playing" event after the element is resumed and we call MediaDecoder::Play(), which could confuse pages.

I asked for the original push to be backed out, so I can fix it and get a clean single-patch landing, as I think we'll need to uplift this to fix bug 1470932.
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0708054687d7
Ensure HTMLMediaElement doesn't play its MediaDecoder in a readyState update after it's OwnerDoc has been removed from its DocShell. r=jya
https://hg.mozilla.org/mozilla-central/rev/0708054687d7
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
See Also: → 1332888
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: