Null checks to prevent occasional crash

Null checks to prevent occasional crash

BUG=423229
Committed: https://crrev.com/0794ea8df9793f17bc6efff92f2960cf9350b5f7
Cr-Commit-Position: refs/heads/master@{#302137}

[Mike Lerman, 2014-10-28 16:59:47]

mlerman@chromium.org changed reviewers:
+ pkasting@chromium.org

[Mike Lerman, 2014-10-28 16:59:48]

Hi Peter,

I'm adding null checks to prevent occasional reported crashes. Can you please
review? Thanks!

Mike

[Peter Kasting, 2014-10-29 18:07:01]

https://codereview.chromium.org/686803003/diff/1/chrome/browser/ui/browser_li...
File chrome/browser/ui/browser_list.cc (right):

https://codereview.chromium.org/686803003/diff/1/chrome/browser/ui/browser_li...
chrome/browser/ui/browser_list.cc:167: if (*it && (*it)->window())
Can both these be NULL, or just one?

Why do we need checks here but not above?

Is there a precise scenario that causes NULLs that we can describe in a comment
here explaining why the NULL checks are necessary?

[Mike Lerman, 2014-10-29 18:26:06]

https://codereview.chromium.org/686803003/diff/1/chrome/browser/ui/browser_li...
File chrome/browser/ui/browser_list.cc (right):

https://codereview.chromium.org/686803003/diff/1/chrome/browser/ui/browser_li...
chrome/browser/ui/browser_list.cc:167: if (*it && (*it)->window())
On 2014/10/29 18:07:01, Peter Kasting wrote:
> Can both these be NULL, or just one?
> 
> Why do we need checks here but not above?
> 
> Is there a precise scenario that causes NULLs that we can describe in a
comment
> here explaining why the NULL checks are necessary?

I don't know what scenario is causing the NULLs unfortunately - we just have a
crash report, no steps to reproduce. I can't think of what could cause this.
Perhaps contrived javascript, such as an OnBeforeUnload that closes a browser
window, though I don't think you can do that.

The first *it check may not be necessary, but since I don't have steps to
reproduce I'm just playing it safe. My intuition is that the window() is what's
null, though, and can only check (*it)->window() if you prefer.

[Peter Kasting, 2014-10-29 18:34:41]

On 2014/10/29 18:26:06, Mike Lerman wrote:
>
https://codereview.chromium.org/686803003/diff/1/chrome/browser/ui/browser_li...
> File chrome/browser/ui/browser_list.cc (right):
> 
>
https://codereview.chromium.org/686803003/diff/1/chrome/browser/ui/browser_li...
> chrome/browser/ui/browser_list.cc:167: if (*it && (*it)->window())
> On 2014/10/29 18:07:01, Peter Kasting wrote:
> > Can both these be NULL, or just one?
> > 
> > Why do we need checks here but not above?
> > 
> > Is there a precise scenario that causes NULLs that we can describe in a
> comment
> > here explaining why the NULL checks are necessary?
> 
> I don't know what scenario is causing the NULLs unfortunately - we just have a
> crash report, no steps to reproduce. I can't think of what could cause this.
> Perhaps contrived javascript, such as an OnBeforeUnload that closes a browser
> window, though I don't think you can do that.

Is there a way we can add more CHECKs here and at other places along the call
chain to understand when/how there can be NULLs here?  I don't want us to ever
have null-checks in our code that we don't understand how/why they can be hit.

[Mike Lerman, 2014-10-29 20:45:49]

On 2014/10/29 18:34:41, Peter Kasting wrote:
> On 2014/10/29 18:26:06, Mike Lerman wrote:
> >
>
https://codereview.chromium.org/686803003/diff/1/chrome/browser/ui/browser_li...
> > File chrome/browser/ui/browser_list.cc (right):
> > 
> >
>
https://codereview.chromium.org/686803003/diff/1/chrome/browser/ui/browser_li...
> > chrome/browser/ui/browser_list.cc:167: if (*it && (*it)->window())
> > On 2014/10/29 18:07:01, Peter Kasting wrote:
> > > Can both these be NULL, or just one?
> > > 
> > > Why do we need checks here but not above?
> > > 
> > > Is there a precise scenario that causes NULLs that we can describe in a
> > comment
> > > here explaining why the NULL checks are necessary?
> > 
> > I don't know what scenario is causing the NULLs unfortunately - we just have
a
> > crash report, no steps to reproduce. I can't think of what could cause this.
> > Perhaps contrived javascript, such as an OnBeforeUnload that closes a
browser
> > window, though I don't think you can do that.
> 
> Is there a way we can add more CHECKs here and at other places along the call
> chain to understand when/how there can be NULLs here?  I don't want us to ever
> have null-checks in our code that we don't understand how/why they can be hit.

Added a DCHECK that may help us figure out if the window accidentally becomes
NULL. Since this is an M39 issue I lean towards leaving the if() that avoids
crashes in stable, but leaving in DCHECKs so that we try and find the issue.

[Mike Lerman, 2014-10-29 20:45:56]

Patchset #2 (id:20001) has been deleted

[Peter Kasting, 2014-10-30 03:07:50]

https://codereview.chromium.org/686803003/diff/40001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_list.cc (right):

https://codereview.chromium.org/686803003/diff/40001/chrome/browser/ui/browse...
chrome/browser/ui/browser_list.cc:156: DCHECK((*it)->window());
I would change these two DCHECKs to CHECKs if you're trying to diagnose an
issue, since most users don't have DCHECKs on.

Plus, CHECKs obviate the need for a conditional -- you will either pass the
CHECK or crash.

We can then take action fairly quickly in response to a CHECK, so we don't have
huge numbers of users hitting it.

[Mike Lerman, 2014-10-30 18:40:58]

On 2014/10/30 03:07:50, Peter Kasting wrote:
>
https://codereview.chromium.org/686803003/diff/40001/chrome/browser/ui/browse...
> File chrome/browser/ui/browser_list.cc (right):
> 
>
https://codereview.chromium.org/686803003/diff/40001/chrome/browser/ui/browse...
> chrome/browser/ui/browser_list.cc:156: DCHECK((*it)->window());
> I would change these two DCHECKs to CHECKs if you're trying to diagnose an
> issue, since most users don't have DCHECKs on.
> 
> Plus, CHECKs obviate the need for a conditional -- you will either pass the
> CHECK or crash.
> 
> We can then take action fairly quickly in response to a CHECK, so we don't
have
> huge numbers of users hitting it.

Ok. I have concerns that the CHECKs may not explain everything and we'll still
need to have a conditional due to some underlying race condition, and M39's
moving out soon-ish. But I shall defer to your experience, we'll push this code
out with the next M39 Devs and Betas, and hopefully learn enough from upcoming
crash reports to solve the problem better before we go stable.

[Peter Kasting, 2014-10-30 18:47:43]

LGTM; you're welcome to add other CHECKs other places if you have ideas on what
might be going wrong here, so you can figure this out in fewer iterations of
shipping patches.

[Mike Lerman, 2014-10-30 18:49:20]

On 2014/10/30 18:47:43, Peter Kasting wrote:
> LGTM; you're welcome to add other CHECKs other places if you have ideas on
what
> might be going wrong here, so you can figure this out in fewer iterations of
> shipping patches.

I don't, particularly :(

Thanks!

[Mike Lerman, 2014-10-30 18:49:24]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-10-30 18:50:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/686803003/60001

[commit-bot: I haz the power, 2014-10-30 19:55:01]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2014-10-30 19:56:06]

Patchset 3 (id:??) landed as
https://crrev.com/0794ea8df9793f17bc6efff92f2960cf9350b5f7
Cr-Commit-Position: refs/heads/master@{#302137}