1153979 - Audit locking around AsyncPanZoomController::SetState()

Status: NEW

(Core :: Panning and Zooming, defect, P3)

Description

[Botond Ballo [:botond] [away until Jun 24]]

AsyncPanZoomController::SetState() has a comment ("Intentional scoping for mutex") that suggests that we don't want to be holding mMonitor during the call to DispatchStateChangeNotification(), which in turns suggests that we shouldn't be holding mMonitor already coming into SetState().

Digging a bit, I found the following justification for not holding the lock during DispatchStateChangeNotification() [2] :

  "I'm not a fan of running these callbacks while holding the mMonitor 
   lock, because it could basically do arbitrary things and hold the 
   lock for a long time, which will interfere with composition and 
   other critical things. Can we bump this to a callback or something?"

(The "callbacks" refer to the GeckoCC functions (since refactored to be just one function) that DispatchStateChangeNotification calls.)

However, not all APZ code refrains from already holding the monitor when SetState is called. A cursory check turns out at least one violating call site (SnapBackIfOverscrolled -> StartOverscrollAnimation -> SetState), and a new one was just added in bug 1152011 in OnScrollWheel.

We should decide whether we should require callers of SetState to *not* hold the lock, and if so, clean up call sites and assert to that effect.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=86b60ef17965#3076
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=869940#c12

Comment 1

[Botond Ballo [:botond] [away until Jun 24]]

Kats, is the concern about not holding the lock while calling the GeckoCC functions still relevant/important?

Comment 2

[Kartikaya Gupta (email:[e-mail])]

I would say yes, it's still relevant. I could probably be convinced otherwise though.