Last Comment Bug 766914 - Unable to play videos on mobile YouTube; redirection loop on 'Play'
: Unable to play videos on mobile YouTube; redirection loop on 'Play'
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: 16 Branch
: ARM Android
: -- major (vote)
: ---
Assigned To: Mark Finkle (:mfinkle) (use needinfo?)
:
Mentors:
http://www.youtube.com
: 766855 767257 (view as bug list)
Depends on: 786623
Blocks: youtube.com
  Show dependency treegraph
 
Reported: 2012-06-21 04:48 PDT by Catalin Suciu [:csuciu]
Modified: 2012-08-31 09:27 PDT (History)
26 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
fixed
verified
betaN+


Attachments
Log file (77.10 KB, text/plain)
2012-06-21 04:48 PDT, Catalin Suciu [:csuciu]
no flags Details
patch (3.48 KB, patch)
2012-06-21 08:02 PDT, Mark Finkle (:mfinkle) (use needinfo?)
mbrubeck: review+
blassey.bugs: review+
Details | Diff | Splinter Review
patch to send XUL Fennec UA to yotuube (1.49 KB, patch)
2012-06-21 08:05 PDT, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
patch 2 (4.75 KB, patch)
2012-06-21 10:32 PDT, Mark Finkle (:mfinkle) (use needinfo?)
mbrubeck: review+
blassey.bugs: approval‑mozilla‑aurora+
blassey.bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review
patch for checkin (4.75 KB, patch)
2012-06-21 10:42 PDT, Mark Finkle (:mfinkle) (use needinfo?)
mark.finkle: review+
Details | Diff | Splinter Review
alternate patch that sends desktop UA (1.93 KB, patch)
2012-06-21 12:05 PDT, Mark Finkle (:mfinkle) (use needinfo?)
bnicholson: feedback+
Details | Diff | Splinter Review

Description Catalin Suciu [:csuciu] 2012-06-21 04:48:35 PDT
Created attachment 635258 [details]
Log file

Nightly 16.0a1 (2012-06-21)

Steps:
1. Go to m.youtube.com
2. Tap on one of the video
3. Tap on play button for the selected video

Expected
Video starts to play

Actual:
Page is refreshed

Log attached
Comment 1 Aaron Train [:aaronmt] 2012-06-21 05:02:18 PDT
So YouTube's changes worked late afternoon yesterday. On 'play' one would be redirected to their desktop site. It looks like they changed something and it's in a user-agent redirection loop.
Comment 2 Lawrence Mandel [:lmandel] (use needinfo) 2012-06-21 05:36:21 PDT
As commented in an e-mail thread, spoofing the iPhone UA gets us to the point where we have an unplayable video. There is a different path (user-agent redirection loop as Aaron points out above) for Fennec.
Comment 3 Lawrence Mandel [:lmandel] (use needinfo) 2012-06-21 06:34:00 PDT
*** Bug 766855 has been marked as a duplicate of this bug. ***
Comment 4 Brad Lassey [:blassey] (use needinfo?) 2012-06-21 06:45:00 PDT
A couple notes:

1) XUL Fennec works fine, kicks out to the youtube app on the device as it always has
2) Native Fennec with its UA switched to XUL Fennec's also works fine, kicking out to the youtube app

if we could get google to serve native fennec the same content as xul fennec, that would be ideal
Comment 5 Lawrence Mandel [:lmandel] (use needinfo) 2012-06-21 06:46:37 PDT
cc Jet, who was in contact with YouTube about getting mobile content in Firefox.
Comment 6 Johnathan Nightingale [:johnath] 2012-06-21 07:03:11 PDT
And kev, on the off chance he was on the same thread, and wakes up earlier.
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2012-06-21 07:54:41 PDT
(In reply to Brad Lassey [:blassey] from comment #4)
> A couple notes:
> 
> 1) XUL Fennec works fine, kicks out to the youtube app on the device as it
> always has
> 2) Native Fennec with its UA switched to XUL Fennec's also works fine,
> kicking out to the youtube app
> 
> if we could get google to serve native fennec the same content as xul
> fennec, that would be ideal

