1306716 - [e10s] Crash when tapping a link

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

I get the following crash in the parent process when tapping a link with a e10s-enabled GeckoView.

#0  NS_CycleCollectorSuspect3 (aPtr=0x587a6e70, aCp=0x0, aRefCnt=0x587a6e74, aShouldDelete=0x0) at /Volumes/Source/source/gecko-apz/xpcom/base/nsCycleCollector.cpp:3998
#1  0x53bff676 in incr (aCp=0x0, aOwner=0x587a6e70, this=0x587a6e74) at /Volumes/Source/source/objdirs/objdir-android-opt/dist/include/nsISupportsImpl.h:190
#2  incr (aOwner=0x587a6e70, this=0x587a6e74) at /Volumes/Source/source/objdirs/objdir-android-opt/dist/include/nsISupportsImpl.h:176
#3  nsXULElementTearoff::AddRef (this=0x587a6e70) at /Volumes/Source/source/gecko-apz/dom/xul/nsXULElement.cpp:147
#4  0x53c09fae in nsXULElement::QueryInterface (this=0x594a1290, aIID=..., aInstancePtr=0xbe9ff4c4) at /Volumes/Source/source/gecko-apz/dom/xul/nsXULElement.cpp:349
#5  0x52e1f0a6 in nsCOMPtr_base::assign_from_qi (this=0xbe9ff4d4, aQI=..., aIID=...) at /Volumes/Source/source/gecko-apz/xpcom/glue/nsCOMPtr.cpp:51
#6  0x53b7c4c0 in nsCOMPtr (aQI=..., this=0xbe9ff4d4) at /Volumes/Source/source/objdirs/objdir-android-opt/dist/include/nsCOMPtr.h:512
#7  mozilla::dom::TabParent::GetFrameLoader (this=this@entry=0x5a90f400, aUseCachedFrameLoaderAfterDestroy=aUseCachedFrameLoaderAfterDestroy@entry=false) at /Volumes/Source/source/gecko-apz/dom/ipc/TabParent.cpp:2767
#8  0x53b7c676 in mozilla::dom::TabParent::GetChildProcessOffset (this=this@entry=0x5a90f400) at /Volumes/Source/source/gecko-apz/dom/ipc/TabParent.cpp:2141
#9  0x53b7ce0c in mozilla::dom::TabParent::SendHandleTap (this=0x5a90f400, aType=mozilla::layers::GeckoContentController::eSingleTap, aPoint=..., aModifiers=0, aGuid=..., aInputBlockId=1) at /Volumes/Source/source/gecko-apz/dom/ipc/TabParent.cpp:1699
#10 0x533ec3b8 in mozilla::layers::RemoteContentController::HandleTap (this=<optimized out>, aTapType=mozilla::layers::GeckoContentController::eSingleTap, aPoint=..., aModifiers=0, aGuid=..., aInputBlockId=1) at /Volumes/Source/source/gecko-apz/gfx/layers/ipc/RemoteContentController.cpp:84
#11 0x533aff00 in applyImpl<mozilla::layers::GeckoContentController, void (mozilla::layers::GeckoContentController::*)(mozilla::layers::GeckoContentController::TapType, mozilla::gfx::PointTyped<mozilla::LayoutDevicePixel> const&, unsigned short, mozilla::layers::ScrollableLayerGuid const&, unsigned long long), StoreCopyPassByValue<mozilla::layers::GeckoContentController::TapType>, StoreCopyPassByValue<mozilla::gfx::PointTyped<mozilla::LayoutDevicePixel, float> >, StoreCopyPassByValue<unsigned short>, StoreCopyPassByValue<mozilla::layers::ScrollableLayerGuid>, StoreCopyPassByValue<unsigned long long>, 0u, 1u, 2u, 3u, 4u> (args=..., 
    m=<optimized out>, o=0x580140b0) at /Volumes/Source/source/objdirs/objdir-android-opt/dist/include/nsThreadUtils.h:729
#12 apply<mozilla::layers::GeckoContentController, void (mozilla::layers::GeckoContentController::*)(mozilla::layers::GeckoContentController::TapType, mozilla::gfx::PointTyped<mozilla::LayoutDevicePixel> const&, unsigned short, mozilla::layers::ScrollableLayerGuid const&, unsigned long long)> (m=<optimized out>, 
    o=0x580140b0, this=<optimized out>) at /Volumes/Source/source/objdirs/objdir-android-opt/dist/include/nsThreadUtils.h:736
