Include referrer chain with inline install requests.

chrome/browser/extensions/webstore_inline_installer.cc

Index: chrome/browser/extensions/webstore_inline_installer.cc
diff --git a/chrome/browser/extensions/webstore_inline_installer.cc b/chrome/browser/extensions/webstore_inline_installer.cc
index 9e992bcb64fc1bedc00a3d598de8f3acc041de6b..931023efe61a2de87f0ce51afcf192c7a5541762 100644
--- a/chrome/browser/extensions/webstore_inline_installer.cc
+++ b/chrome/browser/extensions/webstore_inline_installer.cc
@@ -4,11 +4,17 @@
 
 #include "chrome/browser/extensions/webstore_inline_installer.h"
 
+#include <utility>
+
+#include "base/json/json_writer.h"
 #include "base/strings/stringprintf.h"
+#include "base/values.h"
+#include "chrome/browser/extensions/webstore_data_fetcher.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/exclusive_access/fullscreen_controller.h"
 #include "content/public/browser/navigation_details.h"
+#include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/web_contents.h"
 
 using content::WebContents;
@@ -95,6 +101,40 @@ bool WebstoreInlineInstaller::IsRequestorPermitted(
   return true;
 }
 
+std::string WebstoreInlineInstaller::GetJsonPostData() {
+  content::WebContents* web_contents = GetWebContents();
+
+  if (web_contents) {

[Devlin, 2017/01/30 17:24:48]

Can |web_contents| be null?

If so, we should maybe document when, because it's not always intuitive.  Also,
I think this would be more readable as:

if (!web_contents)
  return std::string();

<lots of other stuff>

rather than

if (web_contents) {
  <lots of other stuff>
}

return std::string()

[robertshield, 2017/01/31 02:48:14]

Spelunking a bit: GetWebContents() returns web_contents() (I switched to using
web_contents() to be consistent with the rest of this file). 

web_contents() comes from contents::WebContentsObserver. Its value is stored in
WebstoreInlineInstaller's constructor and is passed by
WebstoreInlineInstallerFactory::CreateInstaller which gets its WebContents
instance from the TabHelper that owns the WebstoreInlineInstaller instance.
web_contents_ does get reset if the WebContents instance itself gets destroyed,
via a call to contents::WebContentsObserver::ResetWebContents().

So, afaict, web_contents_ should always be non-null except perhaps during
TabHelper destruction.

This code shouldn't be called destruction now since it's called synchronously
from inside a TabContents event handler. 

It potentially could if this gets refactored into something async (like the
first HistoryService-based implementation) at a later date and it's unlikely
that would be obvious during a refactor. 

Given the un-intuitiveness, I'd prefer to leave the check in with a brief
comment. Let me know what you think.
Done.

[robertshield, 2017/01/31 02:56:27]

Actually.. I spelunked some more. Looking at e.g. crash/f397a0e680000000, it
looks like there are cases where the other code in this file that does not check
for web_contents() being null crashes in the field every once in a while in what
I'm guessing is a race against window destruction. 

I'm going to leave the check in here and if you like, I can remove the DCHECK on
line 179 and change it to a run-time check in another CL.

+    content::NavigationController& navigation_controller =
+        web_contents->GetController();
+    content::NavigationEntry* navigation_entry =
+        navigation_controller.GetLastCommittedEntry();
+
+    if (navigation_entry) {
+      const std::vector<GURL>& redirect_urls =
+          navigation_entry->GetRedirectChain();
+
+      if (!redirect_urls.empty()) {
+        base::DictionaryValue dictionary;
+        dictionary.SetString("id", id());
+        dictionary.SetString("referrer", requestor_url_.spec());
+        std::unique_ptr<base::ListValue> redirect_chain =
+            base::MakeUnique<base::ListValue>();
+        for (const GURL& url : redirect_urls) {
+          redirect_chain->AppendString(url.spec());
+        }
+        dictionary.Set("redirect_chain", std::move(redirect_chain));
+
+        std::string json;
+        if (base::JSONWriter::Write(dictionary, &json))

[Devlin, 2017/01/30 17:24:48]

Could this fail?  If this is just to be cautious, maybe use a CHECK() instead so
we know if/when it crashes.

[robertshield, 2017/01/31 02:48:14]

Actually, Write() always DCHECKs before returning false (which should only
happen if the base::Value it gets passed is somehow mangled) so I'll skip the
check here.

+          return json;
+      }
+    }
+  }
+
+  return std::string();
+}
+
 bool WebstoreInlineInstaller::CheckRequestorAlive() const {
   // The frame or tab may have gone away - cancel installation in that case.
   return host_ != nullptr && web_contents() != nullptr;