Closed
Bug 1274030
Opened 8 years ago
Closed 8 years ago
HTMLMediaElement needs to drop mVideoFrameContainer sooner
Categories
(Core :: Audio/Video: Playback, defect, P1)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: khuey, Assigned: jwwang)
References
Details
Attachments
(2 files, 2 obsolete files)
10.04 KB,
text/plain
|
Details | |
12.12 KB,
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
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?
JW - does this come into the shutdown improvement stuff?
Flags: needinfo?(ajones) → needinfo?(jwwang)
Updated•8 years ago
|
Flags: needinfo?(ajones)
Assignee | ||
Comment 3•8 years ago
|
||
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)
Reporter | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Summary: HTMLMediaFrameElement needs to drop mVideoFrameContainer sooner → HTMLMediaElement needs to drop mVideoFrameContainer sooner
Assignee | ||
Comment 5•8 years ago
|
||
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)
Reporter | ||
Comment 6•8 years ago
|
||
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)
Updated•8 years ago
|
Flags: needinfo?(ajones)
Priority: -- → P1
Assignee | ||
Comment 7•8 years ago
|
||
Btw, I can't repro the crash on Linux. Did it only happen on Windows machines?
Assignee | ||
Comment 8•8 years ago
|
||
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)
Reporter | ||
Comment 9•8 years ago
|
||
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)
Assignee | ||
Comment 10•8 years ago
|
||
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 11•8 years ago
|
||
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+
Assignee | ||
Comment 12•8 years ago
|
||
Thanks for the review!
Assignee | ||
Comment 13•8 years ago
|
||
fix comments.
Assignee: nobody → jwwang
Attachment #8755249 -
Attachment is obsolete: true
Attachment #8764799 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 14•8 years ago
|
||
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
Comment 15•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•