Fix rare crash in WebstoreInlineInstaller::CheckInlineInstallPermitted.

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..816022adf34ec9e1e55691eaf1aaef1c9b521733 100644
--- a/chrome/browser/extensions/webstore_inline_installer.cc
+++ b/chrome/browser/extensions/webstore_inline_installer.cc
@@ -30,6 +30,7 @@ const char kInitiatedFromPopupError[] =
     "Inline installs can not be initiated from pop-up windows.";
 const char kInitiatedFromFullscreenError[] =
     "Inline installs can not be initiated from fullscreen.";
+const char kInternalError[] = "Unable to complete installation.";
 
 WebstoreInlineInstaller::WebstoreInlineInstaller(
     content::WebContents* web_contents,
@@ -136,7 +137,10 @@ bool WebstoreInlineInstaller::CheckInlineInstallPermitted(
     const base::DictionaryValue& webstore_data,
     std::string* error) const {
   Browser* browser = chrome::FindBrowserWithWebContents(web_contents());
-  DCHECK(browser);
+  if (!browser) {

[Devlin, 2017/01/31 20:18:47]

Hmm... interesting.  Do you know when this might happen?  Before calling
CheckInlineInstallPermitted(), we call CheckRequestorAlive() (which validates
web_contents()) and early-outs if it's not.  Is there another case in which
FindBrowserWithWebContents() could return null?  I wonder if we could catch this
case earlier, rather than waiting for the round-trip from the webstore?

[robertshield, 2017/02/01 14:21:32]

Yes, I believe in this case, web_contents() is not null, but the containing tab
has been stripped out of the tab_strip_model used by the TabContentsIterator
used by FindBrowserWithWebContents
(https://codesearch.chromium.org/chromium/src/chrome/browser/ui/tab_contents/t...).


I haven't dug very far into exactly when things get pruned out of the
tab_strip_model_, happens during window closing though and it can happen
slightly before the WebContents gets destroyed as per
https://codesearch.chromium.org/chromium/src/chrome/browser/ui/tabs/tab_strip...

Checking before the webstore response comes in is an interesting idea, but I'm
not sure that the WebContents still being attached to a Browser before the
request goes out necessarily guarantees it will still be attached to a Browser
when the response has come in.

[Devlin, 2017/02/06 16:28:41]

Hmm, okay.  What about merging this logic with CheckRequestorAlive()?  I think
for all intents and purposes, if the web contents is no longer attached to a
browser, we don't want to continue.  Then the check in
WebstoreStandaloneInstaller() for CheckRequestorAlive() would catch this case.

[robertshield, 2017/02/10 15:25:23]

That does seem better. Made it so. Apologies for the delay, I took a stab at a
browser_test but couldn't figure out a non-insanely-invasive way of writing one.


Let me know if this is ok as is. 

+    *error = kInternalError;
+    return false;
+  }
   if (browser->is_type_popup()) {
     *error = kInitiatedFromPopupError;
     return false;