1484559 - use-after-poison in [@ RefreshDriver]

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

==5887==ERROR: AddressSanitizer: use-after-poison on address 0x6250002b2c60 at pc 0x7f71927bd7d0 bp 0x7ffdb0f36440 sp 0x7ffdb0f36438
READ of size 8 at 0x6250002b2c60 thread T0
    #0 0x7f71927bd7cf in RefreshDriver src/layout/generic/nsGfxScrollFrame.cpp:1865:21
    #1 0x7f71927bd7cf in RemoveObserver src/layout/generic/nsGfxScrollFrame.cpp:1875
    #2 0x7f71927bd7cf in ~AsyncSmoothMSDScroll src/layout/generic/nsGfxScrollFrame.cpp:1859
    #3 0x7f71927bd7cf in mozilla::ScrollFrameHelper::AsyncSmoothMSDScroll::Release() src/layout/generic/nsGfxScrollFrame.cpp:1774
    #4 0x7f71921a147c in Release src/obj-firefox/dist/include/mozilla/RefPtr.h:42:11
    #5 0x7f71921a147c in Release src/obj-firefox/dist/include/mozilla/RefPtr.h:407
    #6 0x7f71921a147c in ~RefPtr src/obj-firefox/dist/include/mozilla/RefPtr.h:80
    #7 0x7f71921a147c in nsRefreshDriver::Tick(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:1877
    #8 0x7f71921b4ba2 in TickDriver src/layout/base/nsRefreshDriver.cpp:324:13
    #9 0x7f71921b4ba2 in mozilla::RefreshDriverTimer::TickRefreshDrivers(mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) src/layout/base/nsRefreshDriver.cpp:299
    #10 0x7f71921b46c1 in mozilla::RefreshDriverTimer::Tick(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:316:5
    #11 0x7f71921b7cb1 in RunRefreshDrivers src/layout/base/nsRefreshDriver.cpp:755:5
    #12 0x7f71921b7cb1 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:671
    #13 0x7f71921b1b99 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() src/layout/base/nsRefreshDriver.cpp:512:20
    #14 0x7f7187edd240 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1235:14
    #15 0x7f7187ee5fa5 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:519:10
    #16 0x7f71890c3244 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:125:5
    #17 0x7f7188fc50cc in RunInternal src/ipc/chromium/src/base/message_loop.cc:325:10
    #18 0x7f7188fc50cc in RunHandler src/ipc/chromium/src/base/message_loop.cc:318
    #19 0x7f7188fc50cc in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298
    #20 0x7f7191adfaf6 in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:158:27
    #21 0x7f7195b8ef7b in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:290:30
    #22 0x7f7195e47e7c in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:4799:22
    #23 0x7f7195e4af9d in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4944:8
    #24 0x7f7195e4c74e in XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:5036:21
    #25 0x4f591c in do_main src/browser/app/nsBrowserApp.cpp:233:22
    #26 0x4f591c in main src/browser/app/nsBrowserApp.cpp:311
    #27 0x7f71ab52d82f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
    #28 0x424edc in _start (firefox+0x424edc)

