Bug 1240471 - Possibly remove invalid queries and post console messages on youtube rewrite; r=khuey fb=cpeterson
authorKyle Machulis <kyle@nonpolynomial.com>
Thu, 25 Feb 2016 15:49:18 -0800
changeset 324604 9e40f176301408d83cc041537b6c5752965e3481
parent 324603 aad89b3b0eb1db2a026b40f7825e563c3d861bf4
child 324605 54c94f478468a647c7c971d92599a270cf6f8c96
push id1128
push userjlund@mozilla.com
push dateWed, 01 Jun 2016 01:31:59 +0000
treeherdermozilla-release@fe0d30de989d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskhuey
bugs1240471
milestone47.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1240471 - Possibly remove invalid queries and post console messages on youtube rewrite; r=khuey fb=cpeterson
dom/base/nsObjectLoadingContent.cpp
dom/base/nsObjectLoadingContent.h
dom/base/test/mochitest.ini
dom/base/test/test_bug1240471.html
dom/locales/en-US/chrome/dom/dom.properties
--- a/dom/base/nsObjectLoadingContent.cpp
+++ b/dom/base/nsObjectLoadingContent.cpp
@@ -35,16 +35,17 @@
 #include "nsIURL.h"
 #include "nsIWebNavigation.h"
 #include "nsIWebNavigationInfo.h"
 #include "nsIScriptChannel.h"
 #include "nsIBlocklistService.h"
 #include "nsIAsyncVerifyRedirectCallback.h"
 #include "nsIAppShell.h"
 #include "nsIXULRuntime.h"
+#include "nsIScriptError.h"
 
 #include "nsError.h"
 
 // Util headers
 #include "prenv.h"
 #include "mozilla/Logging.h"
 
 #include "nsAutoPtr.h"
@@ -1477,71 +1478,130 @@ nsObjectLoadingContent::CheckJavaCodebas
     LOG(("OBJLC [%p]: Java failed RelaxStrictFileOriginPolicy for file URI",
          this));
     return false;
   }
 
   return true;
 }
 
