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

RESOLVED FIXED in Firefox 50

Status

()

defect
P1
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dbaron, Assigned: Grisha)

Tracking

({crash, topcrash})

unspecified
Firefox 51
Unspecified
Android
Points:
---

Firefox Tracking Flags

(firefox48 wontfix, firefox49 wontfix, fennec50+, firefox50 fixed, firefox51 fixed)

Details

(Whiteboard: [MobileAS], crash signature)

Attachments

(4 attachments)

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?
Flags: needinfo?(rnewman)
Flags: needinfo?(kbrosnan)
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.
tracking-fennec: --- → ?
Flags: needinfo?(s.kaspari)
Flags: needinfo?(rnewman)
Flags: needinfo?(kbrosnan)
Flags: needinfo?(gkruglov)
For history, see Bug 1147992.
See Also: → 1147992
See Also: → 1077645
I pinged the usual suspects at Facebook to see if they can 'fix' this (Bug 1147992 Comment 17).
Comment hidden (mozreview-request)
Assignee

Comment 5

3 years ago
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.
Flags: needinfo?(gkruglov)

Comment 6

3 years ago
mozreview-review
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.
Attachment #8787435 - Flags: review+
(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!
Assignee: nobody → gkruglov
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 10

3 years ago
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).
Assignee

Updated

3 years ago
Priority: -- → P1
Whiteboard: [MobileAS]

Comment 11

3 years ago
mozreview-review
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.
Attachment #8787456 - Flags: review?(rnewman) → review+
Comment hidden (mozreview-request)

Comment 13

3 years ago
Pushed by gkruglov@mozilla.com:
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
Flags: needinfo?(s.kaspari)
Attachment #8787435 - Flags: review?(s.kaspari)

Comment 14

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dbf9f64add05
https://hg.mozilla.org/mozilla-central/rev/adc017f6ddf8
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Assignee

Comment 15

3 years ago
Posted patch safe-intent.patch — — Splinter Review
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
Attachment #8788588 - Flags: approval-mozilla-aurora?
tracking-fennec: ? → 50+
Comment on attachment 8788588 [details] [diff] [review]
safe-intent.patch

Crash fix, has stabilized on Nightly for a few days, Aurora50+
Attachment #8788588 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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?
Flags: needinfo?(gkruglov)
Assignee

Comment 18

3 years ago
This works for me locally on top of current aurora.
Flags: needinfo?(gkruglov)
Assignee

Comment 19

3 years ago
(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!
Iteration: --- → 1.3
You need to log in before you can comment on or make changes to this bug.