Make sure RecordingImageBufferSurface's current frame is valid

Make sure RecordingImageBufferSurface's current frame is valid

The "FallbackReasonUnknown" should not happen. Right now it could still
occur in RecordingImageBufferSurface::finalizeFrameInternal when the
m_currentFrame is null (theoretically).
In this CL, we change ASSERT to CHECK, to make sure that the
m_currentFrame is never null in the finalizeFrameInternal() method.

BUG=585578
Committed: https://crrev.com/9c8483972fea54016b0cf05e5e50fb8cba47248d
Cr-Commit-Position: refs/heads/master@{#403707}

[xidachen, 2016-07-04 13:42:20]

xidachen@chromium.org changed reviewers:
+ isherman@chromium.org, junov@chromium.org

[xidachen, 2016-07-04 13:42:21]

PTAL

[Justin Novosad, 2016-07-04 14:01:05]

https://codereview.chromium.org/2115493004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.cpp
(right):

https://codereview.chromium.org/2115493004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.cpp:274:
*fallbackReason = FallbackReasonCurrentFrameNull;
I don't think this is right. If m_currentFrame is not set, it means that a
fallback has already occured, in which case finalizeFrameInternal should not be
called (ASSERTs above check that).  If this code is reachable, we might be in an
invalid state.  Do you have a repro case that exercises this case?

[xidachen, 2016-07-04 14:06:36]

https://codereview.chromium.org/2115493004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.cpp
(right):

https://codereview.chromium.org/2115493004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.cpp:274:
*fallbackReason = FallbackReasonCurrentFrameNull;
On 2016/07/04 14:01:05, Justin Novosad wrote:
> I don't think this is right. If m_currentFrame is not set, it means that a
> fallback has already occured, in which case finalizeFrameInternal should not
be
> called (ASSERTs above check that).  If this code is reachable, we might be in
an
> invalid state.  Do you have a repro case that exercises this case?

I don't have a repro case. But from the histogram of canary channel, we still
see 0.2% that is Unkonwn. This seems to be the only place that could happen.
Could this happen when browser crashes, or some un-expected behavior?

[Justin Novosad, 2016-07-04 14:16:57]

https://codereview.chromium.org/2115493004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.cpp
(right):

https://codereview.chromium.org/2115493004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.cpp:274:
*fallbackReason = FallbackReasonCurrentFrameNull;
On 2016/07/04 14:06:36, xidachen wrote:
> On 2016/07/04 14:01:05, Justin Novosad wrote:
> > I don't think this is right. If m_currentFrame is not set, it means that a
> > fallback has already occured, in which case finalizeFrameInternal should not
> be
> > called (ASSERTs above check that).  If this code is reachable, we might be
in
> an
> > invalid state.  Do you have a repro case that exercises this case?
> 
> I don't have a repro case. But from the histogram of canary channel, we still
> see 0.2% that is Unkonwn. This seems to be the only place that could happen.
> Could this happen when browser crashes, or some un-expected behavior?

Let's do an experiment on the canary channel then: Change the two first ASSERTs
at the top of this method into "CHECK", and right here, do:
CHECK(m_currentFrame);
return true;

[xidachen, 2016-07-04 14:35:07]

Description was changed from

==========
Add a new categories in DisplayListFallbackReason

The "FallbackReasonUnknown" should not happen. Right now it could still
occur in RecordingImageBufferSurface::finalizeFrameInternal when the
m_currentFrame is null. This CL adds a new category in the enum to
handle that case.

BUG=585578
==========

to

==========
Make sure RecordingImageBufferSurface's current frame is valid

The "FallbackReasonUnknown" should not happen. Right now it could still
occur in RecordingImageBufferSurface::finalizeFrameInternal when the
m_currentFrame is null (theoretically).
In this CL, we change ASSERT to CHECK, to make sure that the
m_currentFrame is never null in the finalizeFrameInternal() method.

BUG=585578
==========

[xidachen, 2016-07-04 14:35:17]

xidachen@chromium.org changed reviewers:
- isherman@chromium.org

[xidachen, 2016-07-04 14:36:37]

https://codereview.chromium.org/2115493004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.cpp
(right):

https://codereview.chromium.org/2115493004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.cpp:274:
*fallbackReason = FallbackReasonCurrentFrameNull;
On 2016/07/04 14:16:57, Justin Novosad wrote:
> On 2016/07/04 14:06:36, xidachen wrote:
> > On 2016/07/04 14:01:05, Justin Novosad wrote:
> > > I don't think this is right. If m_currentFrame is not set, it means that a
> > > fallback has already occured, in which case finalizeFrameInternal should
not
> > be
> > > called (ASSERTs above check that).  If this code is reachable, we might be
> in
> > an
> > > invalid state.  Do you have a repro case that exercises this case?
> > 
> > I don't have a repro case. But from the histogram of canary channel, we
still
> > see 0.2% that is Unkonwn. This seems to be the only place that could happen.
> > Could this happen when browser crashes, or some un-expected behavior?
> 
> Let's do an experiment on the canary channel then: Change the two first
ASSERTs
> at the top of this method into "CHECK", and right here, do:
> CHECK(m_currentFrame);
> return true;
> 

Okay. Let's do the experiment then. I remove the category in the histogram and
replace the ASSERTs by CHECKs. My understanding is that CHECK won't do anything
if it is not hit, is that correct?

[Justin Novosad, 2016-07-04 14:38:55]

https://codereview.chromium.org/2115493004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.cpp
(right):

https://codereview.chromium.org/2115493004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.cpp:261:
ASSERT(!m_fallbackSurface);
I meant this one.

https://codereview.chromium.org/2115493004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.cpp:274:
return m_currentFrame.get();
tautology: just return true here.

[xidachen, 2016-07-04 14:43:36]

https://codereview.chromium.org/2115493004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.cpp
(right):

https://codereview.chromium.org/2115493004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.cpp:261:
ASSERT(!m_fallbackSurface);
On 2016/07/04 14:38:55, Justin Novosad wrote:
> I meant this one.

Done.

https://codereview.chromium.org/2115493004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.cpp:274:
return m_currentFrame.get();
On 2016/07/04 14:38:55, Justin Novosad wrote:
> tautology: just return true here.

Done.

[Justin Novosad, 2016-07-04 17:35:44]

On 2016/07/04 14:43:37, xidachen wrote:

lgtm

[xidachen, 2016-07-04 17:36:31]

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

[commit-bot: I haz the power, 2016-07-04 17:36:51]

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, 2016-07-04 18:48:05]

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