-bool
-nsObjectLoadingContent::ShouldRewriteYoutubeEmbed(nsIURI* aURI)
+void
+nsObjectLoadingContent::MaybeRewriteYoutubeEmbed(nsIURI* aURI, nsIURI* aBaseURI, nsIURI** aOutURI)
 {
   nsCOMPtr<nsIContent> thisContent =
     do_QueryInterface(static_cast<nsIImageLoadingContent*>(this));
   NS_ASSERTION(thisContent, "Must be an instance of content");
 
   // We're only interested in switching out embed and object tags
   if (!thisContent->NodeInfo()->Equals(nsGkAtoms::embed) &&
       !thisContent->NodeInfo()->Equals(nsGkAtoms::object)) {
-    return false;
+    return;
   }
+
   nsCOMPtr<nsIEffectiveTLDService> tldService =
     do_GetService(NS_EFFECTIVETLDSERVICE_CONTRACTID);
   // If we can't analyze the URL, just pass on through.
   if(!tldService) {
     NS_WARNING("Could not get TLD service!");
-    return false;
+    return;
   }
 
   nsAutoCString currentBaseDomain;
   bool ok = NS_SUCCEEDED(tldService->GetBaseDomain(aURI, 0, currentBaseDomain));
   if (!ok) {
     // Data URIs (commonly used for things like svg embeds) won't parse
     // correctly, so just fail silently here.
-    return false;
+    return;
   }
 
   // See if URL is referencing youtube
   if (!currentBaseDomain.EqualsLiteral("youtube.com")) {
-    return false;
+    return;
   }
 
   // We should only rewrite URLs with paths starting with "/v/", as we shouldn't
   // touch object nodes with "/embed/" urls that already do that right thing.
   nsAutoCString path;
   aURI->GetPath(path);
   if (!StringBeginsWith(path, NS_LITERAL_CSTRING("/v/"))) {
-    return false;
+    return;
   }
 
   // See if requester is planning on using the JS API.
   nsAutoCString uri;
   aURI->GetSpec(uri);
   if (uri.Find("enablejsapi=1", true, 0, -1) != kNotFound) {
     Telemetry::Accumulate(Telemetry::YOUTUBE_NONREWRITABLE_EMBED_SEEN, 1);
-    return false;
+    return;
+  }
+
+  // Some youtube urls have invalid query strings attached, e.g.
+  // http://youtube.com/embed/7LcUOEP7Brc&start=35. These URLs work with flash,
+  // but break iframe/object embedding. If this situation occurs with rewritten
+  // URLs, and the user has flash installed, just use flash. If the user does
+  // 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);
+  bool trimQuery = false;
+  if (ampIndex != -1) {
+    int32_t qmIndex = uri.FindChar('?', 0);
+    if (qmIndex == -1 ||
+        qmIndex > ampIndex) {
+      if (!nsContentUtils::IsSWFPlayerEnabled()) {
+        trimQuery = true;
+      } else {
+        // Flash is enabled, just use it in this case.
+        return;
+      }
+    }
   }
 
   // If we've made it this far, we've got a rewritable embed. Log it in
   // telemetry.
   Telemetry::Accumulate(Telemetry::YOUTUBE_REWRITABLE_EMBED_SEEN, 1);
 
-  // Even if node is rewritable, only rewrite if the pref tells us we should.
-  return Preferences::GetBool(kPrefYoutubeRewrite);
+  // If we're pref'd off, return after telemetry has been logged.
+  if (!Preferences::GetBool(kPrefYoutubeRewrite)) {
+    return;
+  }
+
+  nsAutoString utf16OldURI = NS_ConvertUTF8toUTF16(uri);
+  // 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);
+  }
+  // Switch out video access url formats, which should possibly allow HTML5
+  // video loading.
+  uri.ReplaceSubstring(NS_LITERAL_CSTRING("/v/"),
+                       NS_LITERAL_CSTRING("/embed/"));
+  nsAutoString utf16URI = NS_ConvertUTF8toUTF16(uri);
+  nsresult rv = nsContentUtils::NewURIWithDocumentCharset(aOutURI,
+                                                          utf16URI,
+                                                          thisContent->OwnerDoc(),
+                                                          aBaseURI);
+  if (NS_FAILED(rv)) {
+    return;
+  }
+  const char16_t* params[] = { utf16OldURI.get(), utf16URI.get() };
+  const char* msgName;
+  // If there's no query to rewrite, just notify in the developer console
+  // that we're changing the embed.
+  if (!trimQuery) {
+    msgName = "RewriteYoutubeEmbed";
+  } else {
+    msgName = "RewriteYoutubeEmbedInvalidQuery";
+  }
+  nsContentUtils::ReportToConsole(nsIScriptError::warningFlag,
+                                  NS_LITERAL_CSTRING("Plugins"),
+                                  thisContent->OwnerDoc(),
+                                  nsContentUtils::eDOM_PROPERTIES,
+                                  msgName,
+                                  params, ArrayLength(params));
 }
 
 bool
 nsObjectLoadingContent::CheckLoadPolicy(int16_t *aContentPolicy)
 {
   if (!aContentPolicy || !mURI) {
     NS_NOTREACHED("Doing it wrong");
     return false;
@@ -1789,25 +1849,22 @@ nsObjectLoadingContent::UpdateObjectPara
 
   // Note that the baseURI changing could affect the newURI, even if uriStr did
   // not change.
   if (!uriStr.IsEmpty()) {
     rv = nsContentUtils::NewURIWithDocumentCharset(getter_AddRefs(newURI),
                                                    uriStr,
                                                    thisContent->OwnerDoc(),
                                                    newBaseURI);
-    if (ShouldRewriteYoutubeEmbed(newURI)) {
-      // Switch out video access url formats, which should possibly allow HTML5
-      // video loading.
-      uriStr.ReplaceSubstring(NS_LITERAL_STRING("/v/"),
-                              NS_LITERAL_STRING("/embed/"));
-      rv = nsContentUtils::NewURIWithDocumentCharset(getter_AddRefs(newURI),
-                                                     uriStr,
-                                                     thisContent->OwnerDoc(),
-                                                     newBaseURI);
+    nsCOMPtr<nsIURI> rewrittenURI;
+    MaybeRewriteYoutubeEmbed(newURI,
+                             newBaseURI,
+                             getter_AddRefs(rewrittenURI));
+    if (rewrittenURI) {
+      newURI = rewrittenURI;
       newMime = NS_LITERAL_CSTRING("text/html");
     }
 
     if (NS_SUCCEEDED(rv)) {
       NS_TryToSetImmutable(newURI);
     } else {
       stateInvalid = true;
     }
--- a/dom/base/nsObjectLoadingContent.h
+++ b/dom/base/nsObjectLoadingContent.h
@@ -533,17 +533,19 @@ class nsObjectLoadingContent : public ns
      * could possibly be manipulating the youtube video elsewhere on the page
      * via javascript. We can't rewrite these kinds of elements without possibly
      * breaking content, which we want to avoid.
      *
      * If we can rewrite the URL, we change the "/v/" to "/embed/", and change
      * our type to eType_Document so that we render similarly to an iframe
      * embed.
      */
-    bool ShouldRewriteYoutubeEmbed(nsIURI* uri);
+    void MaybeRewriteYoutubeEmbed(nsIURI* aURI,
+                                  nsIURI* aBaseURI,
+                                  nsIURI** aRewrittenURI);
 
     // Helper class for SetupProtoChain
     class SetupProtoChainRunner final : public nsIRunnable
     {
       ~SetupProtoChainRunner();
     public:
       NS_DECL_ISUPPORTS
 
--- a/dom/base/test/mochitest.ini
+++ b/dom/base/test/mochitest.ini
@@ -867,8 +867,9 @@ skip-if = e10s || os != 'linux' || build
 [test_explicit_user_agent.html]
 [test_change_policy.html]
 skip-if = buildapp == 'b2g' #no ssl support
 [test_document.all_iteration.html]
 [test_bug1198095.html]
 [test_bug1187157.html]
 [test_bug769117.html]
 [test_bug1250148.html]
+[test_bug1240471.html]
\ No newline at end of file
new file mode 100644
--- /dev/null
+++ b/dom/base/test/test_bug1240471.html
@@ -0,0 +1,45 @@
+<!DOCTYPE HTML>
+<html>
+  <!--
+       https://bugzilla.mozilla.org/show_bug.cgi?id=1240471
+     -->
+  <head>
+    <meta charset="utf-8">
+    <title>Test for Bug 1240471</title>
+    <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+    <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+    <script type="application/javascript">
+     SimpleTest.waitForExplicitFinish();
+     "use strict";
+     function onLoad () {
+       let youtube_changed_url_noquery = "https://mochitest.youtube.com/embed/Xm5i5kbIXzc";
+       let youtube_changed_url_query = "https://mochitest.youtube.com/embed/Xm5i5kbIXzc?start=10&end=20";
+
+       function testEmbed(embed, expected_url) {
+         ok (embed, "Embed node exists");
+         embed = SpecialPowers.wrap(embed);
+         is (embed.srcURI.spec, expected_url, "Should have src uri of " + expected_url);
+       }
+       info("Running youtube rewrite query test");
+       testEmbed(document.getElementById("testembed-correct"), youtube_changed_url_query);
+       testEmbed(document.getElementById("testembed-wrong"), youtube_changed_url_noquery);
+       testEmbed(document.getElementById("testembed-whywouldyouevendothat"), youtube_changed_url_noquery);
+       SimpleTest.finish();
+     }
+    </script>
+  </head>
+  <body onload="onLoad()">
+    <embed id="testembed-correct"
+           src="https://mochitest.youtube.com/v/Xm5i5kbIXzc?start=10&end=20"
+           type="application/x-shockwave-flash"
+           allowscriptaccess="always"></embed>
+    <embed id="testembed-wrong"
+           src="https://mochitest.youtube.com/v/Xm5i5kbIXzc&start=10&end=20"
+           type="application/x-shockwave-flash"
+           allowscriptaccess="always"></embed>
+    <embed id="testembed-whywouldyouevendothat"
+           src="https://mochitest.youtube.com/v/Xm5i5kbIXzc&start=10?end=20"
+           type="application/x-shockwave-flash"
+           allowscriptaccess="always"></embed>
+  </body>
+</html>
--- a/dom/locales/en-US/chrome/dom/dom.properties
+++ b/dom/locales/en-US/chrome/dom/dom.properties
@@ -196,8 +196,12 @@ ManifestStartURLInvalid=The start URL is
 ManifestStartURLShouldBeSameOrigin=The start URL must be same origin as document.
 # LOCALIZATION NOTE: %1$S is the name of the object whose property is invalid. %2$S is the name of the invalid property. %3$S is the expected type of the property value. E.g. "Expected the manifest's start_url member to be a string."
 ManifestInvalidType=Expected the %1$S's %2$S member to be a %3$S.
 # LOCALIZATION NOTE: %1$S is the name of the property whose value is invalid. %2$S is the (invalid) value of the property. E.g. "theme_color: 42 is not a valid CSS color."
 ManifestInvalidCSSColor=%1$S: %2$S is not a valid CSS color.
 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 Flash 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 Flash 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.