874486 - ASAN: Crashtest layout/xul/tree/crashtests/409807-1.xul triggers error

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Christian Holler (:decoder)]

This crashtest in content/xul/templates/src/crashtests/329335-1.xul fails with AddressSanitizer on mozilla-central revision 81dd97739fa1. This failure seems to reproduce well in debug and opt builds. Here is the ASan error trace from a debug build:


==6207== ERROR: AddressSanitizer: use-after-poison on address 0x2b82d90df978 at pc 0x2b82c0461c3f bp 0x7fff000b2270 sp 0x7fff000b2268
READ of size 4 at 0x2b82d90df978 thread T0
#0 0x2b82c0461c3e in nsTreeBodyFrame::UpdateScrollbars(nsTreeBodyFrame::ScrollParts const&) layout/xul/tree/nsTreeBodyFrame.cpp:864
#1 0x2b82c045e67b in nsTreeBodyFrame::FullScrollbarsUpdate(bool) layout/xul/tree/nsTreeBodyFrame.cpp:4623
#2 0x2b82c045d2b3 in nsTreeBodyFrame::SetView(nsITreeView*) layout/xul/tree/nsTreeBodyFrame.cpp:520
#3 0x2b82c047b21d in nsTreeBoxObject::GetView(nsITreeView**) layout/xul/tree/nsTreeBoxObject.cpp:154
#4 0x2b82c045b124 in nsTreeBodyFrame::EnsureView() layout/xul/tree/nsTreeBodyFrame.cpp:363
#5 0x2b82c045de49 in nsTreeBodyFrame::ReflowFinished() layout/xul/tree/nsTreeBodyFrame.cpp:409
#6 0x2b82c045e7bc in non-virtual thunk to nsTreeBodyFrame::ReflowFinished() layout/xul/tree/nsTreeBodyFrame.cpp:448
#7 0x2b82bf24fe0b in PresShell::HandlePostedReflowCallbacks(bool) layout/base/nsPresShell.cpp:3697
#8 0x2b82bf2468c2 in PresShell::DidDoReflow(bool) layout/base/nsPresShell.cpp:7634
#9 0x2b82bf25120e in PresShell::ProcessReflowCommands(bool) layout/base/nsPresShell.cpp:7936
#10 0x2b82bf250a69 in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) layout/base/nsPresShell.cpp:3891
#11 0x2b82bf279841 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) layout/base/nsRefreshDriver.cpp:959
#12 0x2b82bf27df0d in mozilla::RefreshDriverTimer::Tick() layout/base/nsRefreshDriver.cpp:158
#13 0x2b82c1b0e28d in nsTimerImpl::Fire() xpcom/threads/nsTimerImpl.cpp:547
#14 0x2b82c1b0eac5 in nsTimerEvent::Run() xpcom/threads/nsTimerImpl.cpp:634
#15 0x2b82c1b0313a in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:627
#16 0x2b82c1a503d1 in NS_ProcessNextEvent(nsIThread*, bool) objdir-ff-asan64dbg/xpcom/build/nsThreadUtils.cpp:238
#17 0x2b82c10be4cb in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:82
#18 0x2b82c1bf4631 in MessageLoop::RunInternal() ipc/chromium/src/base/message_loop.cc:219
#19 0x2b82c1bf444e in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:186
#20 0x2b82c0e40b01 in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:163
#21 0x2b82c09e916f in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:269
#22 0x2b82be99bab1 in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:3872
#23 0x2b82be99cddf in XREMain::XRE_main(int, char**, nsXREAppData const*) toolkit/xre/nsAppRunner.cpp:3939
#24 0x2b82be99d781 in XRE_main toolkit/xre/nsAppRunner.cpp:4140
#25 0x41d1f8 in do_main(int, char**, nsIFile*) browser/app/nsBrowserApp.cpp:272
#26 0x41c72f in main browser/app/nsBrowserApp.cpp:632
#27 0x2b82bc46b76c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226
0x2b82d90df978 is located 6456 bytes inside of 8192-byte region [0x2b82d90de040,0x2b82d90e0040)
allocated by thread T0 here:
#0 0x410568 in __interceptor_malloc _asan_rtl_
#1 0x2b82bc9c9dbb in PL_ArenaAllocate nsprpub/lib/ds/plarena.c:202


I suspect this is not the same issue as in bug 787715. The use-after-poison indicates a use-after-free in our internal NSPR allocator, so marking this as s-s for now.

Comment 1

[Mats Palmgren (inactive)]

Hmm, didn't I fix this recently ... oh, I did:
http://hg.mozilla.org/mozilla-central/rev/4ab11d8ed73b

This seems to be the culprit:
http://hg.mozilla.org/mozilla-central/diff/0c513ba74137/layout/xul/tree/nsTreeBodyFrame.cpp#l1.98
clearly the use of 'mHorzPosition' on line 1.117 is unsafe again now.
And so is the use of 'mScrollbarActivity' on line 1.144

I can't reproduce the crash in my local ASan m-c build, can you try
adding weakFrame.IsAlive() checks in those places?

Comment 2

[Christian Holler (:decoder)]

Um, can you explain me more detailed what I need to add where? I'd be glad to give it a try then.

Comment 3

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/diff/0c513ba74137/layout/xul/tree/nsTreeBodyFrame.cpp#l1.98
Sure, I think we need to add back lines 1.110 - 1.112 with s/self/weakFrame/
and add "weakFrame.IsAlive() && " to the condition on line 1.144

We should probably audit all callers (recursively) to these methods to see
that they are safe against arbitrary layout objects being destroyed.

An alternative might be to add a nsAutoScriptBlocker in a strategic place,
like we did in bug 842166.

Comment 4

[Christian Holler (:decoder)]

confirmed :) With these changes, the test doesn't crash anymore.

Comment 5

[Stephen A Pohl [:spohl]]

Attachment: Patch

Comment 6

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/59c556346255

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/59c556346255

Comment 8

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

How far back does this issue go? Was it trunk only?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's in 23. We'd better uplift the fix.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 752716 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 636564
User impact if declined: practically no impact since XUL isn't accessible remotely
Testing completed (on m-c, etc.): just landed
Risk to taking this patch (and alternatives if risky): very low risk
String or IDL/UUID changes made by this patch: none

Comment 11

[Stephen A Pohl [:spohl]]

Thank you for catching this and requesting approval for aurora!

Comment 12

[Stephen A Pohl [:spohl]]

Requesting checkin for aurora.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/512e62ac59d9