1298504 - Crash in mozilla::EventListenerManager::RemoveEventListenerInternal

Status: RESOLVED INCOMPLETE

(Core :: DOM: Events, defect, P3)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

This bug was filed from the Socorro interface and is 
report bp-fc75ddee-6cab-4a0c-9578-fe01b2160826.
=============================================================

Very low volume crash (9 crashes in all of 51 Nightly starting with the 8-20 Nightly). The crashes come from different installations so it's unlikely to be a problem with a particular installation/machine. The crash addresses are all over the place, including one that looks like a poisoned value, so I'm concerned that it's a use-after-free situation, which would make this security-sensitive.

Furthermore, the crash happens at [1] (while accessing EventListenerManager::mListener). I think it implies a bad `this` pointer and also possible use-after-free.

[1] https://hg.mozilla.org/mozilla-unified/annotate/01748a2b1a46/dom/events/EventListenerManager.cpp#l664

Comment 1

[Andrew McCreight [:mccr8]]

These crashes are all happening on this line in nsFileControlFrame::DestroyFrom():
    mContent->RemoveSystemEventListener(NS_LITERAL_STRING("drop"),
                                        mMouseListener, false);

Maybe we're missing a kungFuDeathGroup on mContent or mMouseListener?

Comment 2

[Andrew McCreight [:mccr8]]

It looks like Olli is on PTO. Could you take a look at figuring out what is going on here, Masayuki? Thanks.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, looks like this is new regression only in Nightly. So, we must be able to wait smaug because my queue is already full for urgent keyboard input handling regressions found in 48...

Comment 4

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

I can't really see why kungFuDeathGroup would help here. Those objects should stay alive.
We seem to have similar crashes for quite some time.

Still trying to guess what this could be about.

Comment 5

[Andrea Marchesini [:baku]]

Should we not move the |RefPtr<EventListenerManager> kungFuDeathGrip(this)| outside the for loop?
I don't see any break, so it's possible that EventListenerManager is kept alive only by this kungFuDeathGrip.

Comment 6

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

That we could do, but it shouldn't affect to this issue, as far as I see. ELM should stay alive anyhow since the EventTarget object owning it stays alive.

Comment 7

[Andrea Marchesini [:baku]]

If this is true, I don't see why we should have  RefPtr<EventListenerManager> kungFuDeathGrip(this) here:
https://hg.mozilla.org/mozilla-unified/annotate/01748a2b1a46/dom/events/EventListenerManager.cpp#l663

Comment 8

[Andrea Marchesini [:baku]]

Attachment: a.patch

Comment 9

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

Comment on attachment 8787383 [details] [diff] [review]
a.patch

This is fine, but do this in some other bug, so that we don't end up marking this bug fixed when this patch lands.

Comment 10

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

Attachment: safer_elm_RELI.diff

This is from the other bug. Guess fix.

Comment 11

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

Comment on attachment 8787753 [details] [diff] [review]
safer_elm_RELI.diff

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
We don't know how the crash happens. And we don't know whether this patch fixes the issue.
This is a guess fix for something going wrong around this code.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
commit message would be 
"Bug 1298504, ensure existing listeners are correctly counted before disabling some device APIs, r=mccr8"

Which older supported branches are affected by this flaw?
Looks like there has been similar crashes for ages now

If not all supported branches, which bug introduced the flaw?
NA

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
They are very similar, but each branch seems to need tiny bit different patch.


How likely is this patch to cause regressions; how much testing does it need?
Should be safe.

Comment 12

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

Attachment: for aurora if needed

Comment 13

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

Attachment: for beta

Comment 14

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

Attachment: for esr45

Comment 15

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8787753 [details] [diff] [review]
safer_elm_RELI.diff

sec-audit issues don't need sec-approval to land on trunk. Go ahead and check it in there.

Comment 16

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/354f2bd63b9ec4a6507dc7da011dbea0ba37e8e5

Comment 17

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

apparently I suck, or we have broken test
https://treeherder.mozilla.org/logviewer.html#?job_id=35325251&repo=mozilla-inbound#L3147

backed out
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc4fc50592f0ad5ce120fabb1315efd7c58112f2

Comment 18

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

ah, no. It is our Enable/DisableDeviceSensor and their use which is busted

Comment 19

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

(In reply to Olli Pettay [:smaug] from comment #18)
> ah, no. It is our Enable/DisableDeviceSensor and their use which is busted

In other words, the test passes because it happens to rely on the bug with 'i' handling and that means typeCount == 0 is true twice.

Comment 20

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

I'll file a new bug, since it is quite different stuff.

Comment 21

[Tyson Smith [:tsmith]]

Attachment: asan_log.txt

I came across this on a long running fuzzing job. No test case but I have an ASan log. Is this the same issue?

Comment 22

[Andrew McCreight [:mccr8]]

This crash signature is still happening, but I don't see any crashes in the last 6 months on poison values, so I'm going to mark this incomplete.