0x6250002b2c60 is located 2912 bytes inside of 8192-byte region [0x6250002b2100,0x6250002b4100)
allocated by thread T0 here:
    #0 0x4c5623 in malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:88:3
    #1 0x7f7187e70733 in AllocateChunk src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:193:15
    #2 0x7f7187e70733 in InternalAllocate src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:228
    #3 0x7f7187e70733 in Allocate src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:75
    #4 0x7f7187e70733 in mozilla::ArenaAllocator<8192ul, 8ul>::Allocate(unsigned long) src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:80
    #5 0x7f71924b183a in AllocateByFrameID src/layout/base/nsPresArena.h:39:12
    #6 0x7f71924b183a in AllocateFrame src/layout/base/nsIPresShell.h:206
    #7 0x7f71924b183a in operator new src/layout/generic/ViewportFrame.cpp:34
    #8 0x7f71924b183a in NS_NewViewportFrame(nsIPresShell*, mozilla::ComputedStyle*) src/layout/generic/ViewportFrame.cpp:31
    #9 0x7f71922df18e in nsCSSFrameConstructor::ConstructRootFrame() src/layout/base/nsCSSFrameConstructor.cpp:2661:5
    #10 0x7f719220f381 in mozilla::PresShell::Initialize() src/layout/base/PresShell.cpp:1799:36
    #11 0x7f718be4dd87 in nsContentSink::StartLayout(bool) src/dom/base/nsContentSink.cpp:1274:26
    #12 0x7f718a862a92 in nsHtml5TreeOpExecutor::StartLayout(bool*) src/parser/html/nsHtml5TreeOpExecutor.cpp:673:18
    #13 0x7f718a85dede in nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**, bool*, bool*) src/parser/html/nsHtml5TreeOperation.cpp:1204:17
    #14 0x7f718a85ae36 in nsHtml5TreeOpExecutor::RunFlushLoop() src/parser/html/nsHtml5TreeOpExecutor.cpp:489:17
    #15 0x7f718a8672db in nsHtml5ExecutorFlusher::Run() src/parser/html/nsHtml5StreamParser.cpp:121:18
    #16 0x7f7187edd240 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1235:14
    #17 0x7f7187ee5fa5 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:519:10
    #18 0x7f71890c325e in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
    #19 0x7f7188fc50cc in RunInternal src/ipc/chromium/src/base/message_loop.cc:325:10
    #20 0x7f7188fc50cc in RunHandler src/ipc/chromium/src/base/message_loop.cc:318
    #21 0x7f7188fc50cc in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298
    #22 0x7f7191adfaf6 in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:158:27
    #23 0x7f7195b8ef7b in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:290:30
    #24 0x7f7195e47e7c in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:4799:22
    #25 0x7f7195e4af9d in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4944:8
    #26 0x7f7195e4c74e in XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:5036:21
    #27 0x4f591c in do_main src/browser/app/nsBrowserApp.cpp:233:22
    #28 0x4f591c in main src/browser/app/nsBrowserApp.cpp:311

Comment 1

[Tyson Smith [:tsmith]]

Attachment: prefs.js

required to repro

Comment 2

[Mats Palmgren (inactive)]

Should be protected by frame-poisoning:

#2  0x00007f8caa936629 in mozilla::ScrollFrameHelper::AsyncSmoothMSDScroll::RefreshDriver (this=0x611000b08500, aCallee=0x625000d7abd0) at layout/generic/nsGfxScrollFrame.cpp:1816
1816        return aCallee->mOuter->PresContext()->RefreshDriver();
(rr) p aCallee->mOuter
$1 = (nsContainerFrame *) 0x7ffffffff0dea7ff

Comment 3

[Mats Palmgren (inactive)]

Attachment: For posterity, this is the stack when the scroll frame is destroyed

Comment 4

[Mats Palmgren (inactive)]

Attachment: For posterity, this is the stack when the observer is destroyed

Comment 5

[Mats Palmgren (inactive)]

These prefs are necessary to crash when loading the attached testcase:
user_pref("layout.accessiblecaret.enabled", true);
user_pref("browser.tabs.remote.autostart", false);
user_pref("browser.tabs.remote.autostart.1", false);
user_pref("browser.tabs.remote.autostart.2", false);

Comment 6

[Mats Palmgren (inactive)]

Attachment: fix

The problem is that the current RemoveObserver() code assumes
that the scroll frame outlives its mAsyncScroll / mAsyncSmoothMSDScroll
instances, i.e. that those strong refs are the last (only?) ones.
This is a false assumption since the RefreshDriver holds a local
strong ref on the stack while doing the observer callback.
The testcase destroys the frame in that callback so the observer
dtor runs after the frame is already destroyed.  When the local
instance goes out of scope its dtor call RemoveObserver()
which uses the destroyed frame. (see stacks above)

Comment 7

[Mats Palmgren (inactive)]

BTW, I deliberately left out the details above from the patch
since I'm not sure this crash is always mitigated by frame
poisoning or if there's a way these callback can also destroy
the shell in which case we would have a regular use-after-free.
I don't think so, but just in case.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 9002599 [details] [diff] [review]
fix

Review of attachment 9002599 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d8d234a983ae65eee63239a8536dd24428f6db1
https://hg.mozilla.org/mozilla-central/rev/6d8d234a983a

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Per IRC discussion with Mats, this can ride the trains.

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 9002599 [details] [diff] [review]
fix

Needed for bug 1490561 - see uplift request there.

Comment 12

[Pascal Chevrel:pascalc (PTO until April 26)]

Mats, the patch in comment #9 landed during the 63 nightly cycle so it should be in 63 beta now but you requested an uplift to the beta branch, is there something else in this bug to uplift to 63 beta? Thanks

Comment 13

[Mats Palmgren (inactive)]

