1492308 - PanZoomController.onTouchEvent() will crash if called too soon after a content crash

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[Randall Barker [:rbarker]]

When e10s is enabled PanZoomController.onTouchEvent() will crash if called too soon after a content crash. In FxR the following code restarts a crashed content process:

    @Override
    public void onCrash(GeckoSession session) {
       Log.e(LOGTAG,"Child crashed. Creating new session");
       session.open(mRuntime);
       session.loadUri(getHomeUri());
    }

How ever if when the session is reopened, if there are events going into the PanZoomController, it will crash with the following stack:

FATAL EXCEPTION: main
Process: org.mozilla.vrbrowser, PID: 8579
 java.lang.NullPointerException: NullHandle
    at org.mozilla.gecko.gfx.PanZoomController.handleMouseEvent(Native Method)
    at org.mozilla.gecko.gfx.PanZoomController.handleMouseEvent(PanZoomController.java:148)
    at org.mozilla.gecko.gfx.PanZoomController.onMotionEvent(PanZoomController.java:231)
    at org.mozilla.vrbrowser.ui.BrowserWidget.handleHoverEvent(BrowserWidget.java:158)
    at org.mozilla.vrbrowser.MotionEventGenerator.generateEvent(MotionEventGenerator.java:58)
    at org.mozilla.vrbrowser.MotionEventGenerator.dispatch(MotionEventGenerator.java:107)
    at org.mozilla.vrbrowser.VRBrowserActivity$10.run(VRBrowserActivity.java:361)
    at android.os.Handler.handleCallback(Handler.java:751)
    at android.os.Handler.dispatchMessage(Handler.java:95)
    at android.os.Looper.loop(Looper.java:154)
    at android.app.ActivityThread.main(ActivityThread.java:6121)
    at java.lang.reflect.Method.invoke(Native Method)
    at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:889)
    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:779)

The PanZoomController doesn't crash if it finishes initializing before it starts receiving events.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1492308 - 1. Generate natives binding for all JNIObject classes; r=snorp

Right now we skip generating natives binding for a class if the class
doesn't have native methods. However, we should still generate the
natives binding for JNIObject classes because these classes can still be
attached to C++ objects even without native methods.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1492308 - 2. Create new native handle before clearing old one; r=snorp

Create a new native handle first before deleting any old ones, so we can
be sure the new handle would not have the same value as the old handle,
which can confuse our code.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1492308 - 3. Add test for tapping after content crash; r=snorp

Add a test for the crash scenario where PZC is used after content
crashes and restarts.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1492308 - 4. Add new mechanism for safely disposing native objects; r=snorp

Add a new mechanism in NativePtr::Detach to safely dispose objects. A
new disposer Runnable is passed to OnDetach functions. The OnDetach
functions invoke the disposer after changing the object state. The
disposer then makes sure the object hasn't been attached to another
native object in the meantime. Disposal is only performed when the
current native object matches the original one.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1492308 - 5. Make various objects use the new disposal mechanism; r=snorp

Make LayerViewSupport, NPZCSupport, GeckoEditableSupport, and
SessionAccessibility use the new disposal mechanism to ensure the
disposal is performed safely.

Comment 6

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

Comment on attachment 9012648 [details]
Bug 1492308 - 1. Generate natives binding for all JNIObject classes; r=snorp

James Willcox (:snorp) ([e-mail]) has approved the revision.

Comment 7

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

Comment on attachment 9012649 [details]
Bug 1492308 - 2. Create new native handle before clearing old one; r=snorp

James Willcox (:snorp) ([e-mail]) has approved the revision.

Comment 8

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

Comment on attachment 9012650 [details]
Bug 1492308 - 3. Add test for tapping after content crash; r=snorp

James Willcox (:snorp) ([e-mail]) has approved the revision.

Comment 9

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

Comment on attachment 9012651 [details]
Bug 1492308 - 4. Add new mechanism for safely disposing native objects; r=snorp

James Willcox (:snorp) ([e-mail]) has approved the revision.

Comment 10

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

Comment on attachment 9012652 [details]
Bug 1492308 - 5. Make various objects use the new disposal mechanism; r=snorp

James Willcox (:snorp) ([e-mail]) has approved the revision.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f4cf0de3dfa
1. Generate natives binding for all JNIObject classes; r=snorp
https://hg.mozilla.org/integration/autoland/rev/8ab31240dbec
2. Create new native handle before clearing old one; r=snorp
https://hg.mozilla.org/integration/autoland/rev/7893e8aae295
3. Add test for tapping after content crash; r=snorp
https://hg.mozilla.org/integration/autoland/rev/17a45b4d6fd5
4. Add new mechanism for safely disposing native objects; r=snorp
https://hg.mozilla.org/integration/autoland/rev/005e374d721e
5. Make various objects use the new disposal mechanism; r=snorp

Comment 12

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/0f4cf0de3dfa
https://hg.mozilla.org/mozilla-central/rev/8ab31240dbec
https://hg.mozilla.org/mozilla-central/rev/7893e8aae295
https://hg.mozilla.org/mozilla-central/rev/17a45b4d6fd5
https://hg.mozilla.org/mozilla-central/rev/005e374d721e

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 9012648 [details]
Bug 1492308 - 1. Generate natives binding for all JNIObject classes; r=snorp

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: N/A

User impact if declined: If the content process of a GeckoView consumer (e.g. Focus) crashes, it may not be able to recover from the crash and the entire app may crash on further user interaction.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

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): The patches fix some race conditions in the way we clean up after a content crash; there should no functional changes.

String changes made/needed: None

Comment 14

[Chris Peterson [:cpeterson]]

62=wontfix

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Patch for Beta uplift

Patch for uplifting to Beta 63

Comment 16

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9014576 [details] [diff] [review]
Patch for Beta uplift

Crash fix for GV, uplift approved for 63 beta 13, thanks.

Comment 17

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9012648 [details]
Bug 1492308 - 1. Generate natives binding for all JNIObject classes; r=snorp

Removing the uplift request on the phabricator patch as an hg changeset was approved as a diff.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/00ea8935b012