Closed Bug 1090385 Opened 5 years ago Closed 5 years ago

More robust handling of external intents

Categories

(Firefox for Android :: General, defect, critical)

33 Branch
All
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 36
Tracking Status
firefox33 --- fixed
firefox34 --- fixed
firefox35 --- fixed
firefox36 --- fixed
fennec 33+ ---

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

(Keywords: crash)

Attachments

(1 file)

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)
This makes me sad, but it's probably the best approach for preventing future failures.
Attachment #8512942 - Flags: review?(snorp)
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Comment on attachment 8512942 [details] [diff] [review]
More robust handling of external intents. v1

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

Super sad.
Attachment #8512942 - Flags: review?(snorp) → review+
This should fix both Bug 1090279 and Bug 1090361, so flagging me for uplift.
Flags: needinfo?(rnewman)
https://hg.mozilla.org/mozilla-central/rev/99d578d3c8a1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
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.
Flags: needinfo?(rnewman) → needinfo?(lmandel)
Attachment #8512942 - Flags: approval-mozilla-release?
Attachment #8512942 - Flags: approval-mozilla-beta?
Attachment #8512942 - Flags: approval-mozilla-aurora?
Sylvestre, for your review. Richard wants to make the case to take this in 33.1.
Flags: needinfo?(lmandel) → needinfo?(sledru)
Richard, do you have some numbers on the number of crashes it is causing?
thanks
Flags: needinfo?(sledru) → needinfo?(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.
Flags: needinfo?(rnewman)
Comment on attachment 8512942 [details] [diff] [review]
More robust handling of external intents. v1

OK, taking it then :)
Attachment #8512942 - Flags: approval-mozilla-release?
Attachment #8512942 - Flags: approval-mozilla-release+
Attachment #8512942 - Flags: approval-mozilla-beta?
Attachment #8512942 - Flags: approval-mozilla-beta+
Attachment #8512942 - Flags: approval-mozilla-aurora?
Attachment #8512942 - Flags: approval-mozilla-aurora+
Richard, Ryan told me that the patch does not apply correctly on m-r. Could you rebase it? Thanks
Flags: needinfo?(rnewman)
tracking-fennec: ? → 33+
Flags: needinfo?(rnewman)
Rebased locally; doing a test build before I land.
Flags: qe-verify-
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
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.
Let's see if that sticks.
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
You need to log in before you can comment on or make changes to this bug.