1005409 - MOZ_ASSERT: mozilla::layers::Compositor::AssertOnCompositorThread () at gfx/layers/Compositor.cpp:46

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Gregor Wagner [:gwagner]]

Seen on current trunk b2g with debug gecko on nexus 4 during surfing on yelp.

0xb45b4142 in mozilla::layers::Compositor::AssertOnCompositorThread () at ../../../gfx/layers/Compositor.cpp:46
46	  MOZ_ASSERT(CompositorParent::CompositorLoop() ==
(gdb) bt
#0  0xb45b4142 in mozilla::layers::Compositor::AssertOnCompositorThread () at ../../../gfx/layers/Compositor.cpp:46
#1  0xb45be6c4 in mozilla::layers::AsyncPanZoomController::GetSharedFrameMetricsCompositor (this=0xa9528800) at ../../../gfx/layers/apz/src/AsyncPanZoomController.cpp:524
#2  0xb45c2454 in mozilla::layers::AsyncPanZoomController::~AsyncPanZoomController (this=0xa9528800, __in_chrg=<optimized out>) at ../../../gfx/layers/apz/src/AsyncPanZoomController.cpp:507
#3  0xb45af198 in mozilla::layers::AsyncPanZoomController::Release (this=0xa9528800) at ../../dist/include/mozilla/layers/AsyncPanZoomController.h:66
#4  0xb45c40d4 in ~nsRefPtr (this=0xae495544, __in_chrg=<optimized out>) at ../../dist/include/nsAutoPtr.h:894
#5  mozilla::layers::GestureEventListener::~GestureEventListener (this=0xae495540, __in_chrg=<optimized out>) at ../../../gfx/layers/apz/src/GestureEventListener.cpp:60
#6  0xb45be33c in mozilla::layers::GestureEventListener::Release (this=0xae495540) at ../../../gfx/layers/apz/src/GestureEventListener.h:41
#7  0xb45c40ec in ReleaseCallee (obj=<optimized out>) at ../../../ipc/chromium/src/base/task.h:263
#8  ReleaseCallee (this=0xac060740) at ../../../ipc/chromium/src/base/task.h:317
#9  RunnableMethod<mozilla::layers::GestureEventListener, void (mozilla::layers::GestureEventListener::*)(), Tuple0>::~RunnableMethod (this=0xac060740, __in_chrg=<optimized out>)
    at ../../../ipc/chromium/src/base/task.h:302
#10 0xb45c4108 in RunnableMethod<mozilla::layers::GestureEventListener, void (mozilla::layers::GestureEventListener::*)(), Tuple0>::~RunnableMethod (this=0xac060740, __in_chrg=<optimized out>)
    at ../../../ipc/chromium/src/base/task.h:303
#11 0xb430307c in MessageLoop::RunTask (this=0xb6a4e1a0, task=0xac060740) at ../../../ipc/chromium/src/base/message_loop.cc:358
#12 0xb43039b2 in MessageLoop::DeferOrRunPendingTask (this=<optimized out>, pending_task=<optimized out>) at ../../../ipc/chromium/src/base/message_loop.cc:365
#13 0xb4303cb6 in DoDelayedWork (next_delayed_work_time=0xb6a01e00, this=0xb6a4e1a0) at ../../../ipc/chromium/src/base/message_loop.cc:470
#14 MessageLoop::DoDelayedWork (this=0xb6a4e1a0, next_delayed_work_time=0xb6a01e00) at ../../../ipc/chromium/src/base/message_loop.cc:453
#15 0xb4315c82 in mozilla::ipc::MessagePump::DoDelayedWork (this=0xb6a01df0, aDelegate=<optimized out>) at ../../../ipc/glue/MessagePump.cpp:208
#16 0xb4315cbe in mozilla::ipc::DoWorkRunnable::Notify (this=0xb6a2ba20, aTimer=<optimized out>) at ../../../ipc/glue/MessagePump.cpp:240
#17 0xb4161bea in nsTimerImpl::Fire (this=0xafa8fa60) at ../../../xpcom/threads/nsTimerImpl.cpp:558
#18 0xb4161d68 in nsTimerEvent::Run (this=0xb00cd030) at ../../../xpcom/threads/nsTimerImpl.cpp:639
#19 0xb415f0ee in ProcessNextEvent (result=0xbe8e27f7, mayWait=true, this=0xb6a34700) at ../../../xpcom/threads/nsThread.cpp:715
#20 nsThread::ProcessNextEvent (this=0xb6a34700, mayWait=<optimized out>, result=0xbe8e27f7) at ../../../xpcom/threads/nsThread.cpp:639
#21 0xb411625c in NS_ProcessNextEvent (thread=0xb6a34700, mayWait=<optimized out>) at ../../../xpcom/glue/nsThreadUtils.cpp:263
#22 0xb43160a6 in mozilla::ipc::MessagePump::Run (this=0xb6a01df0, aDelegate=0xb6a4e1a0) at ../../../ipc/glue/MessagePump.cpp:136
#23 0xb43031ba in MessageLoop::RunInternal (this=0xb6a4e1a0) at ../../../ipc/chromium/src/base/message_loop.cc:229
#24 0xb43031d2 in RunHandler (this=0xb6a4e1a0) at ../../../ipc/chromium/src/base/message_loop.cc:222
#25 MessageLoop::Run (this=0xb6a4e1a0) at ../../../ipc/chromium/src/base/message_loop.cc:196
#26 0xb48b217a in nsBaseAppShell::Run (this=0xb2048460) at ../../../widget/xpwidgets/nsBaseAppShell.cpp:164
#27 0xb518babe in nsAppStartup::Run (this=0xb2047df0) at ../../../../toolkit/components/startup/nsAppStartup.cpp:278
#28 0xb5175302 in XREMain::XRE_mainRun (this=0xbe8e298c) at ../../../toolkit/xre/nsAppRunner.cpp:4019
#29 0xb5175502 in XREMain::XRE_main (this=0xbe8e298c, argc=<optimized out>, argv=<optimized out>, aAppData=<optimized out>) at ../../../toolkit/xre/nsAppRunner.cpp:4088
#30 0xb517565c in XRE_main (argc=1, argv=0xbe8e4b44, aAppData=0x23910, aFlags=<optimized out>) at ../../../toolkit/xre/nsAppRunner.cpp:4300
#31 0x0000a45c in do_main (argv=0xbe8e4b44, argc=1) at ../../../b2g/app/nsBrowserApp.cpp:163
#32 main (argc=<optimized out>, argv=<optimized out>) at ../../../b2g/app/nsBrowserApp.cpp:256

This happens on the main thread:
* 1    Thread 938.938    0xb45b4142 in mozilla::layers::Compositor::AssertOnCompositorThread () at ../../../gfx/layers/Compositor.cpp:46

Comment 1

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

As botond said on IRC:

15:05:21 botond: kats: seems what's happening is, when GEL posts one of its member functions as a task, the event loop takes a strong reference to it
15:05:45 botond: kats: the APZC is destroyed in UPZCT, its destructor nulls out its reference to the GEL
15:05:57 botond: kats: but the GEL still has that other reference by the event loop
15:06:06 botond: kats: and the GEL keeps the APZC alive
15:06:23 botond: kats: finally the event loop gets around to destroying the GEL -> the APZC is destroyed
15:06:36 botond: kats: but we assume APZC destructor happens on compositor thread -> problem

From a cursory code examination, there is another possible codepath where the APZC instances are destroyed on the main thread, and that is if the layer tree changes while you have your finger down, and then on touch up we clear the mOverscrollHandoffChain in APZCTreeManager. Clearing that list could effectively remove the last reference to some APZC instances, and cause them to be destroyed.

There may be other codepaths where this is true as well.

Comment 2

[Peter Chang[:pchang]]

I could reproduce this assertion easily with below steps.

Now I have a local patch to fix item b by adding a mutex to protect mAsyncPanZoomController inside GEL. During AsyncPanZoomController::Destroy, it will invoke GEL to set mAsyncPanZoomController as null first. This could fix item b.

But item c is another place that APZC destructor happened on main thread.

a. ContainerLayer
b. GestureEventLister(GEL) (main thread)
c. APZCTreeManager::UpdatePanZoomControllerTree
d. APZCTreeManager::GetTouchInputBlockAPZC (main thread)

Breakpoint 1, ~AsyncPanZoomController (this=0x44df7800, __in_chrg=<value optimized out>) at /tmp/ramdisk/b2g_central_new/gfx/layers/apz/src/AsyncPanZoomController.cpp:507
507	  printf_stderr("pchang AsyncPanZoomController::~AsyncPanZoomController called this %p\n", this);
#0  ~AsyncPanZoomController (this=0x44df7800, __in_chrg=<value optimized out>) at /tmp/ramdisk/b2g_central_new/gfx/layers/apz/src/AsyncPanZoomController.cpp:507
#1  0x4105cadc in mozilla::layers::AsyncPanZoomController::Release (this=0x44df7800) at ../../dist/include/mozilla/layers/AsyncPanZoomController.h:66
#2  0x4106758a in nsRefPtr<mozilla::layers::AsyncPanZoomController>::assign_assuming_AddRef (this=0x4035abc0, aEvent=..., aOutTargetGuid=0xbeb34c40) at ../../dist/include/nsAutoPtr.h:883
#3  operator=<mozilla::layers::AsyncPanZoomController> (this=0x4035abc0, aEvent=..., aOutTargetGuid=0xbeb34c40) at ../../dist/include/nsAutoPtr.h:983
#4  mozilla::layers::APZCTreeManager::ProcessTouchEvent (this=0x4035abc0, aEvent=..., aOutTargetGuid=0xbeb34c40) at /tmp/ramdisk/b2g_central_new/gfx/layers/apz/src/APZCTreeManager.cpp:475
^C


[STR]
a. Open setting
b. Go to the end of setting(Select Help)
c. Keep touching the screen randomly
d. Press home key at the same time
e. repeat step c

Comment 3

[Peter Chang[:pchang]]

(In reply to peter chang[:pchang][:peter] from comment #2)
> I could reproduce this assertion easily with below steps.
> 
> Now I have a local patch to fix item b by adding a mutex to protect
> mAsyncPanZoomController inside GEL. During AsyncPanZoomController::Destroy,
> it will invoke GEL to set mAsyncPanZoomController as null first. This could
> fix item b.
> 
> But item c is another place that APZC destructor happened on main thread.
> 
Forget to mention that these places contain/handle the nsRef of APZC.
[]
> a. ContainerLayer
> b. GestureEventLister(GEL) (main thread)
> c. APZCTreeManager::UpdatePanZoomControllerTree
> d. APZCTreeManager::GetTouchInputBlockAPZC (main thread)
> 
> Breakpoint 1, ~AsyncPanZoomController (this=0x44df7800, __in_chrg=<value
> optimized out>) at
> /tmp/ramdisk/b2g_central_new/gfx/layers/apz/src/AsyncPanZoomController.cpp:
> 507
> 507	  printf_stderr("pchang AsyncPanZoomController::~AsyncPanZoomController
> called this %p\n", this);
> #0  ~AsyncPanZoomController (this=0x44df7800, __in_chrg=<value optimized
> out>) at
> /tmp/ramdisk/b2g_central_new/gfx/layers/apz/src/AsyncPanZoomController.cpp:
> 507
> #1  0x4105cadc in mozilla::layers::AsyncPanZoomController::Release
> (this=0x44df7800) at
> ../../dist/include/mozilla/layers/AsyncPanZoomController.h:66
> #2  0x4106758a in
> nsRefPtr<mozilla::layers::AsyncPanZoomController>::assign_assuming_AddRef
> (this=0x4035abc0, aEvent=..., aOutTargetGuid=0xbeb34c40) at
> ../../dist/include/nsAutoPtr.h:883
> #3  operator=<mozilla::layers::AsyncPanZoomController> (this=0x4035abc0,
> aEvent=..., aOutTargetGuid=0xbeb34c40) at ../../dist/include/nsAutoPtr.h:983
> #4  mozilla::layers::APZCTreeManager::ProcessTouchEvent (this=0x4035abc0,
> aEvent=..., aOutTargetGuid=0xbeb34c40) at
> /tmp/ramdisk/b2g_central_new/gfx/layers/apz/src/APZCTreeManager.cpp:475
> ^C
> 
> 
> [STR]
> a. Open setting
> b. Go to the end of setting(Select Help)
> c. Keep touching the screen randomly
> d. Press home key at the same time
> e. repeat step c

