1240471 - Youtube flash to iframe embed conversion breaks on invalid query string

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[mkdante381]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160117030215

Steps to reproduce:

I see new preference enabled in Nightly:
plugins.rewrite_youtube_embeds;true
This preference fix problem example on site http://tomclancy-thedivision.ubi.com/game/en-US/beta/index.aspx, but not on other sites completely, with default flash youtube embed. Example on http://www.stalker.pl/forum/viewtopic.php?f=9&t=134&start=6276 Firefox Nightly convert this to html5, but window this video have other size. 
Look on solution and how look youtube embed flash video, converted to htm5:
1. Install Greasemonkey

2. Install scripts
https://greasyfork.org/pl/scripts/13174-replace-old-flash-player-based-youtube-embeds-by-their-new-html5-counterparts
and
https://greasyfork.org/pl/scripts/3435-youtube-embed-tweak-html5

3. Execution order script, 1st is "Replace old Flash Player-based YouTube embeds by their new HTML5 counterparts" and 2nd Youtube Embed Tweak HTML5. 

4.In script "Youtube Embed Tweak HTML5" change var videosize to 100%
I changed to default size and settings:
// Set Video Size, with a 16:9 preset, Large (1024x576) medium (720x405) or Set size in percentage, for a video size as a percantage of the container size. 
var videosize = '100%';
// or
//var videosize = '50%';
// nochangeurl must be set to no for player settings to work. yes = default url and no = modified url, size is always modified.
var nochangeurl = 'no';
// Show youtube logo, yes or no
var ytlogo = 'yes';
// Show annoations yes or no. 
var annotation = 'yes';
// Show Related videos at end of playback, yes or no.
var related = 'yes';
// Force https option, yes or no
var ssl = 'yes';


Actual results:

Nightly not convert correctly Youtube embed flash movie on some sites

Comment 1

[mkdante381]

[Tracking Requested - why for this release]:

Comment 2

[mkdante381]

Corrected:
Firefox Nightly convert this to html5, but window this video have other size, video is not played, corrupted player UI, and so on
Look on screenshot:
http://i.imgur.com/tTVJzXA.jpg

Comment 3

[Alex]

I ran into this on a few sites yesterday, the problem is that the sites use URLs like this

http://www.youtube.com/v/eoUf1bbMTa8&hl=pl&fs=1

The query string is malformed, instead of passing the extra variables alongside the video ID, they append them to the video ID. The Flash player handled this situation correctly, but the HTML5 player doesn't.

Comment 4

[Jean-Yves Avenard [:jya]]

This bug makes no sense; we never convert anything.
It's either flash or html5.

YouTube doesn't support Flash anymore.

Raise the bug with youtube.

Comment 5

[mkdante381]

Nope, This bug makes sense. YouTube don't support officially flash, but some sites still have youtube flash embed movie. With addons is possible activate old youtube flash player. In Nightly is new feature, but not "convert" correctly mainly old Flash Player-based YouTube embeds to HTML5.

Comment 6

[Jean-Yves Avenard [:jya]]

Then report the problem to whomever did the addons.

However, since YouTube typically decides what it's going to send based on the useragent, there's no guarantee any such addons will continue to work.

This isn't a gecko media problem

Comment 7

[mkdante381]

I do not write about add-ons, but about new feature in Nightly.
PLS read this:
http://www.ghacks.net/2016/01/16/firefox-to-convert-old-youtube-flash-code-to-html5-video/
Bug is valid. Read all and Stop change status ok? 

Do you know situation? I write about Youtube embed flash movies on third party sites, not on youtube site...

This is not gecko problem, because this is problem this new feature. Nightly change old code flash to html5, but not correctly. 

