1090385 - More robust handling of external intents

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

Bug 1090361, Bug 1090279, Bug 1077645 all demonstrate that we can't trust external intents to not crash us.

Let's try to isolate intent access so that we can validate and protect the rest of the app.

at android.os.Bundle.unparcel(Bundle.java:223)
at android.os.Bundle.getBundle(Bundle.java:1151)
at android.content.Intent.getBundleExtra(Intent.java:5034)
at org.mozilla.gecko.GeckoApp.onCreate(GeckoApp.java:1220)

Comment 1

[Richard Newman [:rnewman]]

Attachment: More robust handling of external intents. v1

This makes me sad, but it's probably the best approach for preventing future failures.

Comment 2

[Richard Newman [:rnewman]]

https://tbpl.mozilla.org/?tree=Try&rev=80b3702f69d3

Comment 3

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

Comment on attachment 8512942 [details] [diff] [review]
More robust handling of external intents. v1

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

Super sad.

Comment 4

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/99d578d3c8a1

Comment 5

[Richard Newman [:rnewman]]

This should fix both Bug 1090279 and Bug 1090361, so flagging me for uplift.

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/99d578d3c8a1

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 8512942 [details] [diff] [review]
More robust handling of external intents. v1

Approval Request Comment
[Feature/regressing bug #]:
  Fixes Bug 1090279 and Bug 1090361, where external intents (from Facebook and others) can cause us to crash.

[User impact if declined]:
  Crashes attributed to Firefox.

[Describe test coverage new/current, TBPL]:
  Exercised by test infrastructure in that it fires intents. Otherwise just manual QA. Our usual testing doesn't test the edge cases that are problematic, unfortunately.

[Risks and why]: 
  Although this looks like a non-trivial refactoring, it's really just a less messy way of wrapping most of our intent accesses in a try-catch block.

[String/UUID change made/needed]:
  None.

Comment 8

[Lawrence Mandel [:lmandel] (use needinfo)]

Sylvestre, for your review. Richard wants to make the case to take this in 33.1.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Richard, do you have some numbers on the number of crashes it is causing?
thanks

Comment 10

[Richard Newman [:rnewman]]

I see 66 reports this week for Bug 1090279, 383 this week for Bug 1090361, both on release.

The latter was the #1 crasher when Aaron filed it two days ago.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8512942 [details] [diff] [review]
More robust handling of external intents. v1

OK, taking it then :)

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Richard, Ryan told me that the patch does not apply correctly on m-r. Could you rebase it? Thanks

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f201b7e443ea

Richard is going to have to take care of the Beta/Release uplifts.

Comment 14

[Richard Newman [:rnewman]]

Rebased locally; doing a test build before I land.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Aurora was busted too. Backed out.
https://hg.mozilla.org/releases/mozilla-aurora/rev/272541036ab7

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=358564&repo=mozilla-aurora

Comment 16

[Richard Newman [:rnewman]]

I landed Beta and Release before seeing your comment, 'cos skia stuff refused to link in my test build. I suspect that they'll work, but if not, hit with the backout stick, please!

https://hg.mozilla.org/releases/mozilla-beta/rev/65515de095b8
https://hg.mozilla.org/releases/mozilla-release/rev/2c6590150a85

Comment 17

[Richard Newman [:rnewman]]

One-liner fix to the Aurora patch:

diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
--- a/mobile/android/base/GeckoApp.java
+++ b/mobile/android/base/GeckoApp.java
@@ -1873,17 +1873,17 @@ public abstract class GeckoApp
         }

         String uri = intent.getDataString();
         if (uri != null) {
             return uri;
         }

         if ((action != null && action.startsWith(ACTION_WEBAPP_PREFIX)) || ACTION_HOMESCREEN_SHORTCUT.equals(action)) {
-            uri = StringUtils.getStringExtra(intent, "args");
+            uri = intent.getStringExtra("args");
             if (uri != null && uri.startsWith("--url=")) {
                 uri.replace("--url=", "");
             }
         }
         return uri;
     }

     protected int getOrientation() {


Testing locally.

Comment 18

[Richard Newman [:rnewman]]

https://hg.mozilla.org/mozilla-aurora/rev/3919b426956c

Comment 19

[Richard Newman [:rnewman]]

Let's see if that sticks.

Comment 20

[Richard Newman [:rnewman]]

Follow-up for release bustage:

https://hg.mozilla.org/releases/mozilla-release/rev/f8bdafd5fac5

Comment 21

[Richard Newman [:rnewman]]

Bustage-tastic.

I don't know why this patch seems to have basically not applied at all.

https://hg.mozilla.org/releases/mozilla-release/rev/13b6fd9ab7a1
https://hg.mozilla.org/releases/mozilla-beta/rev/693b7d0c9b36

Comment 22

[Richard Newman [:rnewman]]

This time with a workaround to allow for building locally!

https://hg.mozilla.org/releases/mozilla-release/rev/1316d20c2270
https://hg.mozilla.org/releases/mozilla-beta/rev/72bdce765298

JFC.