1299939 - Crash in android.os.BadParcelableException: ClassNotFoundException when unmarshalling: com.facebook.intent.thirdparty.NativeThirdPartyUriHelper$FbrpcIntent at android.os.Parcel.readParcelableCreator(Parcel.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This bug was filed from the Socorro interface and is 
report bp-c0e57476-738e-4c19-93be-e4b532160826.
=============================================================

These crashes have increased substantially on **all release channels** in the past week.

I've seen them a number of times clicking links in the Facebook app (on beta).  I suspect they're the result of changes to the Facebook app on android.

android.os.BadParcelableException: ClassNotFoundException when unmarshalling: com.facebook.intent.thirdparty.NativeThirdPartyUriHelper$FbrpcIntent
	at android.os.Parcel.readParcelableCreator(Parcel.java:2535)
	at android.os.Parcel.readParcelable(Parcel.java:2461)
	at android.os.Parcel.readValue(Parcel.java:2364)
	at android.os.Parcel.readArrayMapInternal(Parcel.java:2717)
	at android.os.BaseBundle.unparcel(BaseBundle.java:269)
	at android.os.BaseBundle.containsKey(BaseBundle.java:341)
	at android.content.Intent.hasExtra(Intent.java:6023)
	at org.mozilla.gecko.LauncherActivity.onCreate(LauncherActivity.java:29)
	at android.app.Activity.performCreate(Activity.java:6664)
	at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1118)
	at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2599)
	at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2707)
	at android.app.ActivityThread.-wrap12(ActivityThread.java)
	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1460)
	at android.os.Handler.dispatchMessage(Handler.java:102)
	at android.os.Looper.loop(Looper.java:154)
	at android.app.ActivityThread.main(ActivityThread.java:6077)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:865)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:755)


See more data at:
https://crash-stats.mozilla.com/signature/?date=%3E2016-06-01&signature=android.os.BadParcelableException%3A%20ClassNotFoundException%20when%20unmarshalling%3A%20com.facebook.intent.thirdparty.NativeThirdPartyUriHelper%24FbrpcIntent%20at%20android.os.Parcel.readParcelableCreator%28Parcel.java%29&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_sort=-date&page=1


kbrosnan/rnewman, who should own this sort of thing these days?

Comment 1

[Richard Newman [:rnewman]]

Looks like LauncherActivity isn't using SafeIntent, which we created last time Facebook triggered this bug.

https://dxr.mozilla.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/SafeIntent.java

Sebastian, Grisha: this looks like a super easy fix if you want to try to get it into 49.

Comment 2

[Richard Newman [:rnewman]]

For history, see Bug 1147992.

Comment 3

[Richard Newman [:rnewman]]

I pinged the usual suspects at Facebook to see if they can 'fix' this (Bug 1147992 Comment 17).

Comment 4

[:Grisha Kruglov]

Attachment: Bug 1299939 - Use SafeIntent to handle incoming intents in the LauncherActivity

Review commit: https://reviewboard.mozilla.org/r/76184/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76184/

Comment 5

[:Grisha Kruglov]

Patch above addresses intent handling in the LauncherActivity. However, looking down the line, our intent handling in BrowserApp#onCreate is rather ad-hoc - sometimes we're paranoid, but often we're trusting. So this patch _might_ fix the problem, or it might just move it down the line in case we hit the "trusting" path in whatever gets dispatched...

GeckoApp seems to be better at this though.

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 8787435 [details]
Bug 1299939 - Use SafeIntent to handle incoming intents in the LauncherActivity

https://reviewboard.mozilla.org/r/76184/#review74266

::: mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java:32
(Diff revision 1)
>          super.onCreate(savedInstanceState);
>  
>          GeckoAppShell.ensureCrashHandling();
>  
> -        if (AppConstants.MOZ_ANDROID_CUSTOM_TABS && isCustomTabsIntent() && isCustomTabsEnabled()) {
> -            dispatchCustomTabsIntent();
> +        final SafeIntent safeIntent = new SafeIntent(getIntent());
> +        final boolean shouldSkipTabQueue = safeIntent.getBooleanExtra(

Move this inline; no need to do the work in the first two cases.

Comment 7

[Richard Newman [:rnewman]]

(In reply to :Grisha Kruglov from comment #5)
> So this patch _might_ fix the problem, or it might just move it down the line in case we
> hit the "trusting" path in whatever gets dispatched...

Patches welcome!

Comment 8

[:Grisha Kruglov]

Attachment: Bug 1299939 - Use SafeIntent throughout BrowserApp and delegates

Review commit: https://reviewboard.mozilla.org/r/76208/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76208/

Comment 9

[:Grisha Kruglov]

Comment on attachment 8787435 [details]
Bug 1299939 - Use SafeIntent to handle incoming intents in the LauncherActivity

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76184/diff/1-2/

Comment 10

[:Grisha Kruglov]

The above cleans up BrowserApp intent handling. Other two components that might be involved - TabQueueService and CustomTabsActivity - either explicitly deal with safe/unsafe intents (the former) or are tiny wrappers around GeckoApp (the latter).

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8787456 [details]
Bug 1299939 - Use SafeIntent throughout BrowserApp and delegates

https://reviewboard.mozilla.org/r/76208/#review74284

::: mobile/android/base/java/org/mozilla/gecko/GuestSession.java:61
(Diff revision 1)
>          final NotificationManager manager = (NotificationManager) context.getSystemService(Context.NOTIFICATION_SERVICE);
>          manager.cancel(R.id.guestNotification);
>      }
>  
> -    public static void handleIntent(BrowserApp context, Intent intent) {
> +    public static void handleIntent(BrowserApp context) {
>          context.showGuestModeDialog(BrowserApp.GuestModeDialog.LEAVING);

The naming of this method doesn't seem super accurate -- perhaps change it to `onNotificationIntentReceived`?

::: mobile/android/base/java/org/mozilla/gecko/feeds/ContentNotificationsDelegate.java:48
(Diff revision 1)
> -        if (intent != null && ACTION_CONTENT_NOTIFICATION.equals(intent.getAction())) {
> +        // Nothing to do.
> +        if (browserApp.getIntent() == null) {
> +            return;
> +        }
> +
> +        final SafeIntent intent = new SafeIntent(browserApp.getIntent());

I suppose for safety you should only call `getIntent()` once, then check it and use it.

Comment 12

[:Grisha Kruglov]

Comment on attachment 8787456 [details]
Bug 1299939 - Use SafeIntent throughout BrowserApp and delegates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76208/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dbf9f64add05
Use SafeIntent to handle incoming intents in the LauncherActivity r=rnewman
https://hg.mozilla.org/integration/autoland/rev/adc017f6ddf8
Use SafeIntent throughout BrowserApp and delegates r=rnewman

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dbf9f64add05
https://hg.mozilla.org/mozilla-central/rev/adc017f6ddf8

Comment 15

[:Grisha Kruglov]

Attachment: safe-intent.patch

Improves handling of incoming external intents, which might be poorly constructed (from Facebook, in this case). These two patches wrap all external intent handling in a "SafeIntent" class to prevent us from crashing and revert to sane defaults while processing incoming intents.

[Feature/regressing bug #]: N/A

[User impact if declined]: In case of the latest Facebook beta update, VIEW intents that it sends out causes Fennec to crash while dispatching them internally. Impact: user clicks on a link in Facebook, Fennec handles that action and crashes.

[Describe test coverage new/current, TreeHerder]: Manual testing; all existing tests pass.

[Risks and why]: Low; no functional changes, some very minor refactoring.

[String/UUID change made/needed]: N/A

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8788588 [details] [diff] [review]
safe-intent.patch

Crash fix, has stabilized on Nightly for a few days, Aurora50+

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Bug 1291811 moved SafeIntent.java into its current location, and that patch isn't present on aurora, so these patches don't apply. Could we get a rebased patch for where the files are supposed to be on aurora?

Comment 18

[:Grisha Kruglov]

Attachment: safe-intent-aurora.patch

This works for me locally on top of current aurora.

Comment 19

[:Grisha Kruglov]

(In reply to Wes Kocher (:KWierso) from comment #17)
> Bug 1291811 moved SafeIntent.java into its current location, and that patch
> isn't present on aurora, so these patches don't apply. Could we get a
> rebased patch for where the files are supposed to be on aurora?

Apologies about the bad patch, I forgot about Nick's changes!

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/46c6e8bb7f6f