Closed Bug 1711633 Opened 4 months ago Closed 4 months ago

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


(Core :: Panning and Zooming, defect)




90 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox88 --- unaffected
firefox89 --- wontfix
firefox90 --- fixed


(Reporter: gsvelto, Assigned: botond)




(Keywords: crash, regression)

Crash Data


(1 file)

Crash report:


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/
7 XUL -[ChildView scrollWheel:] widget/cocoa/
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?

Severity: -- → S2
Severity: S2 → --

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?

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.

Component: Layout: Scrolling and Overflow → Panning and Zooming

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.

Regressed by: 1703705

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.

Assignee: nobody → botond
Pushed by
Avoid calling InputQueue::ReceiveInputEvent() with a null APZC. r=hiro

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

Push with failures

Failure log

Backout link

Flags: needinfo?(botond)

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

Flags: needinfo?(botond)

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

Pushed by
Avoid calling InputQueue::ReceiveInputEvent() with a null APZC. r=hiro
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch

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 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
Attachment #9223680 - Flags: approval-mozilla-beta?

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.

Attachment #9223680 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.