Closed Bug 1274030 Opened 4 years ago Closed 4 years ago

HTMLMediaElement needs to drop mVideoFrameContainer sooner

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: khuey, Assigned: jwwang)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached file backtrace
VideoFrameContainer participates in an object graph that includes ImageBridgeChild, which wants to run code on the IPC thread from its dtor. But the IPC thread is going to be shut down before the final cycle collection in bug 1274028.

Can we find a safe place to call ResetState() on HTMLMediaElement? Maybe when it's removed from the DOM in UnbindFromTree?
Can you find someone to look into this?
Flags: needinfo?(ajones)
JW - does this come into the shutdown improvement stuff?
Flags: needinfo?(ajones) → needinfo?(jwwang)
Flags: needinfo?(ajones)
https://hg.mozilla.org/mozilla-central/file/f3f2fa1d7eed5a8262f6401ef18ff8117a3ce43e/dom/html/HTMLMediaElement.cpp#l3342

HTMLMediaElement could pass the VideoFrameContainer to a MediaStream which will keep it alive. So we actually want to ensure VideoFrameContainer and its underlying ImageContainer are deleted before IPC shutdown, right? Or we just want VideoFrameContainer to be deleted?

Btw, does IPC shutdown happen before or after the "xpcom-shutdown" event?
Flags: needinfo?(jwwang) → needinfo?(khuey)
After.

xpcom-shutdown is at https://hg.mozilla.org/mozilla-central/annotate/c4449eab07d3/xpcom/build/XPCOMInit.cpp#l864

The IPC thread currently shuts down at https://hg.mozilla.org/mozilla-central/annotate/c4449eab07d3/xpcom/build/XPCOMInit.cpp#l1033, but that's going to move to https://hg.mozilla.org/mozilla-central/annotate/c4449eab07d3/xpcom/build/XPCOMInit.cpp#l897.  It'll still be after xpcom-shutdown though.  The problem is that there could be HTMLMediaElements alive until https://hg.mozilla.org/mozilla-central/annotate/c4449eab07d3/xpcom/build/XPCOMInit.cpp#l968, which is before where we're currently shutting down IPC but after where I want to shut it down.
Flags: needinfo?(khuey)
Summary: HTMLMediaFrameElement needs to drop mVideoFrameContainer sooner → HTMLMediaElement needs to drop mVideoFrameContainer sooner
Attached file 1274030_part_1.patch (obsolete) —
Hi Kyle,

Can you try if this patch works for you?
Btw, this patch doesn't fix the MSG case per commment 3.
Flags: needinfo?(khuey)
No, it doesn't.  If you grab the patch in bug 1274028 and then run

./mach mochitest --disable-e10s dom/security/test/mixedcontentblocker

You should see the crash.  Basically there's a cycle with HTMLMediaElement that isn't destroyed until the final cycle collection because something else (appears to be a DOM event) has a reference into the cycle.  Which would be fine, except that the cycle holds the ImageBridgeChild alive.
Flags: needinfo?(khuey)
Flags: needinfo?(ajones)
Priority: -- → P1
Btw, I can't repro the crash on Linux. Did it only happen on Windows machines?
Attached patch 1274030_part_1_v2.patch (obsolete) — Splinter Review
Hi kyle,
Can you try if this patch work?

Btw, I notice that even after applying this patch, there is still webrtc in some test cases on Try to keep VideoFrameContainer alive after IPC shutdown.
Attachment #8754214 - Attachment is obsolete: true
Flags: needinfo?(khuey)
Yes, this fixes my problem, at least with that test.  Thank you for jumping on this.  I would have responded sooner but I was on vacation for the weeks preceding London.
Flags: needinfo?(khuey)
Comment on attachment 8755249 [details] [diff] [review]
1274030_part_1_v2.patch

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

Create a persist class to listen to shutdown events so HTMLMediaElement has a chance to drop mVideoFrameContainer when "xpcom-shutdown" is received even when self reference is removed. The original code will prevent HTMLMediaElement from receiving the shutdown event if self reference is removed (in DoRemoveSelfReference()).

Also we need to drop reference to VideoFrameContainer for MediaDecoder and its friends upon Shutdown() calls.
Attachment #8755249 - Flags: review?(cpearce)
Comment on attachment 8755249 [details] [diff] [review]
1274030_part_1_v2.patch

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

Looks good.

::: dom/html/HTMLMediaElement.cpp
@@ +2234,5 @@
> +    MOZ_DIAGNOSTIC_ASSERT(mWeak);
> +    mWeak = nullptr;
> +    nsContentUtils::UnregisterShutdownObserver(this);
> +  }
> +  void AddSelfReference() {

I think saying "SelfReference" here is a bit misleading, as the shutdown observer is not addrefing itself, it's addrefing the media element.

@@ +4640,5 @@
>  }
>  
>  void HTMLMediaElement::DoRemoveSelfReference()
>  {
>    // We don't need the shutdown observer anymore. Unregistering releases

Comment here is now out of date.
Attachment #8755249 - Flags: review?(cpearce) → review+
Thanks for the review!
fix comments.
Assignee: nobody → jwwang
Attachment #8755249 - Attachment is obsolete: true
Attachment #8764799 - Flags: review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/54bf4ff4aa75
clean VideoFrameContainer when "xpcom-shutdown" event received. r=cpearce
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/54bf4ff4aa75
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1303347
You need to log in before you can comment on or make changes to this bug.