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

chrome/renderer/searchbox/searchbox_extension.cc

Index: chrome/renderer/searchbox/searchbox_extension.cc
diff --git a/chrome/renderer/searchbox/searchbox_extension.cc b/chrome/renderer/searchbox/searchbox_extension.cc
index 5cd4a6e2cba75c76c59e341f1b14aca3e3aa9845..aa34ac68909f94fffa737ab42b2dc363909d6c39 100644
--- a/chrome/renderer/searchbox/searchbox_extension.cc
+++ b/chrome/renderer/searchbox/searchbox_extension.cc
@@ -108,9 +108,12 @@ v8::Local<v8::String> GenerateThumbnailURL(
 }
 
 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|.
@@ -149,21 +152,13 @@ v8::Local<v8::Object> GenerateMostVisitedItem(
   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()) {