1274030 - HTMLMediaElement needs to drop mVideoFrameContainer sooner

Status: RESOLVED FIXED

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

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: 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?

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Can you find someone to look into this?

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

JW - does this come into the shutdown improvement stuff?

Comment 3

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

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?

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 5

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

Attachment: 1274030_part_1.patch

Hi Kyle,

Can you try if this patch works for you?
Btw, this patch doesn't fix the MSG case per commment 3.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 7

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

Btw, I can't repro the crash on Linux. Did it only happen on Windows machines?

Comment 8

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

Attachment: 1274030_part_1_v2.patch

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.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 10

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

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.

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 12

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

Thanks for the review!

Comment 13

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

Attachment: 1274030_fix_v3.patch

fix comments.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/54bf4ff4aa75
clean VideoFrameContainer when "xpcom-shutdown" event received. r=cpearce

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/54bf4ff4aa75