NTP thumbnails: Always pass singular "thumbnailUrl" to the iframe

chrome/renderer/searchbox/searchbox_extension.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/renderer/searchbox/searchbox_extension.h"
 
 #include <stddef.h>
 #include <stdint.h>
 #include <string>
 #include <vector>
@@ skipping 90 matching lines @@
     v8::Isolate* isolate,
     int render_view_id,
     InstantRestrictedID most_visited_item_id) {
   return UTF8ToV8String(
       isolate,
       base::StringPrintf(
           "chrome-search://thumb/%d/%d", render_view_id, most_visited_item_id));
 }
 
 v8::Local<v8::String> GenerateThumb2URL(v8::Isolate* isolate,
-                                        const std::string& url) {
-  return UTF8ToV8String(
-      isolate, base::StringPrintf("chrome-search://thumb2/%s", url.c_str()));
+                                        const GURL& page_url,
+                                        const GURL& fallback_thumb_url) {
+  return UTF8ToV8String(isolate,
+                        base::StringPrintf("chrome-search://thumb2/%s?fb=%s",

[sfiera, 2017/03/30 13:40:58]

No url-escaping?

[Marc Treib, 2017/03/30 13:51:03]

Nope :-/

Example of what these URLs look like (from the current live remote NTP):
chrome-search://thumb2/https://codereview.chromium.org/?fb=https://www.google.com/webpagethumbnail?c=63&d=https://codereview.chromium.org/&r=4&s=148:94&a=<someID>

The code that handles these is here:
https://cs.chromium.org/chromium/src/chrome/browser/search/thumbnail_source.c...

And the corresponding code for the remote NTP: newtab/design.js:202

[sfiera, 2017/03/30 13:55:19]

Ah, so any tile with ?fb= in its URL is probably already broken…

[Marc Treib, 2017/03/30 14:04:26]

Argh, yes - I didn't even notice. Though I think we strip params from
TopSites/ML URLs, so it shouldn't actually happen in practice. Still, yes, this
is broken, like so many things...

+                                           page_url.spec().c_str(),
+                                           fallback_thumb_url.spec().c_str()));
 }
 
 // Populates a Javascript MostVisitedItem object from |mv_item|.
 // NOTE: Includes "url", "title" and "domain" which are private data, so should
 // not be returned to the Instant page. These should be erased before returning
 // the object. See GetMostVisitedItemsWrapper() in searchbox_api.js.
 v8::Local<v8::Object> GenerateMostVisitedItem(
     v8::Isolate* isolate,
     int render_view_id,
     InstantRestrictedID restricted_id,
@@ skipping 18 matching lines @@
   base::string16 title = mv_item.title;
   if (title.empty())
     title = base::UTF8ToUTF16(mv_item.url.spec());
 
   v8::Local<v8::Object> obj = v8::Object::New(isolate);
   obj->Set(v8::String::NewFromUtf8(isolate, "renderViewId"),
            v8::Int32::New(isolate, render_view_id));
   obj->Set(v8::String::NewFromUtf8(isolate, "rid"),
            v8::Int32::New(isolate, restricted_id));
 
-  // If the suggestion already has a suggested thumbnail, we create an thumbnail
-  // array with both the local thumbnail and the proposed one.
-  // Otherwise, we just create an array with the generated one.
-  if (!mv_item.thumbnail.spec().empty()) {
-    v8::Local<v8::Array> thumbs = v8::Array::New(isolate, 2);
-    // Note: The "thumb2" source captures a thumbnail on the next visit.
-    thumbs->Set(0, GenerateThumb2URL(isolate, mv_item.url.spec()));
-    thumbs->Set(1, UTF8ToV8String(isolate, mv_item.thumbnail.spec()));
-    obj->Set(v8::String::NewFromUtf8(isolate, "thumbnailUrls"), thumbs);
-  } else {
-    v8::Local<v8::Array> thumbs = v8::Array::New(isolate, 1);
-    thumbs->Set(0,
-                GenerateThumbnailURL(isolate, render_view_id, restricted_id));
-    obj->Set(v8::String::NewFromUtf8(isolate, "thumbnailUrls"), thumbs);
-  }
+  // If the suggestion already has a suggested thumbnail, we create a thumbnail
+  // URL with both the local thumbnail and the proposed one as a fallback.
+  // Otherwise, we just pass on the generated one.
+  obj->Set(v8::String::NewFromUtf8(isolate, "thumbnailUrl"),
+           mv_item.thumbnail.is_valid()
+               ? GenerateThumb2URL(isolate, mv_item.url, mv_item.thumbnail)
+               : GenerateThumbnailURL(isolate, render_view_id, restricted_id));
 
   // If the suggestion already has a favicon, we populate the element with it.
   if (!mv_item.favicon.spec().empty()) {
     obj->Set(v8::String::NewFromUtf8(isolate, "faviconUrl"),
              UTF8ToV8String(isolate, mv_item.favicon.spec()));
   }
 
   obj->Set(v8::String::NewFromUtf8(isolate, "tileSource"),
            v8::Integer::New(isolate, static_cast<int>(mv_item.source)));
   obj->Set(v8::String::NewFromUtf8(isolate, "title"),
@@ skipping 921 matching lines @@
     ThrowInvalidParameters(args);
     return;
   }
 
   DVLOG(1) << render_frame << " UndoMostVisitedDeletion";
   SearchBox::Get(render_frame)
       ->UndoMostVisitedDeletion(args[0]->ToInteger()->Value());
 }
 
 }  // namespace extensions_v8