<a class="header-button" href="https://bugzilla.mozilla.org/home" title="Go to home page"> Bugzilla

Comment 23

•

7 years ago

So the proximate cause of the assert is that the event we are dispatching has an mOwner that is an nsGlobalWindowInner which has been torn down, such that its mWrapper is null. Then when we land in the nsIGlobalObject* overload of FindAssociatedGlobal we do: if (!p) { return JS::CurrentGlobalOrNull(cx); } JSObject* global = p->GetGlobalJSObject(); if (!global) { return nullptr; } which returns null. That propagates out to Event_Binding::Wrap, which returns false due to failure to FindAssociatedGlobal, which then propagates out through GetOrCreateDOMReflector to the assertion point in EventListener::HandleEvent. The event we're firing is being fired via AsyncEventDispatcher and is an "UnselectedTabHover:Disable" event. It looks to me that the window was already torn down by the time BackgroundVideoDecodingPermissionObserver::DisableEvent ran. At that point, doc->GetScopeObject() returns an inner window with no wrapper. And this->GetOwnerWindow() returns null. The stack to the point where we create that AsyncEventDispatcher looks like this: #0 mozilla::AsyncEventDispatcher::AsyncEventDispatcher (this=0x796919a0, aTarget=0x53d82f838000, aEventType=..., aBubbles=true, aOnlyChromeDispatch=true) at /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/AsyncEventDispatcher.h:37 #1 0x0000000072f0a5b3 in mozilla::BackgroundVideoDecodingPermissionObserver::DisableEvent (this=<optimized out>) at /builds/worker/workspace/build/src/dom/media/BackgroundVideoDecodingPermissionObserver.cpp:133 #2 0x0000000072f1058b in mozilla::BackgroundVideoDecodingPermissionObserver::UnregisterEvent (this=0x7f1e8257c550) at /builds/worker/workspace/build/src/dom/media/BackgroundVideoDecodingPermissionObserver.cpp:94 #3 0x0000000072f82e88 in mozilla::MediaDecoder::Shutdown (this=this@entry=0x525181a5e00) at /builds/worker/workspace/build/src/dom/media/MediaDecoder.cpp:323 #4 0x0000000072f248f4 in mozilla::ChannelMediaDecoder::Shutdown (this=0x525181a5e00) at /builds/worker/workspace/build/src/dom/media/ChannelMediaDecoder.cpp:268 #5 0x0000000072e96008 in mozilla::dom::HTMLMediaElement::ShutdownDecoder (this=this@entry=0x766f71f61800) at /builds/worker/workspace/build/src/dom/html/HTMLMediaElement.cpp:1732 #6 0x0000000072ea1827 in mozilla::dom::HTMLMediaElement::~HTMLMediaElement (this=0x766f71f61800, __in_chrg=<optimized out>) at /builds/worker/workspace/build/src/dom/html/HTMLMediaElement.cpp:3885 #7 0x0000000072e5793f in mozilla::dom::HTMLAudioElement::~HTMLAudioElement (this=0x766f71f61800, __in_chrg=<optimized out>) at /builds/worker/workspace/build/src/dom/html/HTMLAudioElement.cpp:36 #8 0x0000000072e57959 in mozilla::dom::HTMLAudioElement::~HTMLAudioElement (this=0x766f71f61800, __in_chrg=<optimized out>) at /builds/worker/workspace/build/src/dom/html/HTMLAudioElement.cpp:39 #9 0x000000007182b9e6 in SnowWhiteKiller::~SnowWhiteKiller (this=this@entry=0x7ffd8e8c1fa0, __in_chrg=<optimized out>) at /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2729 #10 0x000000007182bfe5 in RemoveSkippableVisitor::~RemoveSkippableVisitor (this=0x7ffd8e8c1fa0, __in_chrg=<optimized out>) at /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2833 so we're way into killing off this node and its document, and certainly shouldn't be firing events by this point. Olli, should we be doing something in AsyncEventDispatcher to prevent firing the event if the event's putative global is gone like this? Chris, should this code really be trying to fire an event with that stack? Seems pretty fishy...

Flags: needinfo?(cpearce)

Flags: needinfo?(bugs)

Comment 24

•

7 years ago

Oh, and maybe we should fall back to the current global if the nsIGlobalObject is torn down like this in FindAssociatedGlobal...

Chris Pearce [:cpearce (Not reading bugmail)]

Assignee

Comment 25

•

7 years ago

Media code here certainly looks rather weird. I'm not so sure about AsyncEventDispatcher. Since it should just work, given that events are used also as a communication channel between content and chrome and so. I think I'd prefer if FindAssociatedGlobal would fall back to current global.

Flags: needinfo?(bugs)

Comment 26

•

7 years ago