Comment 4

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

Item c runs on the compositor thread, not the main thread.

Comment 5

[Botond Ballo [:botond]]

(In reply to peter chang[:pchang][:peter] from comment #2)
> Breakpoint 1, ~AsyncPanZoomController (this=0x44df7800, __in_chrg=<value
> optimized out>) at
> /tmp/ramdisk/b2g_central_new/gfx/layers/apz/src/AsyncPanZoomController.cpp:
> 507
> 507	  printf_stderr("pchang AsyncPanZoomController::~AsyncPanZoomController
> called this %p\n", this);
> #0  ~AsyncPanZoomController (this=0x44df7800, __in_chrg=<value optimized
> out>) at
> /tmp/ramdisk/b2g_central_new/gfx/layers/apz/src/AsyncPanZoomController.cpp:
> 507
> #1  0x4105cadc in mozilla::layers::AsyncPanZoomController::Release
> (this=0x44df7800) at
> ../../dist/include/mozilla/layers/AsyncPanZoomController.h:66
> #2  0x4106758a in
> nsRefPtr<mozilla::layers::AsyncPanZoomController>::assign_assuming_AddRef
> (this=0x4035abc0, aEvent=..., aOutTargetGuid=0xbeb34c40) at
> ../../dist/include/nsAutoPtr.h:883
> #3  operator=<mozilla::layers::AsyncPanZoomController> (this=0x4035abc0,
> aEvent=..., aOutTargetGuid=0xbeb34c40) at ../../dist/include/nsAutoPtr.h:983
> #4  mozilla::layers::APZCTreeManager::ProcessTouchEvent (this=0x4035abc0,
> aEvent=..., aOutTargetGuid=0xbeb34c40) at
> /tmp/ramdisk/b2g_central_new/gfx/layers/apz/src/APZCTreeManager.cpp:475