[commit-bot: I haz the power, 2016-07-04 18:48:06]

Dry run: This issue passed the CQ dry run.

[xidachen, 2016-07-04 18:50:10]

The CQ bit was checked by xidachen@chromium.org

[commit-bot: I haz the power, 2016-07-04 18:50:15]

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, 2016-07-04 18:54:04]

Description was changed from

==========
Make sure RecordingImageBufferSurface's current frame is valid

The "FallbackReasonUnknown" should not happen. Right now it could still
occur in RecordingImageBufferSurface::finalizeFrameInternal when the
m_currentFrame is null (theoretically).
In this CL, we change ASSERT to CHECK, to make sure that the
m_currentFrame is never null in the finalizeFrameInternal() method.

BUG=585578
==========

to

==========
Make sure RecordingImageBufferSurface's current frame is valid

The "FallbackReasonUnknown" should not happen. Right now it could still
occur in RecordingImageBufferSurface::finalizeFrameInternal when the
m_currentFrame is null (theoretically).
In this CL, we change ASSERT to CHECK, to make sure that the
m_currentFrame is never null in the finalizeFrameInternal() method.

BUG=585578
==========

[commit-bot: I haz the power, 2016-07-04 18:54:05]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-04 18:54:07]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-04 18:56:04]

Description was changed from

==========
Make sure RecordingImageBufferSurface's current frame is valid

The "FallbackReasonUnknown" should not happen. Right now it could still
occur in RecordingImageBufferSurface::finalizeFrameInternal when the
m_currentFrame is null (theoretically).
In this CL, we change ASSERT to CHECK, to make sure that the
m_currentFrame is never null in the finalizeFrameInternal() method.

BUG=585578
==========

to

==========
Make sure RecordingImageBufferSurface's current frame is valid

The "FallbackReasonUnknown" should not happen. Right now it could still
occur in RecordingImageBufferSurface::finalizeFrameInternal when the
m_currentFrame is null (theoretically).
In this CL, we change ASSERT to CHECK, to make sure that the
m_currentFrame is never null in the finalizeFrameInternal() method.

BUG=585578

Committed: https://crrev.com/9c8483972fea54016b0cf05e5e50fb8cba47248d
Cr-Commit-Position: refs/heads/master@{#403707}
==========

[commit-bot: I haz the power, 2016-07-04 18:56:05]

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