1346439 - Crash in RefPtr<T>::assign_assuming_AddRef | RefPtr<T>::assign_with_AddRef | mozilla::a11y::NotificationController::CoalesceMutationEvents

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-cae495d1-d988-4da2-bd7a-4eaf12170309.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	RefPtr<mozilla::a11y::AccTreeMutationEvent>::assign_assuming_AddRef(mozilla::a11y::AccTreeMutationEvent*) 	obj-firefox/dist/include/mozilla/RefPtr.h:62
1 	xul.dll 	RefPtr<mozilla::a11y::AccReorderEvent>::assign_with_AddRef(mozilla::a11y::AccReorderEvent*) 	obj-firefox/dist/include/mozilla/RefPtr.h:56
2 	xul.dll 	mozilla::a11y::NotificationController::CoalesceMutationEvents() 	accessible/base/NotificationController.cpp:341
3 	xul.dll 	mozilla::a11y::NotificationController::WillRefresh(mozilla::TimeStamp) 	accessible/base/NotificationController.cpp:805
4 	xul.dll 	nsRefreshDriver::Tick(__int64, mozilla::TimeStamp) 	layout/base/nsRefreshDriver.cpp:1798
5 	xul.dll 	mozilla::RefreshDriverTimer::TickDriver(nsRefreshDriver*, __int64, mozilla::TimeStamp) 	layout/base/nsRefreshDriver.cpp:326
6 	xul.dll 	mozilla::RefreshDriverTimer::TickRefreshDrivers(__int64, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) 	layout/base/nsRefreshDriver.cpp:295
7 	xul.dll 	mozilla::RefreshDriverTimer::Tick(__int64, mozilla::TimeStamp) 	layout/base/nsRefreshDriver.cpp:316
8 	xul.dll 	mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::TimeStamp) 	layout/base/nsRefreshDriver.cpp:663
9 	xul.dll 	mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) 	layout/base/nsRefreshDriver.cpp:583
10 	xul.dll 	mozilla::detail::RunnableMethodImpl<void ( mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(mozilla::TimeStamp), 1, 0, mozilla::TimeStamp>::Run() 	obj-firefox/dist/include/nsThreadUtils.h:810
11 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1216
12 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:96
13 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:225
14 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:205
15 	xul.dll 	nsBaseAppShell::Run() 	widget/nsBaseAppShell.cpp:156
16 	xul.dll 	nsAppShell::Run() 	widget/windows/nsAppShell.cpp:262
17 	xul.dll 	nsAppStartup::Run() 	toolkit/components/startup/nsAppStartup.cpp:283
18 	xul.dll 	XREMain::XRE_mainRun() 	toolkit/xre/nsAppRunner.cpp:4472
19 	xul.dll 	XREMain::XRE_main(int, char** const, nsXREAppData const*) 	toolkit/xre/nsAppRunner.cpp:4605
20 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp:4696
21 	firefox.exe 	do_main 	browser/app/nsBrowserApp.cpp:282
22 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:115
23 	firefox.exe 	__scrt_common_main_seh 	f:/dd/vctools/crt/vcstartup/src/startup/exe_common.inl:253
24 	kernel32.dll 	BaseThreadInitThunk 	
25 	ntdll.dll 	__RtlUserThreadStart 	
26 	ntdll.dll 	_RtlUserThreadStart

this crash signature is starting to show up in firefox 52 & later. so far it's happening on 32bit builds on windows. a number of user comments seems to indicate that these crashes happen repeatedly while filling out some job application questionnaire.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

All the crashes are at address 0x18. Can you please take a look, Trev?

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: fixup linked list manipulation in CoalesceMutationEvents()

Its possible to coalesce away events such that the first two events in the
  queue are reorder events where the second reorder can be coalesced with the
  first.  In that case there's no point in shuffling the list before removing
  the second reorder event.

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 8848209 [details] [diff] [review]
fixup linked list manipulation in CoalesceMutationEvents()

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

r+. This is a bit complicated. If my scribbles are correct this patch will avoid the 'reorder previous' pointing to itself, which seems like a good thing to avoid.

Comment 4

[Trevor Saunders (:tbsaunde)]

> r+. This is a bit complicated. If my scribbles are correct this patch will
> avoid the 'reorder previous' pointing to itself, which seems like a good
> thing to avoid.

in a way its actually not that complicated, but that's not quiet write.  In general we start out with something like

mFirstMutationEvent -> ... > reorder <=> ... <=> event <=> ...
where the order of reorder and event can be reversed.

We want to produce this exact sequence
mFirstMutationEvent -> ... <=> reorder <=> event <=> ...
that is event is immediately after reorder.

In the general case this all works fine, the problematic case is this.
mFirstMutationEvent-> reorder <=> event <=> ...

in that case with the old code we would set mFirstMutationEvent to point to event that is reorder->next.  Then we'd set event->prev = reorder->prev.  Then we'd try to set event->prev->next = reorder.  However that was just null->next which of course faulted.

So the first obvious fix would be to handle the special case of event->prev is null.  However its worth noting this issue can only happen if reorder is already immediately before event, which is where we want it, so messing with the list is just extra work.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/023faad1dc96
fixup linked list manipulation in CoalesceMutationEvents() r=davidb

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/023faad1dc96

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta/ESR52 approval on this when you get a chance.

Comment 8

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8848209 [details] [diff] [review]
fixup linked list manipulation in CoalesceMutationEvents()

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: crashes, and Ryan asked me to.
User impact if declined: crashes
Fix Landed on Version:55
Risk to taking this patch (and alternatives if risky): low, complicated code, but fix is fairly straight forward.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]:1270916
[User impact if declined]:crashes
[Is this code covered by automated tests?]:yes, though no new tests for this bug in particular.
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no, str unknown.
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]:should be low, complicated code, but fix is fairly straight forward.
[Why is the change risky/not risky?]:
[String changes made/needed]:none

Comment 9

[Gerry Chang [:gchang]]

Comment on attachment 8848209 [details] [diff] [review]
fixup linked list manipulation in CoalesceMutationEvents()

Fix a crash. Aurora54+ & Beta53+.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/272b6311b65e

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b387337a6728

Comment 12

[Julien Cristau [:jcristau]]

Comment on attachment 8848209 [details] [diff] [review]
fixup linked list manipulation in CoalesceMutationEvents()

crash fix for esr52

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/a9fcc2dc324a