Fix rare crash in WebstoreInlineInstaller::CheckInlineInstallPermitted.

Fix rare crash in WebstoreInlineInstaller::CheckInlineInstallPermitted.

It assumed that FindBrowserWithWebContents couldn't return nullptr.

BUG=687189
Review-Url: https://codereview.chromium.org/2669563003
Cr-Commit-Position: refs/heads/master@{#451512}
Committed: https://chromium.googlesource.com/chromium/src/+/5418d62d2fd3dc113874faa6f8b1417ba5257f30

[robertshield, 2017-01-31 15:30:51]

The CQ bit was checked by robertshield@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 15:31:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 16:18:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 16:18:40]

Dry run: This issue passed the CQ dry run.

[robertshield, 2017-01-31 18:57:15]

Potential fix for a rare-ish crash. This might be wrong and the error string is
probably unsatisfactory. Let me know if this is worthwhile or if something else
should happen instead :-)

[robertshield, 2017-01-31 18:57:36]

robertshield@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[robertshield, 2017-01-31 18:57:37]

Potential fix for a rare-ish crash. This might be wrong and the error string is
probably unsatisfactory. Let me know if this is worthwhile or if something else
should happen instead :-)

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

https://codereview.chromium.org/2669563003/diff/1/chrome/browser/extensions/w...
File chrome/browser/extensions/webstore_inline_installer.cc (right):

https://codereview.chromium.org/2669563003/diff/1/chrome/browser/extensions/w...
chrome/browser/extensions/webstore_inline_installer.cc:140: if (!browser) {
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:33]

https://codereview.chromium.org/2669563003/diff/1/chrome/browser/extensions/w...
File chrome/browser/extensions/webstore_inline_installer.cc (right):

https://codereview.chromium.org/2669563003/diff/1/chrome/browser/extensions/w...
chrome/browser/extensions/webstore_inline_installer.cc:140: if (!browser) {
On 2017/01/31 20:18:47, Devlin wrote:
> 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?

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]

https://codereview.chromium.org/2669563003/diff/1/chrome/browser/extensions/w...
File chrome/browser/extensions/webstore_inline_installer.cc (right):

https://codereview.chromium.org/2669563003/diff/1/chrome/browser/extensions/w...
chrome/browser/extensions/webstore_inline_installer.cc:140: if (!browser) {
On 2017/02/01 14:21:32, robertshield_slow_reviews wrote:
> On 2017/01/31 20:18:47, Devlin wrote:
> > 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?
> 
> 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.

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-09 02:46:49]

The CQ bit was checked by robertshield@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 02:47:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 06:00:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 06:00:44]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2669563003/diff/1/chrome/browser/extensions/w...
File chrome/browser/extensions/webstore_inline_installer.cc (right):

https://codereview.chromium.org/2669563003/diff/1/chrome/browser/extensions/w...
chrome/browser/extensions/webstore_inline_installer.cc:140: if (!browser) {
On 2017/02/06 16:28:41, Devlin (OOO feb 8 and feb 9) wrote:
> On 2017/02/01 14:21:32, robertshield_slow_reviews wrote:
> > On 2017/01/31 20:18:47, Devlin wrote:
> > > 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?
> > 
> > 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.
> 
> 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.

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.

[Devlin, 2017-02-18 02:00:06]

lgtm; sorry I missed the latest patch set (feel free to ping if it's more than a
day or so :))

[robertshield, 2017-02-18 03:24:04]

On 2017/02/18 02:00:06, Devlin wrote:
> lgtm; sorry I missed the latest patch set (feel free to ping if it's more than
a
> day or so :))

Oh hey, no worries, I got swamped and forgot about this too :-) Thanks for the
review!

[robertshield, 2017-02-18 03:24:06]

The CQ bit was checked by robertshield@chromium.org

[commit-bot: I haz the power, 2017-02-18 03:24:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-18 05:25:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-18 05:25:46]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[robertshield, 2017-02-19 03:42:51]

The CQ bit was checked by robertshield@chromium.org

[commit-bot: I haz the power, 2017-02-19 03:43:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-19 04:56:11]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487475771000100,
"parent_rev": "250263fa2e293a555c78479e00b5758c2e56d97e", "commit_rev":
"5418d62d2fd3dc113874faa6f8b1417ba5257f30"}

[commit-bot: I haz the power, 2017-02-19 04:56:48]

Description was changed from

==========
Fix rare crash in WebstoreInlineInstaller::CheckInlineInstallPermitted.

It assumed that FindBrowserWithWebContents couldn't return nullptr.

BUG=687189
==========

to

==========
Fix rare crash in WebstoreInlineInstaller::CheckInlineInstallPermitted.

It assumed that FindBrowserWithWebContents couldn't return nullptr.

BUG=687189

Review-Url: https://codereview.chromium.org/2669563003
Cr-Commit-Position: refs/heads/master@{#451512}
Committed:
https://chromium.googlesource.com/chromium/src/+/5418d62d2fd3dc113874faa6f8b1...
==========

[commit-bot: I haz the power, 2017-02-19 04:56:49]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/5418d62d2fd3dc113874faa6f8b1...