Look example on this site:
http://www.ghacks.net/2007/03/02/one-of-the-greatest-line-rider-movies-ever/
in stable, beta and Aurora YT movie embedded on up is in flash, but on Nightly with "plugins.rewrite_youtube_embeds;true" is in HTML5, UI player skin youtube is corrupted(there is no pause button and other artifacts), but movie is playable. 
Other situation (and certainly on the other www sites with the old code flash youtube)on http://www.stalker.pl/forum/viewtopic.php?f=9&t=134&start=6276 , because on stable, beta and Aurora YouTube embedded movies are on Flash(this is normal), but on Nightly with "plugins.rewrite_youtube_embeds;true" ui player skin is corrupted(there is no pause button and other artifacts) and movie is not possible to played.

Rules on bugzilla:
1. Read all what reporter write about bug
2. If you don't know how it works or don't know anything about this bug/situation, don't give advice about it
3. Don't change status bug, until someone else has not taken

Comment 8

[Alice0775 White]

mkdante381,
Could you test with the following step?
1. Rename SiteSecurityServiceState.txt to SiteSecurityServiceState.txt.backup in your profile folder
2. Run Nightly with the profile
3. And then Follow step of comment#7


** Do not forget to revert the name of SiteSecurityServiceState.txt after test.

Comment 9

[mkdante381]

