869019 - Regression: YouTube broken on Nightly - java.lang.StackOverflowError @ getOpenURIIntent(GeckoAppShell.java:1058)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

E/GeckoConsole(11822): [JavaScript Error: "NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIDOMLocation.href]" {file: "http://m.youtube.com/?reload=5&rdm=mmaekim6#/watch?feature=g-logo-xit&v=-nYSwuhsLa0" line: 173}]


W/System.err(11822): java.lang.StackOverflowError
W/System.err(11822): 	at android.net.Uri.<init>(Uri.java:50)
W/System.err(11822): 	at android.net.Uri$AbstractHierarchicalUri.<init>(Uri.java:1029)
W/System.err(11822): 	at android.net.Uri$AbstractHierarchicalUri.<init>(Uri.java:1029)
W/System.err(11822): 	at android.net.Uri$StringUri.<init>(Uri.java:465)
W/System.err(11822): 	at android.net.Uri$StringUri.<init>(Uri.java:457)
W/System.err(11822): 	at android.net.Uri.parse(Uri.java:429)
W/System.err(11822): 	at org.mozilla.gecko.GeckoAppShell.getOpenURIIntent(GeckoAppShell.java:1058)

......

STR: http://m.youtube.com, tap play on one of the top-level videos.

--
Nightly (05/06)
LG Nexus 4 (Android 4.2.2)

Comment 1

[Aaron Train [:aaronmt]]

Attachment: Raw log (Nightly 05/06)

Comment 2

[Aaron Train [:aaronmt]]

http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e478c0c1940f&tochange=120f3f229643

Comment 3

[Richard Newman [:rnewman]]

cpeterson broke this in Bug 847839 (landed in 22), with this chunk of (merge?) diff:

diff --git a/mobile/android/base/GeckoAppShell.java b/mobile/android/base/GeckoAppShell.java
--- a/mobile/android/base/GeckoAppShell.java
+++ b/mobile/android/base/GeckoAppShell.java
@@ -1055,17 +1055,26 @@ public class GeckoAppShell
             return intent;
         }

         if (!isUriSafeForScheme(uri)) {
             return null;
         }

         final String scheme = uri.getScheme();
-        final Intent intent = getIntentForActionString(action);
+
+        final Intent intent;
+        if ("vnd.youtube".equals(scheme) && getHandlersForURL(targetURI, action).length == 0) {
+            // Special case youtube to fallback to our own player
+            intent = new Intent(VideoPlayer.VIDEO_ACTION);
+            intent.setClassName(GeckoApp.mAppContext.getPackageName(),
+                                "org.mozilla.gecko.VideoPlayer");
+        } else {
+            intent = getIntentForActionString(action);
+        }

Comment 4

[Richard Newman [:rnewman]]

I think the fix here is to remove the recursive call, and replace it with a direct call to getHandlersForIntent with the fallback intent.

Comment 5

[Richard Newman [:rnewman]]

I'll have a patch up soon, I hope, but someone else might have to pick this up and finish it, because I'm not 100% sure what the correct behavior is supposed to be.

Comment 6

[Richard Newman [:rnewman]]

Attachment: Tentative patch. v1

Let's see how this is. (It's still building and untested, but...)

Comment 7

[Richard Newman [:rnewman]]

Attachment: Tentative patch. v2

Now with fewer syntax errors!

Comment 8

[Chris Peterson [:cpeterson]]

Comment on attachment 745902 [details] [diff] [review]
Tentative patch. v2

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

LGTM. Thanks for fixing my regression! <:)

Comment 9

[Richard Newman [:rnewman]]

Waiting for Fennec to stop crashing on my phone so I can test this…

Comment 10

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f95c172c51d

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4f95c172c51d

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

can we get a functionality test for getting an external helper application?