Note that the YouTube player app needs to be installed for the "kick out" to work.
Comment 8 Mark Finkle (:mfinkle) (use needinfo?) 2012-06-21 08:02:51 PDT
Created attachment 635315 [details] [diff] [review]
patch

This patch uses the basics of the "Request Desktop Site" patch to force sending a XUL UA to youtube.  The domain matching is a bit loose, only testing for "youtube" anywhere in the host.
Comment 9 Brad Lassey [:blassey] (use needinfo?) 2012-06-21 08:05:23 PDT
Created attachment 635316 [details] [diff] [review]
patch to send XUL Fennec UA to yotuube
Comment 10 Jason Smith [:jsmith] 2012-06-21 08:09:19 PDT
FYI - This bug is on the top list of topics to talk about at the mobile web compat meeting at 9am, if anyone is interested in attending to discuss it.
Comment 11 Brad Lassey [:blassey] (use needinfo?) 2012-06-21 08:14:47 PDT
Comment on attachment 635315 [details] [diff] [review]
patch

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

::: mobile/android/chrome/content/browser.js
@@ +2868,5 @@
> +#expand     let version = "__MOZ_APP_VERSION__";
> +            ua += " Fennec/" + version;
> +            channel.setRequestHeader("User-Agent", ua, false);
> +          }
> +        }

nit, enclose this case in curly brackets so channel and channelWindow are only scoped to this case
Comment 12 Mark Finkle (:mfinkle) (use needinfo?) 2012-06-21 08:49:24 PDT
As an example, in XUL Fennec when you tap the video to start playing, Youtube sends us this URL:

vnd.youtube:e9flNIz9qhg?vndapp=youtube_mobile&vndclient=mv-google&vndel=home&vnddnc=1
Comment 13 Erin Lancaster [:elan] 2012-06-21 09:07:20 PDT
Question: If you tube revs their site again, will this fix over-ride their changes?
Comment 14 Matt Brubeck (:mbrubeck) 2012-06-21 09:31:25 PDT
Comment on attachment 635315 [details] [diff] [review]
patch

>+    Services.obs.removeObserver(this, "http-on-modify-request", false);

Nit: The last argument (false) should be removed.
Comment 15 Brian Nicholson (:bnicholson) (PTO through August 1) 2012-06-21 09:35:03 PDT
One thing to consider is "http-on-modify-request" is called for every resource on every page. Having N tabs open would multiply the number of observed callbacks by N. It would be more efficient to have a single observer that operates on all tabs rather than having each tab listen for this event. I was going to make this same change in bug 766406.

