Harden DisplayItemList in release mode not to crash on cached display item failures.

Harden DisplayItemList in release mode not to crash on cached display item failures.

Experience shows that we have difficult corner cases where we cannot guarantee yet that we have perfect paint invalidation, cache checking and double-painting. Failures in any of them will sometimes lead to a crash without this CL.

BUG=497614
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197383

[chrishtr, 2015-06-17 23:34:31]

chrishtr@chromium.org changed reviewers:
+ pdr@chromium.org, wangxianzhu@chromium.org

[Xianzhu, 2015-06-18 00:03:27]

https://codereview.chromium.org/1192613003/diff/1/Source/platform/graphics/pa...
File Source/platform/graphics/paint/DisplayItemList.cpp (right):

https://codereview.chromium.org/1192613003/diff/1/Source/platform/graphics/pa...
Source/platform/graphics/paint/DisplayItemList.cpp:272: ASSERT_NOT_REACHED();
We should put a 'continue;' after the above statement, otherwise we'll crash at
line 281.

https://codereview.chromium.org/1192613003/diff/1/Source/platform/graphics/pa...
Source/platform/graphics/paint/DisplayItemList.cpp:278: if (foundIt !=
currentEnd)
Would it be better to put this 'if' and the 'if' at line 268 into an 'if-else'?

[pdr., 2015-06-18 00:05:46]

Can we hold off on this for a few days? We had ~2k crashes in canary in the past
24h and our inflight patches may only fix some of them. Delaying this just a
little longer will let us verify that we get all of the crashes solved.

[chrishtr, 2015-06-18 00:08:53]

https://codereview.chromium.org/1192613003/diff/1/Source/platform/graphics/pa...
File Source/platform/graphics/paint/DisplayItemList.cpp (right):

https://codereview.chromium.org/1192613003/diff/1/Source/platform/graphics/pa...
Source/platform/graphics/paint/DisplayItemList.cpp:272: ASSERT_NOT_REACHED();
On 2015/06/18 at 00:03:26, Xianzhu wrote:
> We should put a 'continue;' after the above statement, otherwise we'll crash
at line 281.

Done.

https://codereview.chromium.org/1192613003/diff/1/Source/platform/graphics/pa...
Source/platform/graphics/paint/DisplayItemList.cpp:278: if (foundIt !=
currentEnd)
On 2015/06/18 at 00:03:26, Xianzhu wrote:
> Would it be better to put this 'if' and the 'if' at line 268 into an
'if-else'?

It's inside an ifndef NDEBUG block, prefer to keep this separate.

[chrishtr, 2015-06-18 17:02:56]

I think we should commit this, and consider reverting it once we have fixed the
current batch of bugs.

[chrishtr, 2015-06-18 17:03:01]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-06-18 17:03:27]

The CQ bit was unchecked by chrishtr@chromium.org

[chrishtr, 2015-06-18 17:03:42]

On 2015/06/18 at 17:02:56, chrishtr wrote:
> I think we should commit this, and consider reverting it once we have fixed
the current batch of bugs.

Xianzhu, pdr: need your review/opinion.

[pdr., 2015-06-18 17:32:50]

On 2015/06/18 at 17:03:42, chrishtr wrote:
> On 2015/06/18 at 17:02:56, chrishtr wrote:
> > I think we should commit this, and consider reverting it once we have fixed
the current batch of bugs.
> 
> Xianzhu, pdr: need your review/opinion.

I'm okay with that. LGTM here, but please wait for Xianzhu's opinion.

[Xianzhu, 2015-06-18 17:37:21]

On 2015/06/18 17:03:42, chrishtr wrote:
> On 2015/06/18 at 17:02:56, chrishtr wrote:
> > I think we should commit this, and consider reverting it once we have fixed
> the current batch of bugs.
> 
> Xianzhu, pdr: need your review/opinion.

The CL looks good, but I prefer pdr@'s opinion in #4.

I think you are planning this:
 Canary v1: Submit this CL
 Canary v2: Fix current batch of bugs
 Canary v3: Revert this CL
With this plan, we might see the crash rate drops after v1, and perhaps no
change after v2. This seems to hide the remaining issues (if any) until v3.

If we don't submit this CL in v1, we'll see if there are still remaining issues
after v2. So I prefer to hold off on this.

[chrishtr, 2015-06-18 17:39:16]

On 2015/06/18 at 17:37:21, wangxianzhu wrote:
> On 2015/06/18 17:03:42, chrishtr wrote:
> > On 2015/06/18 at 17:02:56, chrishtr wrote:
> > > I think we should commit this, and consider reverting it once we have
fixed
> > the current batch of bugs.
> > 
> > Xianzhu, pdr: need your review/opinion.
> 
> The CL looks good, but I prefer pdr@'s opinion in #4.
> 
> I think you are planning this:
>  Canary v1: Submit this CL
>  Canary v2: Fix current batch of bugs
>  Canary v3: Revert this CL
> With this plan, we might see the crash rate drops after v1, and perhaps no
change after v2. This seems to hide the remaining issues (if any) until v3.
> 
> If we don't submit this CL in v1, we'll see if there are still remaining
issues after v2. So I prefer to hold off on this.

Ok let's wait to see if the crash rate drops to an acceptable value in v1 with
the merges performed yesterday. If not, we'll have to commit this to unblock dev
channel for other features.

[Xianzhu, 2015-06-18 17:53:04]

On 2015/06/18 17:39:16, chrishtr wrote:
> On 2015/06/18 at 17:37:21, wangxianzhu wrote:
> > On 2015/06/18 17:03:42, chrishtr wrote:
> > > On 2015/06/18 at 17:02:56, chrishtr wrote:
> > > > I think we should commit this, and consider reverting it once we have
> fixed
> > > the current batch of bugs.
> > > 
> > > Xianzhu, pdr: need your review/opinion.
> > 
> > The CL looks good, but I prefer pdr@'s opinion in #4.
> > 
> > I think you are planning this:
> >  Canary v1: Submit this CL
> >  Canary v2: Fix current batch of bugs
> >  Canary v3: Revert this CL
> > With this plan, we might see the crash rate drops after v1, and perhaps no
> change after v2. This seems to hide the remaining issues (if any) until v3.
> > 
> > If we don't submit this CL in v1, we'll see if there are still remaining
> issues after v2. So I prefer to hold off on this.
> 
> Ok let's wait to see if the crash rate drops to an acceptable value in v1 with
> the merges performed yesterday. If not, we'll have to commit this to unblock
dev
> channel for other features.

I overlooked the dev-blocking factor, and perhaps my sequence of v1 and v2 was
wrong. If we have already landed the fixes in v1, then I think we can land this
CL in v2 now. lgtm.

[chrishtr, 2015-06-18 17:54:08]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2015-06-18 17:54:32]

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

[commit-bot: I haz the power, 2015-06-18 17:58:42]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197383