Oh OK, only the other branches then :-)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9002599 [details] [diff] [review]
fix

Per bug 1490561 c19.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9002599 [details] [diff] [review]
fix

This doesn't graft cleanly to ESR60. Please attach a rebased patch and re-request approval.

Comment 16

[Mats Palmgren (inactive)]

Attachment: esr60 patch

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: 

User impact if declined: 

Fix Landed on Version: 

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): 

String or UUID changes made by this patch:

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9014568 [details] [diff] [review]
esr60 patch

Needed for bug 1490561, approved for ESR 60.3.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/99e58b5307ce

Comment 19

[Daniel Bodea [:danibodea]]

I have attempted to reproduce this issue in order to verify it in Nightly, but I could not.

After loading the prefs mentioned in comment 1, the following error would be displayed in Nightly:
"The proxy server is refusing connections" and the steps could not be continued.

If I set the prefs mentioned in comment 5, and load test case from comment 0, the crash would not reproduce.

What am I missing? I have attempted to reproduce the crash in Nightly v63.0a1 from 2018-08-15.

Comment 20

[Daniel Bodea [:danibodea]]

Is it OS specific?

Comment 21

[Mats Palmgren (inactive)]

(In reply to Bodea Daniel [:danibodea] from comment #19)
> If I set the prefs mentioned in comment 5, and load test case from comment
> 0, the crash would not reproduce.

Right, the only prefs you need are the ones in comment 5.

> What am I missing? I have attempted to reproduce the crash in Nightly
> v63.0a1 from 2018-08-15.

The crash will likely only reproduce in an ASAN Linux build.
It might take a few minutes for the crash to occur (one of
the testacases for this bug and bug 1490561 did anyway -
I don't recall which one).

Comment 22

[Daniel Bodea [:danibodea]]

I have tried reproducing it again on Ubuntu 16 and Ubuntu 18 and these were my steps:
1. Downloaded a asan-reporter build from a date prior to the date of the push in mozilla central:
http://archive.mozilla.org/pub/firefox/nightly/2018/08/2018-08-20-10-01-29-mozilla-central/firefox-63.0a1.en-US.linux-x86_64-asan-reporter.tar.bz2
2. Opened browser with a profile that has update disabled.
3. Went to "about:config" and set prefs:
user_pref("layout.accessiblecaret.enabled", true);
user_pref("browser.tabs.remote.autostart", false);
4. The other 2 required prefs were not in the pref list, so I created them: 
user_pref("browser.tabs.remote.autostart.1", false);
user_pref("browser.tabs.remote.autostart.2", false);
*Is this a correct step? Is it known that these two prefs aren't there?*
5. Opened the attached test case from comment 0:
https://bugzilla.mozilla.org/attachment.cgi?id=9002323

Actual: The page kept refreshing continuously for more than 20 minutes, but no crash occurred.

@Mats: I cannot verify this issue until I can reproduce it. Do you have any idea of any other details that I might be missing from the reproduction process? 

Thank you!

Comment 23

[Mats Palmgren (inactive)]

I think you need to restart the browser after entering those prefs,
or alternatively just add them directly in the "prefs.js" file in
the profile directory before starting.  So if you did that, then
your steps seem correct.

It isn't super-important to verify this fix though, so don't worry
about it if the crash doesn't occur for you.

Comment 24

[Daniel Bodea [:danibodea]]

I have managed to reproduce the issue in Nightly asan-reporter build from 2018-08-19 using the STR from comment 22 and Mats' suggestion from comment 23 (you were right!) and I have verified the fix in Nightly v63.0a1 from 2018-08-31.
The crash did not occur.

Furthermore, I would need an asan-reporter build for Beta and ESR60 verification, wouldn't I?

Could I also verify it for those versions? Also, considering Mats' comment 23, should I?

Thanks!

Comment 25

[Daniel Bodea [:danibodea]]

The fix is now verified in Beta v63.0b13, asan build containing the fix, from taskcluster:
https://index.taskcluster.net/v1/task/gecko.v2.mozilla-beta.latest.firefox.linux64-asan-opt/artifacts/public/build/target.tar.bz2
No crash occurred.

The fix is also verified in ERS60 v60.2.3esr, asan build containing the fix, from taskcluster:
https://index.taskcluster.net/v1/task/gecko.v2.mozilla-esr60.latest.firefox.linux64-asan-opt/artifacts/public/build/target.tar.bz2
No crash occurred either.

Uplift Successful! Thank you!