1077402 - [B2G][Flame] Compositor monitor assertion failure (AssertCurrentThreadInMonitor)

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Andrew Osmond [:aosmond] (he/him)]

Crash in the compositor encountered on b2g-inbound debug builds for flame:

(gdb) bt
#0  tgkill () at bionic/libc/arch-arm/bionic/tgkill.S:46
#1  0xb6f0b13c in pthread_kill (t=<optimized out>, sig=6) at bionic/libc/bionic/pthread_kill.cpp:49
#2  0xb6f0b350 in raise (sig=6) at bionic/libc/bionic/raise.cpp:32
#3  0xb6f0a086 in __libc_android_abort () at bionic/libc/bionic/abort.cpp:55
#4  0xb6f199a0 in abort () at bionic/libc/arch-arm/bionic/abort_arm.S:41
#5  0xb6a6c9f4 in PR_Assert (s=0xb6aeab50 "mon->entryCount != 0 && pthread_equal(mon->owner, pthread_self())", file=<optimized out>, ln=ln@entry=572) at ../../../../../../b2g-inbound/nsprpub/pr/src/io/prlog.c:554
#6  0xb6a7f05a in PR_AssertCurrentThreadInMonitor (mon=0xad64d820) at ../../../../../../b2g-inbound/nsprpub/pr/src/pthreads/ptsynch.c:571
#7  0xb49497a0 in AssertCurrentThreadIn (this=0xafd7d180) at ../../dist/include/mozilla/ReentrantMonitor.h:129
#8  mozilla::layers::AsyncPanZoomController::GetFrameMetrics (this=0xafd7d000) at ../../../../b2g-inbound/gfx/layers/apz/src/AsyncPanZoomController.cpp:2812
#9  0xb4949a88 in mozilla::layers::Axis::GetFrameMetrics (this=this@entry=0xafd7d450) at ../../../../b2g-inbound/gfx/layers/apz/src/Axis.cpp:369
#10 0xb4949acc in mozilla::layers::Axis::GetCompositionLength (this=this@entry=0xafd7d450) at ../../../../b2g-inbound/gfx/layers/apz/src/Axis.cpp:345
#11 0xb4949bde in mozilla::layers::AsyncPanZoomController::GetOverscrollTransform (this=0xafd7d000) at ../../../../b2g-inbound/gfx/layers/apz/src/AsyncPanZoomController.cpp:2474
#12 0xb4966954 in mozilla::layers::ApplyAsyncTransformToScrollbarForContent (aScrollbar=0xaf800d10, aContent=..., aScrollbarIsDescendant=<optimized out>) at ../../../../b2g-inbound/gfx/layers/composite/AsyncCompositionManager.cpp:739
#13 0xb4966b24 in mozilla::layers::AsyncCompositionManager::AlignFixedAndStickyLayers (this=0xafd7d450, aLayer=0xafd7d000, aTransformedSubtreeRoot=0xaf800ca0, aTransformScrollId=12517308902844220416, aPreviousTransformForRoot=..., 
    aCurrentTransformForRoot=..., aFixedLayerMargins=...) at ../../../../b2g-inbound/gfx/layers/composite/AsyncCompositionManager.cpp:261

I reproduced within 1 minute after boot every time and can provide a core file if further analysis is required.

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

Attachment: bug1077402.patch

Add monitor locking for accessing protected properties.

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

FYI, this seems to have been introduced by bug 1043859.

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8499536 [details] [diff] [review]
bug1077402.patch

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

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +2473,5 @@
> +    ReentrantMonitorAutoEnter lock(mMonitor);
> +    compositionSize.SizeTo(mX.GetCompositionLength(), mY.GetCompositionLength());
> +  }
> +  float scaleX = 1 + kStretchFactor * fabsf(mX.GetOverscroll()) / compositionSize.width;
> +  float scaleY = 1 + kStretchFactor * fabsf(mY.GetOverscroll()) / compositionSize.height;

The only thing I was not certain on was whether GetOverscroll should be protected as well. It does not have an assert in its accessor but that may just be incidental :).

Comment 4

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

Comment on attachment 8499536 [details] [diff] [review]
bug1077402.patch

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

Thanks for the patch! I think it would be better to just stick lock at the top of the function so everything in it is protected. Functions like Axis::IsOverscrolled and Axis::GetOverscroll should be protected as well. r=me with that.

Comment 5

[Andrew Osmond [:aosmond] (he/him)]

Attachment: bug1077402.patch, v2

Moved lock to the top of function as per review.

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d50a11c43f96

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Wait until the Try run is actually finished before setting the flags please.

Comment 8

[Andrew Osmond [:aosmond] (he/him)]

My bad, sorry. Done now!

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/76e64cc73ccf

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/76e64cc73ccf

Comment 12

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

Comment on attachment 8499553 [details] [diff] [review]
bug1077402.patch, v2

Approval Request Comment
[Feature/regressing bug #]: bug 1043859, which is pending aurora approval
[User impact if declined]: debug builds will assert and crash when going into overscroll
[Describe test coverage new/current, TBPL]: verified on m-c by local testing
[Risks and why]: low-risk, only needs uplift if 1043859 is uplifted.
[String/UUID change made/needed]: none

Comment 13

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

I'll uplift this along with bug 1043859.

Comment 14

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

Try push for the rebased patches: https://tbpl.mozilla.org/?tree=Try&rev=4b007739646c

Landed on aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/34dd5b90c0d9