1325919 - Crash in mozilla::dom::TabGroup::EventTargetFor

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ting-Yu Chou [:ting] (away)]

This bug was filed from the Socorro interface and is 
report bp-0e240157-0d5f-427a-b7f6-71ef12161225.
=============================================================
Top #11 of Nightly 20161225030206 on Windows, 3 crashes from 3 installations. The first appearance was build 20161225030206, could be from bug 1320753.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

crashers are bad. Either this should get fixed asap or the problematic patches should be backed out.

(I wonder why Bug 1320753 is still open. One shouldn't keep bugs open and land patches from it every now and then. Separate bugs makes managing all the backouts and such easier to follow.)

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: assertion patch

This doesn't seem to be happening very often yet (five crashes in three days so far). I'd like to leave the original patch in to try to see what's happening. This patch adds some more assertions.

Right now we null out some fields on the TabGroup when the last window has left (this avoids the need to cycle collect it). The assertion is supposed to check that we never use those fields after that point. So someone is using a TabGroup after the last window tied to that TabGroup has been destroyed.

One possibility is that the TabGroup gets reused (another window enters after the last window left). So I added an assertion to check for that.

Another possibility is that all the windows are really dead. In that case, the only way to get to the TabGroup is via a DocGroup I think. So I added some assertions to see if we're going through a DocGroup to get to the TabGroup. There are also other possibilities that seem less likely (maybe an existing UAF in the code?). Hopefully one of these new assertions will hit and we'll have a better idea of what is happening.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8822089 [details] [diff] [review]
assertion patch

One should be able to "use" TabGroup/DocGroup even after there are no windows left. Otherwise all the users will need to check if there are windows.

Could we have different setup for TabGroup vs DispatcherEventTarget.
For example, could TabGroup always keep DispatcherEventTarget alive using UniquePtr or so, but
DispatcherEventTarget would forward addref/releasing to TabGroup or something.
I'm not atm familiar with DispatcherEventTarget lifetime management.


But sure, we can first add these assertions.
(However I really don't like keeping crashers even in Nightly. It is so effective way to get people out from using Nightlies).

Could you use MOZ_DIAGNOSTIC_ASSERT here, so that we don't accidentally leak the assertions to beta/release. I think Nightly/Aurora is good enough for this diagnostic stuff.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch v2

I thought about this some more. Maybe it's not right to forbid people from dispatching after the last window dies. There maybe could still be a document alive (I guess). Anyway, this just dispatches to the main thread directly in that case.

The UniquePtr thing would be hard to implement since mEventTargets[i] may point to either a DispatcherEventTarget or to a ThrottledEventQueue (which then points to a DispatcherEventTarget).

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8822278 [details] [diff] [review]
patch v2

wrong patch, or something pushed to mozreview didn't get to bugzilla the right way.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch v3

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/317cb251b8e2
Allow EventTargetFor to be used after last window has been destroyed (r=smaug)

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/317cb251b8e2

Comment 9

[Marco Castelluccio [:marco]]

Are 51 or 52 affected?