Maybe you could put this in a class called UserAgent, and I could merge my AgentMode class into that?
Comment 16 Matt Brubeck (:mbrubeck) 2012-06-21 09:46:59 PDT
(In reply to Brian Nicholson (:bnicholson) from comment #15)
> One thing to consider is "http-on-modify-request" is called for every
> resource on every page. Having N tabs open would multiply the number of
> observed callbacks by N. It would be more efficient to have a single
> observer that operates on all tabs rather than having each tab listen for
> this event. I was going to make this same change in bug 766406.
> 
> Maybe you could put this in a class called UserAgent, and I could merge my
> AgentMode class into that?

Good point.  For simplicity in this patch (which is a temporary hack and will be removed soon), we could just add the observer to BrowserApp instead of Tab.prototype.
Comment 17 Brian Nicholson (:bnicholson) (PTO through August 1) 2012-06-21 10:02:23 PDT
Also, it may not be a big deal, but this check would match other hosts containing "youtube" (youtube-global.blogspot.com, www.youtube-mp3.org). You could make sure the end of the host string is ".youtube.com" to avoid this.
Comment 18 Brian Nicholson (:bnicholson) (PTO through August 1) 2012-06-21 10:07:06 PDT
(In reply to Brian Nicholson (:bnicholson) from comment #17)
> Also, it may not be a big deal, but this check would match other hosts
> containing "youtube" (youtube-global.blogspot.com, www.youtube-mp3.org). You
> could make sure the end of the host string is ".youtube.com" to avoid this.

Although that wouldn't match http://youtube.com - maybe a regex like:
/^(.+\.)?youtube\.com$/ ?
Comment 19 Brian Nicholson (:bnicholson) (PTO through August 1) 2012-06-21 10:11:17 PDT
(In reply to Mark Finkle (:mfinkle) from comment #8)
> Created attachment 635315 [details] [diff] [review]
> patch
> 
> This patch uses the basics of the "Request Desktop Site" patch to force
> sending a XUL UA to youtube.  The domain matching is a bit loose, only
> testing for "youtube" anywhere in the host.

Oh, just saw this.
Comment 20 Mark Finkle (:mfinkle) (use needinfo?) 2012-06-21 10:32:03 PDT
Created attachment 635365 [details] [diff] [review]
patch 2

This patch moves the code to a singleton. It was just as easy as doing it in BrowserApp and a tab cleaner imo.
Comment 21 Matt Brubeck (:mbrubeck) 2012-06-21 10:38:50 PDT
Comment on attachment 635365 [details] [diff] [review]
patch 2

>+  uninit: function ua_uninit() {
>+    Services.obs.removeObserver(this, "http-on-modify-request", false);
>+  },

Same nit as before: The "false" should be removed.
Comment 22 Mark Finkle (:mfinkle) (use needinfo?) 2012-06-21 10:42:45 PDT
Created attachment 635373 [details] [diff] [review]
patch for checkin

Patch with Matt's nit fixed. Ready to push.
Comment 23 Jason Smith [:jsmith] 2012-06-21 11:59:55 PDT
Top priority for QA to verify.
Comment 25 Mark Finkle (:mfinkle) (use needinfo?) 2012-06-21 12:05:11 PDT
Created attachment 635404 [details] [diff] [review]
alternate patch that sends desktop UA

This patch applies over that previous patch and sends the desktop UA instead of XUL UA.
Comment 26 Aaron Train [:aaronmt] 2012-06-21 12:17:44 PDT
Tested the private build in channel with the first patch checked in and playback functionality was restored; YouTube is fully functional as a mobile site now (rather pleasant to use now over desktop). I have not encountered any bizarre regressions thus far. 

Kevin, Naoki, what's it like on 2.2?

--
Tested via Galaxy Nexus (Android 4.0.4) & Droid Bionic (Android 2.3)
Comment 27 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-06-21 13:25:41 PDT
Comment on attachment 635373 [details] [diff] [review]
patch for checkin

>+      if (channel.URI.host.indexOf("youtube") != -1) {

I think you probably want a stricter check for whether this is the host you want.  (Probably either an equality check or maybe something using the effective-TLD code?)
Comment 28 Matt Brubeck (:mbrubeck) 2012-06-21 14:12:06 PDT
(In reply to David Baron [:dbaron] from comment #27)
> I think you probably want a stricter check for whether this is the host you
> want.  (Probably either an equality check or maybe something using the
> effective-TLD code?)

We decided that false positives were basically harmless for this temporary hack, so we intentionally erred on that side.
Comment 29 Xiao Meng Wei :xwei 2012-06-21 14:45:50 PDT
(In reply to Aaron Train [:aaronmt] from comment #26)
> Tested the private build in channel with the first patch checked in and
> playback functionality was restored; YouTube is fully functional as a mobile
> site now (rather pleasant to use now over desktop). I have not encountered
> any bizarre regressions thus far. 
> 
> Kevin, Naoki, what's it like on 2.2?
> 
> --
> Tested via Galaxy Nexus (Android 4.0.4) & Droid Bionic (Android 2.3)

Tested on Droid 2, android 2.2, the fix is working.
Comment 30 Aaron Train [:aaronmt] 2012-06-21 17:45:29 PDT
Verified Fixed via 14.0 Beta 8 - Candidate #1, Build #2 (buildID=20120621141442)

--
Samsung Galaxy Nexus (Android 4.0.4)
Samsung Nexus S (Android 4.0.3)
HTC Nexus One (Android 2.3.4)
Comment 31 Jason Smith [:jsmith] 2012-06-21 21:21:59 PDT
*** Bug 767257 has been marked as a duplicate of this bug. ***
Comment 32 Jet Villegas (:jet) 2012-07-27 15:52:27 PDT
Reopening this one per this July 5 e-mail from youtube:

"We verified that our fix works with both the stable release that launched last week, as well as the "Firefox Beta" app in the store. It is live now, so you can verify that it works on m.youtube.com"

They pushed a fix that should render the UA hack unnecessary. I'd love to back it out of Fennec if the youtube server-side fix is verified. Testing a Fennec build from just before 14.0b8 should verify the server changes.
Comment 33 Jason Smith [:jsmith] 2012-07-27 15:55:10 PDT
As a point of comparison, testing this on Firefox OS that does not contain the hack ends up showing vnd.youtube when you try to view a video (I believe that's the youtube android activity).
Comment 34 Jason Smith [:jsmith] 2012-07-27 17:04:36 PDT
Tested using FF Android 14.0 Beta 6 - Video plays with no issues.

I might suggest though we open a separate bug to track this to eliminate possible confusion.
Comment 35 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-28 20:46:55 PDT
Aaron - Can you close/verify this bug and open a new bug to backout the change?
Comment 36 Aaron Train [:aaronmt] 2012-07-29 11:12:37 PDT
(In reply to Mark Finkle (:mfinkle) from comment #35)
> Aaron - Can you close/verify this bug and open a new bug to backout the
> change?

bug 778561
Comment 37 Chris Lord [:cwiiis] 2012-08-25 05:50:10 PDT
It would appear that this is happening again - Going to any Youube video on the mobile site in current Nightly results in a refresh loop on play.
Comment 38 Lawrence Mandel [:lmandel] (use needinfo) 2012-08-25 06:23:52 PDT
(In reply to Chris Lord [:cwiiis] from comment #37)
> It would appear that this is happening again - Going to any Youube video on
> the mobile site in current Nightly results in a refresh loop on play.

I don't encounter this issue with the current nightly. YouTube videos correctly kick out to the YouTube app. I tested with Nightly 2012-08-25 on a Samsung Galaxy S2 ICS 4.0.4. With what version of Nightly and what device and OS are you testing?
Comment 39 Chris Lord [:cwiiis] 2012-08-25 07:53:23 PDT
Nightly 2012-08-25 on a Google Nexus 7.
Comment 40 Lawrence Mandel [:lmandel] (use needinfo) 2012-08-25 17:51:19 PDT
:cwiiis - 
1. What version of Android are you running? 
2. Can you reproduce the issue on all channels or only on Nightly? 
3. Have you tried clearing the data/cache for Nightly? Any change?
Comment 41 Aaron Train [:aaronmt] 2012-08-27 07:40:15 PDT
08/27 GN 4.1.1, I get the Android YouTube player; wfm.
Comment 42 Jason Smith [:jsmith] 2012-08-27 07:41:30 PDT
Closing.
Comment 43 Chris Lord [:cwiiis] 2012-08-29 04:53:37 PDT
(In reply to Jason Smith [:jsmith] from comment #42)
> Closing.

This still doesn't work on the Nexus 7. Will file a new bug as people seem eager to keep this one closed.

Note You need to log in before you can comment on or make changes to this bug.