#13 mozilla::detail::RunnableMethodImpl<void (mozilla::layers::GeckoContentController::*)(mozilla::layers::GeckoContentController::TapType, mozilla::gfx::PointTyped<mozilla::LayoutDevicePixel, float> const&, unsigned short, mozilla::layers::ScrollableLayerGuid const&, unsigned long long), true, false, mozilla::layers::GeckoContentController::TapType, mozilla::gfx::PointTyped<mozilla::LayoutDevicePixel, float>, unsigned short, mozilla::layers::ScrollableLayerGuid, unsigned long long>::Run (this=<optimized out>) at /Volumes/Source/source/objdirs/objdir-android-opt/dist/include/nsThreadUtils.h:764
#14 0x53c6eff4 in mozilla::AndroidBridge::RunDelayedUiThreadTasks (this=0x582f01f0) at /Volumes/Source/source/gecko-apz/widget/android/AndroidBridge.cpp:1430
#15 0x4085bc50 in ?? ()
#16 0x4085bc50 in ?? ()

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

This seems to be caused because we don't have a cycle collector on the UI (APZ controller) thread.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Call TabParent::SendHandleTap from the right thread on Android

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Call TabParent::SendHandleTap from the right thread on Android

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/53291cfac404
Call TabParent::SendHandleTap from the right thread on Android r=kats

Comment 5

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/53291cfac404

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Don't retrieve (and ref) the TabParent on a non-main thread

This was still a bit busted because we can't ref TabParent on a non-main thread

Comment 7

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

Comment on attachment 8797629 [details] [diff] [review]
Don't retrieve (and ref) the TabParent on a non-main thread

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

Would like to see a new version with the comments addressed.

::: gfx/layers/ipc/RemoteContentController.cpp
@@ +59,5 @@
>  
>    // If we get here we're probably in the parent process, but we might be in
>    // the GPU process in some shutdown phase where the LayerTreeState or
>    // APZCTreeManagerParent structures are torn down. In that case we'll just get
>    // a null TabParent.

Drop this comment

@@ +68,1 @@
>      MOZ_ASSERT(XRE_IsParentProcess());

Move this MOZ_ASSERT into HandleTap before the |if (NS_IsMainThread)| condition.

@@ +92,1 @@
>      }

Move this return out of the |if (apzctmp)| block so that it is hit unconditionally at the end of the |if XRE_GetProcessType| block.

@@ +94,5 @@
> +
> +  if (NS_IsMainThread()) {
> +    HandleTapOnMainThread(aTapType, aPoint, aModifiers, aGuid, aInputBlockId);
> +  } else {
> +    // We don't want to get the TabParent or call TabParent::SendHandleTap() from a non-main thread

Add "(this might happen on Android, where this is called on the Java UI thread)" to this comment.

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Don't retrieve (and ref) the TabParent on a non-main thread

Comment 9

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

Comment on attachment 8797725 [details] [diff] [review]
Don't retrieve (and ref) the TabParent on a non-main thread

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

r+ with comments addressed (sorry I missed these in the previous review).

::: gfx/layers/ipc/RemoteContentController.cpp
@@ +59,5 @@
> +  if (tab) {
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +    // If we got a TabParent we're definitely in the parent process, and the
> +    // message is going to a child process. Also, we're guaranteed to be on the main thread.

Drop this comment as well, and move the assert up to the start of the function.

::: gfx/layers/ipc/RemoteContentController.h
@@ +89,5 @@
> +  void HandleTapOnMainThread(TapType aType,
> +                               const LayoutDevicePoint& aPoint,
> +                               Modifiers aModifiers,
> +                               const ScrollableLayerGuid& aGuid,
> +                               uint64_t aInputBlockId);

nit: alignment

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32c669981d22
Don't retrieve (and ref) the TabParent on a non-main thread r=kats

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Follow up to not pass references across threads (!)

Sigh.

Comment 12

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

Comment on attachment 8797770 [details] [diff] [review]
Follow up to not pass references across threads (!)

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

::: gfx/layers/ipc/RemoteContentController.cpp
@@ +54,2 @@
>                                                 Modifiers aModifiers,
> +                                               const ScrollableLayerGuid aGuid,

drop the const as well, throughout this patch

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/32c669981d22

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/809141755973
Follow up to not pass references across threads (!) r=kats

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/809141755973