So here, we have mApzcForInputBlock being assigned to. If the old value was the last reference to an APZC, that APZC is destroyed.

I think we can work around this by checking, when an APZC is destroyed, if it is the same as mApzcForInpuBlock, and if so, nulling out mApzcForInputBlock. I think this is OK because an APZC whose layer no longer exists does not need to continue receiving touch events.

Comment 6

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

I think a more comprehensive solution is to just move the code from the AsyncPanZoomController destructor the Destroy function, which is always called on the compositor thread.

Comment 7

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

*to* the Destroy function

Comment 8

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

Attachment: Patch

Move stuff into Destroy() instead of running it in the destructor.

Comment 9

[Botond Ballo [:botond]]

Comment on attachment 8421100 [details] [diff] [review]
Patch

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

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +554,5 @@
> +  if (compositor && mSharedFrameMetricsBuffer) {
> +    unused << compositor->SendReleaseSharedCompositorFrameMetrics(mFrameMetrics.GetScrollId(), mAPZCId);
> +  }
> +
> +  { // scope the lock

Why are we scoping the lock if we are at the end of the function?

@@ +556,5 @@
> +  }
> +
> +  { // scope the lock
> +    ReentrantMonitorAutoEnter lock(mMonitor);
> +    if (mSharedFrameMetricsBuffer) {

It is safe to 'delete' a null pointer, so there is no need to have these checks.

Comment 10

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

(In reply to Botond Ballo [:botond] from comment #9)
> > +  { // scope the lock
> 
> Why are we scoping the lock if we are at the end of the function?
> 

Mostly just for safety, in case things get added afterwards.

> 
> It is safe to 'delete' a null pointer, so there is no need to have these
> checks.

True, I'll remove those checks.

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7383139dde90

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7383139dde90