1711633 - Crash in [@ pthread_mutex_lock | mozilla::layers::APZEventResult::APZEventResult]

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Gabriele Svelto [:gsvelto]]

Crash report: https://crash-stats.mozilla.org/report/index/68202038-babb-4e00-8e2e-a6aa10210515

Reason: EXC_BAD_ACCESS / KERN_INVALID_ADDRESS

Top 10 frames of crashing thread:

0 libsystem_pthread.dylib pthread_mutex_lock 
1 XUL mozilla::layers::APZEventResult::APZEventResult gfx/layers/apz/src/APZInputBridge.cpp:36
2 XUL mozilla::layers::InputQueue::ReceivePanGestureInput gfx/layers/apz/src/InputQueue.cpp:406
3 XUL mozilla::layers::InputQueue::ReceiveInputEvent gfx/layers/apz/src/InputQueue.cpp:53
4 XUL mozilla::layers::APZCTreeManager::ReceiveInputEvent gfx/layers/apz/src/APZCTreeManager.cpp:1677
5 XUL {virtual override thunk} 
6 XUL nsChildView::DispatchAPZWheelInputEvent widget/cocoa/nsChildView.mm:1983
7 XUL -[ChildView scrollWheel:] widget/cocoa/nsChildView.mm:3378
8 AppKit -[NSWindow _reallySendEvent:isDelayedEvent:] 
9 AppKit -[NSWindow sendEvent:] 

Low-volume crash but definitely valid. It seems like we're writing through a NULL pointer and bug 1678505 looks like the bug that potentially caused this regression. Hiro, can you have a look?

Comment 1

[Gabriele Svelto [:gsvelto]]

To elaborate on this it seems like we're trying to take a mutex that is NULL however I don't see mutexes being explicitly involved in this code. Maybe it's hidden in one of the classes that are involved?

Comment 2

[Jonathan Kew [:jfkthame]]

Maybe it's mRecursiveMutex in the AsyncPanZoomController? The call to aInitialTarget->IsRootContent() will want to lock that. Though I don't offhand see how it could be null; the RecursiveMutex constructor has MOZ_RELEASE_ASSERTs to check for failure to initialize the underlying pthread_mutex_t.

Comment 3

[Botond Ballo [:botond]]

In the section of APZCTreeManager::ReceiveInputEvent() which deals with PANGESTURE_INPUT, this call to InputQueue::ReceiveInputEvent is guarded by if (state.mHit.mTargetApzc), but this call (added in bug 1703705) is not.

The existence of the check around the first call suggests that state.mHit.mTargetApzc can sometimes be null. If we get into the second call site (which is what Line 4 of the stack trace in comment 0 suggests) with a null APZC, we'll get this crash.

Comment 4

[Botond Ballo [:botond]]

For the first call site, the behaviour if the target APZC is null is that the event is dropped, so it seems reasonable to add that as a condition for the second call site as well.

Comment 5

[Botond Ballo [:botond]]

Attachment: Bug 1711633 - Avoid calling InputQueue::ReceiveInputEvent() with a null APZC. r=hiro

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/edd0d0219eb3
Avoid calling InputQueue::ReceiveInputEvent() with a null APZC. r=hiro

Comment 7

[Iulian Moraru]

Backed out for causing linux toolchains bustage on APZCTreeManager.cpp.

Push with failures

Failure log

Backout link

Comment 8

[Botond Ballo [:botond]]

Ah, that's why the block condition was there redundantly, not having it there causes a warning-as-error with some compilers.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

I don't even remember now I really added it for the reason. :p

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/03f91efa03fa
Avoid calling InputQueue::ReceiveInputEvent() with a null APZC. r=hiro

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/03f91efa03fa

Comment 12

[Botond Ballo [:botond]]

I don't know if this has any chance of being approved for a late beta uplift, but we can give it a try.

Comment 13

[Botond Ballo [:botond]]

Comment on attachment 9223680 [details]
Bug 1711633 - Avoid calling InputQueue::ReceiveInputEvent() with a null APZC. r=hiro

Beta/Release Uplift Approval Request

• User impact if declined: Touchpad scrolling on Windows and Mac can sometimes result in the whole browser crashing (including in release builds).

We don't have STR for this crash, so it's hard to say what sort of page structure or interaction triggers it.

• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Low risk, the patch is just adding a null pointer check
• String changes made/needed: none

Comment 14

[Pascal Chevrel (relman team) -> :pascalc|PTO until Sept 2]

Comment on attachment 9223680 [details]
Bug 1711633 - Avoid calling InputQueue::ReceiveInputEvent() with a null APZC. r=hiro

We just shipped our RC2 and merge on Monday and ship 89 on Tuesday, our last beta was last week.