766914 - Unable to play videos on mobile YouTube; redirection loop on 'Play'

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Catalin Suciu [:csuciu]]

Attachment: 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]]

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)]

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 4

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

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)]

cc Jet, who was in contact with YouTube about getting mobile content in Firefox.

Comment 6

[Johnathan Nightingale [:johnath]]

And kev, on the off chance he was on the same thread, and wakes up earlier.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

(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?)]

Attachment: 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?)]

Attachment: patch to send XUL Fennec UA to yotuube

Comment 10

[Jason Smith [:jsmith]]

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?)]

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?)]

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]]

Question: If you tube revs their site again, will this fix over-ride their changes?

Comment 14

[Matt Brubeck (:mbrubeck)]

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)]

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)]

(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)]

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)]

(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)]

(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?)]

Attachment: 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)]

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?)]

Attachment: patch for checkin

Patch with Matt's nit fixed. Ready to push.

Comment 23

[Jason Smith [:jsmith]]

Top priority for QA to verify.

Comment 24

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/551d99cd89d4
https://hg.mozilla.org/releases/mozilla-aurora/rev/01e96c76e268
https://hg.mozilla.org/releases/mozilla-beta/rev/f54b2bbd3baf (14_0_Beta)
https://hg.mozilla.org/releases/mozilla-beta/rev/35a98282d2a6 (MOBILE140_2012061216_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-beta/rev/aea5ee5b6801 (default)

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: 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]]

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-4, no longer working on Mozilla)]

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)]

(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]

(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]]

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 32

[Jet Villegas (inactive)]

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]]

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]]

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?)]

Aaron - Can you close/verify this bug and open a new bug to backout the change?

Comment 36

[Aaron Train [:aaronmt]]

(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]]

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)]

(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]]

Nightly 2012-08-25 on a Google Nexus 7.

Comment 40

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

: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]]

08/27 GN 4.1.1, I get the Android YouTube player; wfm.

Comment 42

[Jason Smith [:jsmith]]

Closing.

Comment 43

[Chris Lord [:cwiiis]]

(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.