1348222 - heap-use-after-free in [@ mozilla::dom::Selection::ScrollSelectionIntoViewEvent::Run()]

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: log.txt

Found in mozilla-inbound 20170316-38ecd019eceb

Required fuzzpriv fuzzing plugin.

==4587==ERROR: AddressSanitizer: heap-use-after-free on address 0x60d000125b58 at pc 0x7f2c88f6dbf0 bp 0x7ffc92baf300 sp 0x7ffc92baf2f8
READ of size 8 at 0x60d000125b58 thread T0
    #0 0x7f2c88f6dbef in assign_assuming_AddRef /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:62:17
    #1 0x7f2c88f6dbef in operator= /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:166
    #2 0x7f2c88f6dbef in Forget /home/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1195
    #3 0x7f2c88f6dbef in mozilla::dom::Selection::ScrollSelectionIntoViewEvent::Run() /home/worker/workspace/build/src/layout/generic/nsSelection.cpp:6166
    #4 0x7f2c824d182c in nsThread::ProcessNextEvent(bool, bool*) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1269:14
    #5 0x7f2c824ce158 in NS_ProcessNextEvent(nsIThread*, bool) /home/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:389:10
    #6 0x7f2c832770b1 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/glue/MessagePump.cpp:96:21
...
see log.txt

Comment 1

[Tyson Smith [:tsmith]]

The test is taking longer than expected to reduce. I will upload it asap.

Comment 2

[Mats Palmgren (inactive)]

This looks like a dupe of bug 1343795, which already has a fix waiting to land on 2017-03-21.

Comment 3

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Comment 4

[Tyson Smith [:tsmith]]

FWIW, I could only repro this in mozilla-inbound.

Comment 5

[Mats Palmgren (inactive)]

(In reply to Tyson Smith [:tsmith] from comment #4)
> FWIW, I could only repro this in mozilla-inbound.

Interesting, I saw something similar: I couldn't reproduce the crash at first
in my local mozilla-central ASAN build; I updated the tree and then it crashed.
After applying the fix in bug 1343795 it still crashed!  On the very line added
by that patch:

#0  0x00000000004c7a20 in __asan::ReportGenericError
#1  0x00000000004c84d8 in __asan_report_load8
#2  0x00007fe99e361d3d in nsCycleCollectingAutoRefCnt::incr (this=0x60c0002a9bb0,...
#3  nsCycleCollectingAutoRefCnt::incr (this=0x60c0002a9bb0...
#4  mozilla::dom::Selection::AddRef
#5  0x00007fe99e37ace9 in mozilla::dom::Selection::ScrollSelectionIntoViewEvent::Run nsSelection.cpp:6165
(rr) fr 5
#5  0x00007fe99e37ace9 in mozilla::dom::Selection::ScrollSelectionIntoViewEvent::Run
6165      RefPtr<Selection> kungFuDeathGrip = mSelection;

The ScrollSelectionIntoViewEvent (a.k.a 'this') looks alive and so does this->mSelection.

Hmm, I wonder what happened on trunk recently?
(I'm now at rev 348125:b7ba1d282775, I don't know what my previous rev was but
the update command said: "added 2073 changesets with 13715 changes to 8610 files")

Comment 6

[Mats Palmgren (inactive)]

Actually, this->mSelection isn't alive, it just wasn't poisoned like I expected.
So, this looks like a regression from bug 1347820 at first glance...
"ev.forget()" here:
http://searchfox.org/mozilla-central/rev/006005beff40d377cfd2f69d3400633c5ff09127/layout/generic/nsSelection.cpp#6193,6200
makes 'ev' null, so we assign null to mScrollEvent, which means ~Selection
can't revoke it:
http://searchfox.org/mozilla-central/rev/006005beff40d377cfd2f69d3400633c5ff09127/layout/generic/nsSelection.cpp#3519

Comment 7

[Mats Palmgren (inactive)]

Rev 9339469b766a does not crash, rev c9689473edba does crash.
So this is a regression from bug 1347820.

Comment 8

[Mats Palmgren (inactive)]

Attachment: fix

Comment 9

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

This shows a possible error prone case when labeling runnables.
Using .forget() and then afterwards thinking the runnable is still there.

Comment 10

[Mats Palmgren (inactive)]

Jeremy, it might be a good idea to audit your other labeling changes that
have similar code, in case there are any uses of 'ev' after the ev.forget().
Just want to make sure we don't miss anything...

Comment 11

[Mats Palmgren (inactive)]

Since this is a recent trunk-only regression, I'll go ahead and land it without explicit approval...

Comment 12

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/df0f179a8a9c0755b85e480334601b24069facf9

Comment 13

[Jeremy Chen ([e-mail])]

(In reply to Mats Palmgren (:mats) from comment #10)
> Jeremy, it might be a good idea to audit your other labeling changes that
> have similar code, in case there are any uses of 'ev' after the ev.forget().
> Just want to make sure we don't miss anything...

Okay, I'll double check my other labeling work.
Thank you for the quick fix.

Comment 15

[Mats Palmgren (inactive)]

This was merged to m-c 2017-03-18 15:25:
https://hg.mozilla.org/mozilla-central/rev/df0f179a8a9c