Closed Bug 1240471 Opened 4 years ago Closed 4 years ago

Youtube flash to iframe embed conversion breaks on invalid query string

Categories

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

46 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla47
Tracking Status
firefox43 --- unaffected
firefox44 --- unaffected
firefox45 --- unaffected
firefox46 + unaffected
firefox47 + verified

People

(Reporter: mkdante381, Assigned: qdot)

References

()

Details

(Keywords: regression)

Attachments

(1 file, 4 obsolete files)

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
[Tracking Requested - why for this release]:
OS: Unspecified → Windows 7
Hardware: Unspecified → x86_64
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
Component: Untriaged → Audio/Video: Playback
Product: Firefox → Core
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.
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.
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.
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
Status: UNCONFIRMED → RESOLVED
Closed: 4 years ago
Resolution: --- → INVALID
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
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
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.
Flags: needinfo?(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
Flags: needinfo?(mkdante381)
Thanks
I have filed Bug 1240566 regarding video control ui problem.
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.
Assignee: nobody → kyle
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.
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
:jya is right about the Video/Audio playback component is irrelevant. Bug 769117 should also have been classified under Core/DOM.
Component: Audio/Video: Playback → DOM
Tracking since this is a regression in 46. 
Confirming based on comment 11.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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?
Flags: needinfo?(cpeterson)
Summary: Nightly does not convert correctly flash youtube embed movie to html5 → Youtube flash to iframe embed conversion breaks on invalid query string
(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.
Flags: needinfo?(cpeterson)
(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.
Flags: needinfo?(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
Flags: needinfo?(cpeterson)
> (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">
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.
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.
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.
Attachment #8720058 - Flags: feedback?(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".
Attachment #8720058 - Flags: feedback?(cpeterson) → feedback+
> "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.
(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.
(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. :)
Flags: needinfo?(kyle)
(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.
Flags: needinfo?(kyle)
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.
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...
Kyle, do you have any updates on your YouTube query string patch?
Flags: needinfo?(kyle)
Recent regression, tracking for 46+.  (Assuming 47 is also affected)
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.
Attachment #8720058 - Attachment is obsolete: true
Flags: needinfo?(kyle)
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. :)
Flags: needinfo?(kyle)
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/.
(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.
Flags: needinfo?(kyle)
(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.
Fixed accidental deletion in l10n file.
Attachment #8724900 - Attachment is obsolete: true
Attachment #8724900 - Flags: review?(cpeterson)
Attachment #8724902 - Flags: review?(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.
Attachment #8724902 - Flags: review?(cpeterson) → feedback+
Ok, I'll finish out the tests then have a DOM person look at it.
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
Attachment #8725110 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9e40f1763014
Status: NEW → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
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).
Now Nightly force https on http YouTube embeded movies? Nightly must force https, because without this html5 http embeded movies not work/loading.
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.
Attachment #8725110 - Flags: approval-mozilla-aurora?
(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.
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.
Flags: needinfo?(kyle)
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.
Flags: needinfo?(kyle)
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.
Attachment #8725110 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
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"
Tested on latest Nightly
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?
Flags: needinfo?(kyle)
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.
Flags: needinfo?(kyle)
OK. Doing nothing sounds good to me. :)
Status: RESOLVED → VERIFIED
(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.
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>
(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.
(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?
Blocks: 1258053
I filed bug 1258053 to continue the discussion.
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?
Beta 46 is unaffected, because youtube rewriting was disabled in bug 1255918
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.