OK thx for reply. After rename SiteSecurityServiceState.txt to SiteSecurityServiceState.txt.backup in my profile folder and Run Nightly with my profile 
On site:
http://www.ghacks.net/2007/03/02/one-of-the-greatest-line-rider-movies-ever/ 
with youtube player ui skin all is ok and possible to play movie
On site:
http://www.stalker.pl/forum/viewtopic.php?f=9&t=134&start=6276 
with youtube player ui skin all is ok but not possible to play movie, error from comment 2( http://i.imgur.com/tTVJzXA.jpg ). Maybe bug because, Stalker.pl Forum use old flash youtube bbcode with http url and don't support https. Nightly "convert" this but not connect with content movie. My suggestion is that, Firefox Nightly and later in stable, beta and aurora channel, default changing url from http to https

Comment 10

[Alice0775 White]

Thanks
I have filed Bug 1240566 regarding video control ui problem.

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Yeah, this bug is definitely valid (:jya, see bug 769117, we do rewrite youtube URLs now). We may have to just not rewrite URLs with invalid query strings also. We currently skip anything with enablejsapi=1 since that usually means js manipulation of flash, but I wasn't aware of sites attaching strings that would break the iframe version of youtube embeds. Will take a look at this soon.

Comment 12

[mkdante381]

Thanks for reply. Better for security is force https, when Nightly "convert" old flash youtube movie and better to work, because maybe on sites with old flash youtube code http, after add https, then it will be possible play html5 embedded movie.

Comment 13

[mkdante381]

Next bug
1. clear all history in latest nightly with "plugins.rewrite_youtube_embeds;true"
2. go to http://tomclancy-thedivision.ubi.com/game/en-us/beta/
3. Go to youtube, log-in and change for your language
4. go to http://tomclancy-thedivision.ubi.com/game/en-us/beta/ and embedded movie player ui is in english only, corrupted youtube embedded player ui(no pause button, no cog for change resolution and so on)
This is problem with SiteSecurityServiceState.txt

Comment 14

[Masatoshi Kimura [:emk]]

:jya is right about the Video/Audio playback component is irrelevant. Bug 769117 should also have been classified under Core/DOM.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this is a regression in 46. 
Confirming based on comment 11.

Comment 16

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

I've confirmed that conversion does break when there's an invalid query string on the embed url, like 

http://www.youtube.com/v/eoUf1bbMTa8&hl=pl&fs=1

What do we want to do in this case? I guess I can not convert if a query string is in an invalid format?

Comment 17

[Chris Peterson [:cpeterson]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #16)
> I've confirmed that conversion does break when there's an invalid query
> string on the embed url, like 
> 
> http://www.youtube.com/v/eoUf1bbMTa8&hl=pl&fs=1
> 
> What do we want to do in this case? I guess I can not convert if a query
> string is in an invalid format?

I wonder how common that problem is. How would you detect these bogus URLs? If you can reliably detect them, then it makes sense to leave them alone and not rewrite them.

Comment 18

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Chris Peterson [:cpeterson] from comment #17)
> (In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #16)
> > I've confirmed that conversion does break when there's an invalid query
> > string on the embed url, like 
> > 
> > http://www.youtube.com/v/eoUf1bbMTa8&hl=pl&fs=1
> > 
> > What do we want to do in this case? I guess I can not convert if a query
> > string is in an invalid format?
> 
> I wonder how common that problem is. How would you detect these bogus URLs?
> If you can reliably detect them, then it makes sense to leave them alone and
> not rewrite them.

Usually parsing URL queries is the job of the server, not the browser. We do have the URLSearchParams class which we could possibly use to try and detect malformed embeds, but that'd take some work since it's more for tokenizing or serializing strings, not checking validity.

Another option is to just not rewrite /anything/ that has a query string, but that might be overreaching some, since query strings for things like time settings (t=55s for example) are pretty popular.

Comment 19

[Chris Peterson [:cpeterson]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #18)
> Usually parsing URL queries is the job of the server, not the browser. We do
> have the URLSearchParams class which we could possibly use to try and detect
> malformed embeds, but that'd take some work since it's more for tokenizing
> or serializing strings, not checking validity.

> Another option is to just not rewrite /anything/ that has a query string,
> but that might be overreaching some, since query strings for things like
> time settings (t=55s for example) are pretty popular.

Technically, this broken video doesn't actually have a query string because there is no question mark. :)

What is your recommendation? Maybe just check for URLs with '&' without '?' or with '&' preceding '?'? We don't know how common this problem is, but Alex said in comment 3 that he has already seen this problem "on a few sites yesterday".

> > > http://www.youtube.com/v/eoUf1bbMTa8&hl=pl&fs=1

Comment 20

[arni2033]

> (comment 16) I guess I can not convert if a query string is in an invalid format?
> (comment 18) query strings for things like time settings (t=55s for example) are pretty popular

YouTube IFrame Player API: https://developers.google.com/youtube/player_parameters
Note that some parameters are also supported by HTML5 player and probably can be "converted"
I used such urls many times (in the internet):
> data:text/html,<embed src="https://www.youtube.com/v/SLBi0i8gGlE&start=27&end=32&autoplay=1">

Comment 21

[Alex]

It seems that for a while YouTube itself suggested the malformed URLs as the default way to embed videos (The oEmbed homepage shows an example of it http://oembed.com/#section5), and I've found a few references to sample code (e.g. http://stackoverflow.com/questions/412467/how-to-embed-youtube-videos-in-php) that generate malformed URLs, so people copying it will perpetuate it.

I don't think fixing these URLs when re-writing them would break anything, it shouldn't be visible to the site, it's just the internal requested URL that'd change.

Comment 22

[Chris Peterson [:cpeterson]]

Thanks for the links, arni2033 and Alex. That's very interesting that the oEmbed example code has the same problem. That suggests the problem might be more common than we expected. To be fair, YouTube did not publish that particular example code. :)

I suspect this happened when YouTube allowed the video ID to be moved from the query string (e.g. "https://www.youtube.com/watch?v=SLBi0i8gGlE&start=27") into the URL path (e.g. "https://www.youtube.com/v/SLBi0i8gGlE&start=27") and people "cleaned up" their URLs without fixing the query string parameters.

Comment 23

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Remove invalid queries and post console messages on youtube embed rewrites

This patch removes invalid queries from embeds, and warns developers when an embed change happens. Just realized the logic is faulty (i.e. http://youtube.com/embed/VideoIdHere&start=15?somethingelse=1 would be considered valid), so need to fix that, but am also looking for input on the wording of the developer console messages.

Comment 24

[Chris Peterson [:cpeterson]]

Comment on attachment 8720058 [details] [diff] [review]
Patch 1 (v1) - Remove invalid queries and post console messages on youtube embed rewrites

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

::: dom/base/nsObjectLoadingContent.cpp
@@ +1802,5 @@
>        uriStr.ReplaceSubstring(NS_LITERAL_STRING("/v/"),
>                                NS_LITERAL_STRING("/embed/"));
> +      // At this point, we've got a rewritten URI that will point to the proper
> +      // iframe embed of youtube. Some domains have invalid query strings
> +      // attached due to improper conversion of URLs, e.g.

"due to improper conversion of URLs" makes the invalid query strings sound like it is our fault when it's not. :-) Maybe just remove that substring?

@@ +1808,5 @@
> +      // flash, but break iframe/object embedding. If this situation occurs with
> +      // rewritten URLs, chop off the query in order to make the video load
> +      // correctly, and warn about it in the developer console.
> +      if (uriStr.FindChar('&', 0) != -1 &&
> +          uriStr.FindChar('?', 0) == -1) {

As you said in comment 23, this check doesn't cover the invalid case where the query string's (first) question mark comes after the first ampersand.

::: dom/locales/en-US/chrome/dom/dom.properties
@@ +202,5 @@
>  PatternAttributeCompileFailure=Unable to check <input pattern='%S'> because the pattern is not a valid regexp: %S
>  # LOCALIZATION NOTE: Do not translate "postMessage" or DOMWindow. %S values are origins, like https://domain.com:port
>  TargetPrincipalDoesNotMatch=Failed to execute 'postMessage' on 'DOMWindow': The target origin provided ('%S') does not match the recipient window's origin ('%S').
> +# LOCALIZATION NOTE: Do not translate 'youtube'. %S values are origins, like https://domain.com:port
> +RewriteYoutubeEmbed=Rewriting old-style youtube embed (%S) to iframe embed (%S). Please update page to use iframe instead of embed/object, if possible.

LGTM. I think the message should include the word "Flash" somewhere. Also YouTube should be capitalized. Something like "Rewriting old-style youtube embed".

Comment 25

[arni2033]

> "due to improper conversion of URLs" makes the invalid query strings
> sound like it is our fault when it's not.
I strongly disagree. Currently Nightly takes completely valid working page and breaks it.
Furthermore, it claims that site owner did something wrong and must perform some more work.

I haven't noticed it that Youtube Flash embeds were prohibited, so they should continue working as expected. If you remove parameters, it breaks the video. So Nightly should signalize in console that this breakage is intentional decision of Firefox devs, and isn't a site owner's fault.

Comment 26

[arni2033]

(I forgot this part)
OR, alternatively, Nightly should indeed say something about rewriting in console, but
don't break the page in that case, i.e. that function "ShouldRewriteYoutubeEmbed"
should return false (or something more sensible) in case you don't like URI.

Comment 27

[Chris Peterson [:cpeterson]]

(In reply to arni2033 from comment #25)
> > "due to improper conversion of URLs" makes the invalid query strings
> > sound like it is our fault when it's not.
> I strongly disagree. Currently Nightly takes completely valid working page
> and breaks it.
> Furthermore, it claims that site owner did something wrong and must perform
> some more work.

Thanks, arni2033. That's a good point.

Kyle, if we detect an invalid query string, maybe we should not rewrite the embed and just let Flash try to play?

We could add some extra logic to rewrite the embed without its invalid query string if the user doesn't have Flash enabled, but I don't think we need to go that far for this corner case. :)

Comment 28

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Chris Peterson [:cpeterson] from comment #27)

> Kyle, if we detect an invalid query string, maybe we should not rewrite the
> embed and just let Flash try to play?

Yup. Will just push the check back into the ShouldRewrite logic, and have that return false. Will still post console message about not being able to rewrite and that it should be fixed.

Comment 29

[Alex]

If the user's got Flash installed then it's probably for the best to let it play them, but if they don't (like my systems) then it's trading one form of brokenness for another (The grey squares become black squares)

I could reenable Flash, but obviously I'd rather not, and if I still need to enable Flash to use these old style embeds, then there's not much point in rewriting them for me.

Comment 30

[mkdante381]

This feature "plugins.rewrite_youtube_embeds;true", make no sense, when https is not forced. Many movies will not be played in HTML5, when Youtube embeded movie is with http. Flash instead HTML5 on http embed movie is not good option. Many users not use flash. My proposition is force https on embeded Youtube movies as an option(additional preference). Adobe flash is danger, same bugs, often crash...

Comment 31

[Chris Peterson [:cpeterson]]

Kyle, do you have any updates on your YouTube query string patch?

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

Recent regression, tracking for 46+.  (Assuming 47 is also affected)

Comment 33

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v2) - Remove invalid queries and post console messages on youtube embed rewrites

Consolidated all of the rewriting work into a single function, since the logic is getting a little hairy now. Still needs mochitests, which I'm working on, they're just somewhat complicated from having to turn plugins on/off to test things.

Comment 34

[Chris Peterson [:cpeterson]]

Kyle, do you think we should pref off the YouTube rewriter in 46? We only have one week until 46 rides from Aurora to Beta and we still have two (known) regressions: this bug and bug 1246088. This feature is not urgent, so getting more bake time in 47 to find any other regressions might be a good idea. :)

Comment 35

[Chris Peterson [:cpeterson]]

Comment on attachment 8724355 [details] [diff] [review]
Patch 1 (v2) - Remove invalid queries and post console messages on youtube embed rewrites

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

::: dom/base/nsObjectLoadingContent.cpp
@@ +40,4 @@
>  #include "nsIAsyncVerifyRedirectCallback.h"
>  #include "nsIAppShell.h"
>  #include "nsIXULRuntime.h"
> +#include "nsIScriptError.h"

nsIScriptError.h sorts before nsIXULRuntime.h alphabetically.

@@ +1540,5 @@
> +  // not have flash installed or activated, chop off the query in order to make
> +  // the video load correctly as an iframe. In either case, warn about it in the
> +  // developer console.
> +  int32_t ampIndex = uri.FindChar('&', 0);
> +  int32_t qmIndex = uri.FindChar('?', 0);

We only need to look for '?' if we found '&', so you can move FindChar('?') inside the `if (ampIndex != -1)` conditional. And since we only care if qmIndex > ampIndex, I think you can start searching after ampIndex like `FindChar('?', ampIndex + 1)` instead of from 0.

@@ +1565,5 @@
> +
> +  nsAutoString utf16OldURI = NS_ConvertUTF8toUTF16(uri);
> +  // Switch out video access url formats, which should possibly allow HTML5
> +  // video loading.
> +  nsresult rv;

You can move `nsresult rv` down to the NewURIWithDocumentCharset() call, where rv is first initialized.

@@ +1569,5 @@
> +  nsresult rv;
> +  // If we need to trim the query off the URL, it means it's malformed, and an
> +  // ampersand comes first. Use the index we found earlier.
> +  if (trimQuery) {
> +    uri.Truncate(ampIndex + 4);

Where does the magic number 4 some from?

@@ +1597,5 @@
> +                                    NS_LITERAL_CSTRING("Plugins"),
> +                                    thisContent->OwnerDoc(),
> +                                    nsContentUtils::eDOM_PROPERTIES,
> +                                    "RewriteYoutubeEmbedInvalidQuery",
> +                                    params, ArrayLength(params));

These two ReportToConsole() calls are mostly duplicated code. The only difference is the fifth parameter, the message name. You can consolidate them into one call by just introducing a new local variable for the message name depending on trimQuery ? "RewriteYoutubeEmbed" : "RewriteYoutubeEmbedInvalidQuery".

@@ +1867,3 @@
>        newMime = NS_LITERAL_CSTRING("text/html");
>      }
> +    

remove whitespace.

::: dom/locales/en-US/chrome/dom/dom.properties
@@ +205,5 @@
>  TargetPrincipalDoesNotMatch=Failed to execute 'postMessage' on 'DOMWindow': The target origin provided ('%S') does not match the recipient window's origin ('%S').
> +# LOCALIZATION NOTE: Do not translate 'youtube'. %S values are origins, like https://domain.com:port
> +RewriteYoutubeEmbed=Rewriting old-style youtube embed (%S) to iframe embed (%S). Please update page to use iframe instead of embed/object, if possible.
> +# LOCALIZATION NOTE: Do not translate 'youtube'. %S values are origins, like https://domain.com:port
> +RewriteYoutubeEmbedInvalidQuery=Rewriting old-style youtube embed (%S) to iframe embed (%S). Query was invalid and removed from URL. Please update page to use iframe instead of embed/object, if possible.
\ No newline at end of file

I think we should change s/youtube embed/YouTube Flash embed/.

Comment 36

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Chris Peterson [:cpeterson] from comment #34)
> Kyle, do you think we should pref off the YouTube rewriter in 46? We only
> have one week until 46 rides from Aurora to Beta and we still have two
> (known) regressions: this bug and bug 1246088. This feature is not urgent,
> so getting more bake time in 47 to find any other regressions might be a
> good idea. :)

Yeah, since we're still finding weirdness about this at this point, I'm fine pref'ing it off for another version.

Comment 37

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Chris Peterson [:cpeterson] from comment #35)
> Comment on attachment 8724355 [details] [diff] [review]
> Patch 1 (v2) - Remove invalid queries and post console messages on youtube
> embed rewrites
> 
> Review of attachment 8724355 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsObjectLoadingContent.cpp
> @@ +40,4 @@
> >  #include "nsIAsyncVerifyRedirectCallback.h"
> >  #include "nsIAppShell.h"
> >  #include "nsIXULRuntime.h"
> > +#include "nsIScriptError.h"
> 
> nsIScriptError.h sorts before nsIXULRuntime.h alphabetically.

None of the rest of that list is in alphabetical order, which is why I added it at the end. Otherwise nsIAsync... would be after nsIApp... too

> @@ +1540,5 @@
> > +  // not have flash installed or activated, chop off the query in order to make
> > +  // the video load correctly as an iframe. In either case, warn about it in the
> > +  // developer console.
> > +  int32_t ampIndex = uri.FindChar('&', 0);
> > +  int32_t qmIndex = uri.FindChar('?', 0);
> 
> We only need to look for '?' if we found '&', so you can move FindChar('?')
> inside the `if (ampIndex != -1)` conditional. And since we only care if
> qmIndex > ampIndex, I think you can start searching after ampIndex like
> `FindChar('?', ampIndex + 1)` instead of from 0.

If we see a '?' before the ampersand, it's a valid query and we don't want to change it. If there's no '?' at all, or the '?' comes after the ampersand, it breaks. So we need to know where it is from 0, not from where the ampersand is.

> @@ +1569,5 @@
> > +  nsresult rv;
> > +  // If we need to trim the query off the URL, it means it's malformed, and an
> > +  // ampersand comes first. Use the index we found earlier.
> > +  if (trimQuery) {
> > +    uri.Truncate(ampIndex + 4);
> 
> Where does the magic number 4 some from?

That was an accident, shouldn't have been in here, sorry about that.

Comment 38

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v3) - Remove invalid queries and post console messages on youtube embed rewrites

Comment 39

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v4) - Remove invalid queries and post console messages on youtube embed rewrites

Fixed accidental deletion in l10n file.

Comment 40

[Chris Peterson [:cpeterson]]

Comment on attachment 8724902 [details] [diff] [review]
Patch 1 (v4) - Remove invalid queries and post console messages on youtube embed rewrites

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

LGTM but you should probably ask a DOM person to review the URI manipulation.

Comment 41

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ok, I'll finish out the tests then have a DOM person look at it.

Comment 42

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v5) - Remove invalid queries and post console messages on youtube embed rewrites

Now with mochitests.

Comment 43

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8725110 [details] [diff] [review]
Patch 1 (v5) - Remove invalid queries and post console messages on youtube embed rewrites

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

::: dom/base/nsObjectLoadingContent.h
@@ +539,5 @@
>       * embed.
>       */
> +    void MaybeRewriteYoutubeEmbed(nsIURI* aURI,
> +                                   nsIURI* aBaseURI,
> +                                   nsIURI** aRewrittenURI);

formatting

Comment 44

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e40f1763014

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9e40f1763014

Comment 46

[Francesco Lodolo [:flod]]

Youtube should be spelled correctly as YouTube in the strings (also no need to update the string ID if you just want to land a quick followup).

Comment 47

[mkdante381]

Now Nightly force https on http YouTube embeded movies? Nightly must force https, because without this html5 http embeded movies not work/loading.

Comment 48

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8725110 [details] [diff] [review]
Patch 1 (v5) - Remove invalid queries and post console messages on youtube embed rewrites

Approval Request Comment
[Feature/regressing bug #]: Bug 769117
[User impact if declined]: Some YouTube embed rewrites will fail silently
[Describe test coverage new/current, TreeHerder]: Mochitests, landed to m-c
[Risks and why]: None, especially since we're most likely pref'ing this off before it hits release until things are a bit more stable.
[String/UUID change made/needed]: Added a couple of strings that will be relayed to the dev console when YouTube rewrites happen.

Comment 49

[Francesco Lodolo [:flod]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #48)
> [String/UUID change made/needed]: Added a couple of strings that will be
> relayed to the dev console when YouTube rewrites happen.

Please create an ad-hoc version of this patch with hard-coded strings if you plan to land on string frozen branches. We're at the end of the cycle and this landing likely bleed into beta.

Comment 50

[Liz Henry (:lizzard) (relman/hg->git project)]

If you can get a new patch in today we can still uplift tonight or over the weekend. If not then we can do this next week once 46 is in beta.

Comment 51

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Nevermind, this is going to sit until 47. There's been too many problems so far to try and push it forward, I'd rather late it bake a bit longer. We'll be pref'ing the feature off in beta next week to make sure it doesn't go out early.

Comment 52

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8725110 [details] [diff] [review]
Patch 1 (v5) - Remove invalid queries and post console messages on youtube embed rewrites

After talking with qdot we are going to let this ride with 47.

Comment 53

[mkdante381]

This work good on youtube embeded movies on http://www.stalker.pl/forum/viewtopic.php?f=9&t=134&start=6276 , but not work fullscreen, inscription "fullscreen is not available"

Comment 54

[mkdante381]

Tested on latest Nightly

Comment 55

[Chris Peterson [:cpeterson]]

I can reproduce the fullscreen problem on Nightly. Looking at the log messages in the web console, I see that the problem is we are dropping the "fs=1" query string parameter. Without fs=1, YouTube's embedded player apparently disallows fullscreen playback.

Possible next steps:

1. Do nothing. Let fullscreen break for videos with invalid query strings. The site can fix their HTML.

2. Always add "fs=1" query string parameter to the rewritten URLs. We could do this even if fs=1 wasn't in the original query string because preventing fullscreen is user-unfriendly.

3. Fix up the invalid query strings. We decided previously that this is too fragile or risky for the corner case where embedded videos have invalid URLs. The site can just fix their HTML.

I suggest we do #1 or #2.

@ Kyle: what do you recommend?

Comment 56

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

I vote #1. for #2, that means I have to figure out whether there already is a query, and either do ? or & appropriately, and I really don't want to continue any farther down the slippery slope than we already are.

Comment 57

[Chris Peterson [:cpeterson]]

OK. Doing nothing sounds good to me. :)

Comment 58

[mkdante381]

(In reply to Chris Peterson [:cpeterson] from comment #55)
> I can reproduce the fullscreen problem on Nightly. Looking at the log
> messages in the web console, I see that the problem is we are dropping the
> "fs=1" query string parameter. Without fs=1, YouTube's embedded player
> apparently disallows fullscreen playback.
> 
> Possible next steps:
> 
> 1. Do nothing. Let fullscreen break for videos with invalid query strings.
> The site can fix their HTML.
> 
> 2. Always add "fs=1" query string parameter to the rewritten URLs. We could
> do this even if fs=1 wasn't in the original query string because preventing
> fullscreen is user-unfriendly.
> 
> 3. Fix up the invalid query strings. We decided previously that this is too
> fragile or risky for the corner case where embedded videos have invalid
> URLs. The site can just fix their HTML.
> 
> I suggest we do #1 or #2.
> 
> @ Kyle: what do you recommend?

Wrong, Step 1 does not make sense
This site ( http://www.stalker.pl/forum/viewtopic.php?f=9&t=134&start=6276 ) from comment 53 use old flash embeded code, http embeded movies, with allow fullscreen. This feature is almost perfect. Nightly rewrite this movies to html5, but only fulscreen is not available, the rest is OK. Firefox must recognize when embeded movie sites use fullscreen or not.

Comment 59

[Alex]

It's not an issue with the query string, malformed or valid, it's that the "allowfullscreen" attribute only has an effect for <iframe>, it's ignored on <object> or <embed>

Comment 60

[Chris Peterson [:cpeterson]]

(In reply to mkdante381 from comment #58)
> This site ( http://www.stalker.pl/forum/viewtopic.php?f=9&t=134&start=6276 )
> from comment 53 use old flash embeded code, http embeded movies, with allow
> fullscreen. This feature is almost perfect. Nightly rewrite this movies to
> html5, but only fulscreen is not available, the rest is OK. Firefox must
> recognize when embeded movie sites use fullscreen or not.

The problem is that the website is using an invalid YouTube URL:

http://www.youtube.com/v/eoUf1bbMTa8&hl=pl&fs=1
                                    ^ this "&" should be "?"

The website should fix its YouTube URLs. YouTube's HTML5 video player doesn't understand these invalid URLs (which caused the original problem reported in comment 0). YouTube's Flash video player will happily load the invalid URLs. So all the URL parameters (in this example: "&hl=pl&fs=1") are ignored. Without the "&fs=1" parameter, YouTube's HTML5 video player doesn't allow fullscreen playback.

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #56)
> I vote #1. for #2, that means I have to figure out whether there already is
> a query, and either do ? or & appropriately, and I really don't want to
> continue any farther down the slippery slope than we already are.

Perhaps we could append "?fs=1" to the new URL only in the case where we truncate the invalid query string. Then we wouldn't need to scan the original URL to see if the new URL needs "?fs=1" or "&fs=1". We would leave valid query strings untouched, respecting the web page's original intention.

Comment 61

[Masatoshi Kimura [:emk]]

(In reply to Chris Peterson [:cpeterson] from comment #60)
> YouTube's Flash video player will happily load the
> invalid URLs. So all the URL parameters (in this example: "&hl=pl&fs=1") are
> ignored.

No, YouTube's Flash video player does NOT ignore the "invalid" URL parameters. It parses the "invalid" parameters somehow. For example, <https://www.youtube.com/v/7LcUOEP7Brc&start=35> will start playback at 0:35.
Rather, I'm not sure these types of URL should be called "invalid". Nothing prevents web apps from taking parameters from the path component. It is up to apps how the URL will be parsed.

> Perhaps we could append "?fs=1" to the new URL only in the case where we
> truncate the invalid query string. Then we wouldn't need to scan the
> original URL to see if the new URL needs "?fs=1" or "&fs=1". We would leave
> valid query strings untouched, respecting the web page's original intention.

How difficult is it to replace the first "&" to "?" if "?" is not found in the URL?

Comment 62

[Masatoshi Kimura [:emk]]

I filed bug 1258053 to continue the discussion.

Comment 63

[Liz Henry (:lizzard) (relman/hg->git project)]

I'm wondering here if, since we know it's an issue with 46 and I believe we may be fixing this in 47, should we add this as a known problem to the release notes?

Comment 64

[arni2033]

Beta 46 is unaffected, because youtube rewriting was disabled in bug 1255918

Comment 65

[mkdante381]

Fullscreen not working on http://www.stalker.pl/forum/viewtopic.php?f=9&t=134&p=320453#p320453