Closed Bug 1346439 Opened 8 years ago Closed 7 years ago

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


(Core :: Disability Access APIs, defect)

52 Branch
Not set



Tracking Status
firefox-esr45 --- unaffected
firefox52 --- wontfix
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed


(Reporter: philipp, Assigned: tbsaunde)



(Keywords: crash, regression)

Crash Data


(1 file)

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/
14 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/
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.
All the crashes are at address 0x18. Can you please take a look, Trev?
Flags: needinfo?(tbsaunde+mozbugs)
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.
Attachment #8848209 - Flags: review?(dbolter)
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.
Attachment #8848209 - Flags: review?(dbolter) → 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.

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.
Flags: needinfo?(tbsaunde+mozbugs)
Pushed by
fixup linked list manipulation in CoalesceMutationEvents() r=davidb
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Please request Aurora/Beta/ESR52 approval on this when you get a chance.
Assignee: nobody → tbsaunde+mozbugs
Flags: needinfo?(tbsaunde+mozbugs)
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 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
Flags: needinfo?(tbsaunde+mozbugs)
Attachment #8848209 - Flags: approval-mozilla-esr52?
Attachment #8848209 - Flags: approval-mozilla-beta?
Attachment #8848209 - Flags: approval-mozilla-aurora?
Comment on attachment 8848209 [details] [diff] [review]
fixup linked list manipulation in CoalesceMutationEvents()

Fix a crash. Aurora54+ & Beta53+.
Attachment #8848209 - Flags: approval-mozilla-beta?
Attachment #8848209 - Flags: approval-mozilla-beta+
Attachment #8848209 - Flags: approval-mozilla-aurora?
Attachment #8848209 - Flags: approval-mozilla-aurora+
Comment on attachment 8848209 [details] [diff] [review]
fixup linked list manipulation in CoalesceMutationEvents()

crash fix for esr52
Attachment #8848209 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
You need to log in before you can comment on or make changes to this bug.