Alwu wrote this code, so I'll defer this to him... Can we assume that the browser object in chrome code is being destroyed too?

Flags: needinfo?(cpearce) → needinfo?(alwu)

Alastor Wu [:alwu]

Comment 27

•

7 years ago

The disable event should only be dispatched if the Chrome browser exists. That event is used to notify the Chrome browser that you don't need to send any tab-hover event afterward [1]. Although I don't really understand the relationship between the Chrome browser and outer window, could we assume they're in same lifetime? If so, we could add the null check for outer window in Enable/DisableEvent() [2]. [1] https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/toolkit/content/widgets/browser.xml#725 [2] https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/dom/media/BackgroundVideoDecodingPermissionObserver.cpp#122

Flags: needinfo?(alwu) → needinfo?(bugs)

Comment 28

•

7 years ago

> Can we assume that the browser object in chrome code is being destroyed too? No. > could we assume they're in same lifetime? No, in the sense that the chrome <browser> can get removed from the DOM and continue to exist as an object while the outer window inside it will be destroyed. However, if you can't reach the outer window from the document, then you can't reach the chrome <browser> either and there's no point in firing the event.

Assignee

Updated

•

7 years ago

Blocks: 1397297

Assignee

Comment 29

•

7 years ago

Attached patch fallback_to_current_global.diff — Details — Splinter Review

So, perhaps this?

Flags: needinfo?(bugs)

Attachment #8998240 - Flags: review?(bzbarsky)

Alastor Wu [:alwu]

Updated

•

7 years ago

Comment 30

•

7 years ago

(the patch looks rather promising from bug 1397297's point of view)

https://hg.mozilla.org/mozilla-central/rev/b3a5c5824ad8

Comment 31

•

7 years ago

Comment on attachment 8998240 [details] [diff] [review] fallback_to_current_global.diff r=me

Attachment #8998240 - Flags: review?(bzbarsky) → review+

Pulsebot

Comment 32

•

7 years ago

Pushed by opettay@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b3a5c5824ad8 fallback to current global in FindAssociatedGlobal when nsIGlobalObject doesn't have JS object anymore, r=bz

Anne (:annevk)

Updated

•

7 years ago

Priority: -- → P2

Andreea Pavel [:apavel]

Comment 33

•

7 years ago

bugherder

Status: NEW → RESOLVED

Closed: 7 years ago

status-firefox63: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla63

Calixte Denizet (:calixte)

Updated

•

7 years ago

Assignee: nobody → bugs

Comment 34

•

7 years ago

Please request Beta approval on this when you get a chance.

status-firefox61: --- → unaffected

status-firefox62: --- → affected

status-firefox-esr52: --- → unaffected

status-firefox-esr60: --- → unaffected

Flags: needinfo?(bugs)

Assignee

Comment 35

•

7 years ago

Comment on attachment 8998240 [details] [diff] [review] fallback_to_current_global.diff Approval Request Comment [Feature/Bug causing the regression]: Old stuff [User impact if declined]: This is basically to avoid assertion failures when running tests [Is this code covered by automated tests?]: tests do seem to trigger this code occasionally, causing failures [Has the fix been verified in Nightly?]: So far looking good [Needs manual test from QE? If yes, steps to reproduce]: NA [List of other uplifts needed for the feature/fix]: NA [Is the change risky?]: Shouldn't be too risky, falling back to the same global in case nsIGlobalObject is being deleted as what is used if there is no nsIGlobalObject anymore at all [Why is the change risky/not risky?]: See above [String changes made/needed]: NA

Flags: needinfo?(bugs)

Attachment #8998240 - Flags: approval-mozilla-beta?

Comment 36

•

7 years ago

Comment on attachment 8998240 [details] [diff] [review] fallback_to_current_global.diff Helps avoid oranges in CI. Approved for 62.0b17.

Attachment #8998240 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

https://hg.mozilla.org/releases/mozilla-beta/rev/a1f21507bec3

Comment 37

•

7 years ago

bugherder uplift

status-firefox62: affected → fixed

Comment hidden (Intermittent Failures Robot)

Julien Cristau [:jcristau]

Assignee

Comment 39

•

6 years ago

ok, this is revealing some other bug. Needs to be backed out from beta. https://bugzilla.mozilla.org/show_bug.cgi?id=1482029#c29

Dorel Luca [:dluca]

Comment 40

•

6 years ago

As requested by Smaug: Backed out changeset a1f21507bec3 (bug 1451172) for conflicts with bug 1482029. a=backout

Updated

•

6 years ago

status-firefox62: fixed → wontfix

Updated

•

6 years ago

Depends on: 1482029

Updated

•

6 years ago

Attachment #8998240 - Flags: approval-mozilla-beta+ → approval-mozilla-beta-