Fix ASSERT(m_canvas) failures when contextDisabled()

Fix ASSERT(m_canvas) failures when contextDisabled()

contextDisabled() is for performance tests to let GraphicsContext to
draw nothing. In either slimming-paint mode or non-slimming-paint mode,
we should check it at beginning of every drawing method of
GraphicsContext to ensure the performance when contextDisabled() is
as expected.

Especially in slimming-paint mode, missing the check causes null
dereference of m_canvas or ASSERT(m_canvas) failure.

- Ensure contextDisabled() is checked in all drawing methods of
  GraphicsContext;
- Use SkNullCanvas when contextDisabled() to ensure drawings directly
  using GraphicsContext::canvas() will not encounter null canvas and
  be also controled by contextDisabled().

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

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

[Xianzhu, 2015-03-10 18:48:52]

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

[chrishtr, 2015-03-10 18:53:19]

How about just setting the SkNullCanvas in beginRecording() and removing it in
endRecording()?

[Xianzhu, 2015-03-10 19:05:20]

On 2015/03/10 18:53:19, chrishtr wrote:
> How about just setting the SkNullCanvas in beginRecording() and removing it in
> endRecording()?

This will miss the non-slimming-paint mode.

As SkNullCanvas is used only when contextDisabled(), when context is not
disabled, we can still assert the drawings out of DrawingRecorder in slimming
paint mode with the ASSERT(m_canvas).

[f(malita), 2015-03-10 19:12:02]

This should not be a problem in non-slimming-paint mode: we have perf tests
which pass a null canvas, and AFAIK they don't crash => all paths must be
properly guarded by contextDisabled() checks, no? For example
https://chromeperf.appspot.com/report?masters=ChromiumPerf&bots=linux-release...

Do you see any non-SP crashes?

(the null canvas tests and GC instrumentation were added by schenney IIRC)

[Xianzhu, 2015-03-10 19:29:02]

On 2015/03/10 19:12:02, f(malita) wrote:
> This should not be a problem in non-slimming-paint mode: we have perf tests
> which pass a null canvas, and AFAIK they don't crash => all paths must be
> properly guarded by contextDisabled() checks, no?
> For example
>
https://chromeperf.appspot.com/report?masters=ChromiumPerf&bots=linux-release...
> 

The benchmarks are passing a SkNullCanvas, not a null pointer, right?

Passing SkNullCanvas and setting contextDisabled() look duplicated for non-SP
mode. Is contextDisabled() used for any benchmark running in non-SP mode? If
not, I would ignore non-SP mode in this change.

> Do you see any non-SP crashes?
> 
> (the null canvas tests and GC instrumentation were added by schenney IIRC)

[f(malita), 2015-03-10 19:34:24]

fmalita@chromium.org changed reviewers:
+ fmalita@chromium.org

[f(malita), 2015-03-10 19:34:24]

https://codereview.chromium.org/987383003/diff/1/Source/platform/graphics/Gra...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/987383003/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.cpp:559: if (contextDisabled())
Already checked below. Probably just need to relocate the assert.

https://codereview.chromium.org/987383003/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.cpp:1114: void
GraphicsContext::drawBitmap(const SkBitmap& bitmap, SkScalar left, SkScalar top,
const SkPaint* paint)
I don't think this method is used at all - we should probably get rid of it.

https://codereview.chromium.org/987383003/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.cpp:1116: if (contextDisabled())
Already checked below. Relocate the assert?

https://codereview.chromium.org/987383003/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.cpp:1133: if (contextDisabled())
Note that some of these methods are "internal" entry points (they are only ever
reached by calling other GC methods) - so the check is redundant. In this case,
I believe we always go through drawImage() first.

(it used to be that Skia types in the signature were a good indication of
internal/guarded API, but not so much recently)

https://codereview.chromium.org/987383003/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.cpp:1154: ASSERT(m_canvas);
Since m_disabledState is effectively const for the lifetime of a GC (we should
probably mark it as such too), we could assert once in the ctor instead of
everywhere.

Although the latter may be desirable for documentation purposes, so up to you.

[f(malita), 2015-03-10 19:36:43]

On 2015/03/10 19:29:02, Xianzhu wrote:
> On 2015/03/10 19:12:02, f(malita) wrote:
> > This should not be a problem in non-slimming-paint mode: we have perf tests
> > which pass a null canvas, and AFAIK they don't crash => all paths must be
> > properly guarded by contextDisabled() checks, no?
> > For example
> >
>
https://chromeperf.appspot.com/report?masters=ChromiumPerf&bots=linux-release...
> > 
> 
> The benchmarks are passing a SkNullCanvas, not a null pointer, right?
> 
> Passing SkNullCanvas and setting contextDisabled() look duplicated for non-SP
> mode. Is contextDisabled() used for any benchmark running in non-SP mode? If
> not, I would ignore non-SP mode in this change.

I think you're right about SkNullCanvas.

There's also a "record_time_painting_disabled" version of those tests
(https://chromeperf.appspot.com/report?masters=ChromiumPerf&bots=linux-release...)
which I thought does exactly that (sets up a fully disabled GC, possibly with a
null canvas). But I can't find the setup code for it ATM - Stephen?

[f(malita), 2015-03-10 20:22:05]

On 2015/03/10 19:36:43, f(malita) wrote:
> On 2015/03/10 19:29:02, Xianzhu wrote:
> > On 2015/03/10 19:12:02, f(malita) wrote:
> > > This should not be a problem in non-slimming-paint mode: we have perf
tests
> > > which pass a null canvas, and AFAIK they don't crash => all paths must be
> > > properly guarded by contextDisabled() checks, no?
> > > For example
> > >
> >
>
https://chromeperf.appspot.com/report?masters=ChromiumPerf&bots=linux-release...
> > > 
> > 
> > The benchmarks are passing a SkNullCanvas, not a null pointer, right?
> > 
> > Passing SkNullCanvas and setting contextDisabled() look duplicated for
non-SP
> > mode. Is contextDisabled() used for any benchmark running in non-SP mode? If
> > not, I would ignore non-SP mode in this change.
> 
> I think you're right about SkNullCanvas.
> 
> There's also a "record_time_painting_disabled" version of those tests
>
(https://chromeperf.appspot.com/report?masters=ChromiumPerf&bots=linux-release...)
> which I thought does exactly that (sets up a fully disabled GC, possibly with
a
> null canvas). But I can't find the setup code for it ATM - Stephen?

Found it - we do run contextDisabled() non-SP perf tests but pass an
SkNullCanvas even in that case:

https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pictu...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...


So it appears a null canvas can only occur with SP?

[chrishtr, 2015-03-10 20:24:11]

On 2015/03/10 at 20:22:05, fmalita wrote:
> On 2015/03/10 19:36:43, f(malita) wrote:
> > On 2015/03/10 19:29:02, Xianzhu wrote:
> > > On 2015/03/10 19:12:02, f(malita) wrote:
> > > > This should not be a problem in non-slimming-paint mode: we have perf
tests
> > > > which pass a null canvas, and AFAIK they don't crash => all paths must
be
> > > > properly guarded by contextDisabled() checks, no?
> > > > For example
> > > >
> > >
> >
https://chromeperf.appspot.com/report?masters=ChromiumPerf&bots=linux-release...
> > > > 
> > > 
> > > The benchmarks are passing a SkNullCanvas, not a null pointer, right?
> > > 
> > > Passing SkNullCanvas and setting contextDisabled() look duplicated for
non-SP
> > > mode. Is contextDisabled() used for any benchmark running in non-SP mode?
If
> > > not, I would ignore non-SP mode in this change.
> > 
> > I think you're right about SkNullCanvas.
> > 
> > There's also a "record_time_painting_disabled" version of those tests
> >
(https://chromeperf.appspot.com/report?masters=ChromiumPerf&bots=linux-release...)
> > which I thought does exactly that (sets up a fully disabled GC, possibly
with a
> > null canvas). But I can't find the setup code for it ATM - Stephen?
> 
> Found it - we do run contextDisabled() non-SP perf tests but pass an
SkNullCanvas even in that case:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pictu...
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> 
> So it appears a null canvas can only occur with SP?

Yes. That's why I think this CL should allocate SkNullCanvas the canvas in
beginRecording. That should be sufficient.

[f(malita), 2015-03-10 20:32:18]

On 2015/03/10 20:24:11, chrishtr wrote:
> On 2015/03/10 at 20:22:05, fmalita wrote:
> > So it appears a null canvas can only occur with SP?
> 
> Yes. That's why I think this CL should allocate SkNullCanvas the canvas in
> beginRecording. That should be sufficient.

Could SP just pass a top-level SkNullCanvas* to the GC ctor instead of a
nullptr?

[chrishtr, 2015-03-10 20:45:34]

On 2015/03/10 at 20:32:18, fmalita wrote:
> On 2015/03/10 20:24:11, chrishtr wrote:
> > On 2015/03/10 at 20:22:05, fmalita wrote:
> > > So it appears a null canvas can only occur with SP?
> > 
> > Yes. That's why I think this CL should allocate SkNullCanvas the canvas in
> > beginRecording. That should be sufficient.
> 
> Could SP just pass a top-level SkNullCanvas* to the GC ctor instead of a
nullptr?

I guess it doesn't make a big difference. ASSERT(m_canvas == SkNullCanvas) vs
ASSERT(NULL) and code in beginRecording.

[Xianzhu, 2015-03-10 20:50:02]

PTAL.

Thanks for the pointers.

Now I believe contextDisabled() is the SP counterpart of non-SP's passing
SkNullCanvas to GraphicsContext. In Patch Set I removed all contextDisabled()
checks, and create a SkNullCanvas in beginRecording().

[chrishtr, 2015-03-10 22:18:02]

lgtm

[Xianzhu, 2015-03-10 23:00:30]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2015-03-10 23:00:57]

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

[commit-bot: I haz the power, 2015-03-11 03:02:24]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191673

[f(malita), 2015-03-11 03:17:58]

On 2015/03/10 20:50:02, Xianzhu wrote:
> In Patch Set I removed all contextDisabled()
> checks, and create a SkNullCanvas in beginRecording().

Not sure I follow the removal of the contextDisabled() checks:
RECORD_WITH_SK_NULL_CANVAS & RECORD_WITH_PAINTING_DISABLED used to measure
different things, but at this point the difference appears to be gone. If that's
the case, why have a RECORD_WITH_PAINTING_DISABLED mode at all? Are we planning
to get rid of it?

(not complaining about dropping a bunch of boilerplate tests, I've been hoping
we get to do that for a while, but want to understand what the plan is for
RECORD_WITH_PAINTING_DISABLED)

[Stephen Chennney, 2015-03-11 12:58:05]

On 2015/03/11 03:17:58, f(malita) wrote:
> On 2015/03/10 20:50:02, Xianzhu wrote:
> > In Patch Set I removed all contextDisabled()
> > checks, and create a SkNullCanvas in beginRecording().
> 
> Not sure I follow the removal of the contextDisabled() checks:
> RECORD_WITH_SK_NULL_CANVAS & RECORD_WITH_PAINTING_DISABLED used to measure
> different things, but at this point the difference appears to be gone. If
that's
> the case, why have a RECORD_WITH_PAINTING_DISABLED mode at all? Are we
planning
> to get rid of it?
> 
> (not complaining about dropping a bunch of boilerplate tests, I've been hoping
> we get to do that for a while, but want to understand what the plan is for
> RECORD_WITH_PAINTING_DISABLED)

Removing all the contextDisabled calls means that you have broken performance
testing. Expect to see a regression in rasterize_and_record for the
record_time_painting_disabled mode, which is intended to show the performance of
Blink paint methods. "Null canvas" mode included the costs of graphics context
-> skia code. These will now be the same, presumably.

In Slimming Paint mode the plan was to use "context disabled" as a mode where no
display items are emitted. There's a line item for it on the spreadsheet. The
"null canvas" mode will be something where we use a null canvas for final
raster, maybe, or where we use SkPictureRecorders that don't record anything
(but we still create display items).

Personally I think this should be reverted to get our perf measurements back.
It's was already useful for S.P. perf measurements. Specifically, it tells us
how much more work we are doing to create display lists vs just issuing graphics
context commands.

[f(malita), 2015-03-11 13:06:57]

On 2015/03/11 12:58:05, Stephen Chenney wrote:
> Expect to see a regression in rasterize_and_record for the
> record_time_painting_disabled mode, which is intended to show the performance
of
> Blink paint methods. "Null canvas" mode included the costs of graphics context
> -> skia code. These will now be the same, presumably.

https://chromeperf.appspot.com/report?masters=ChromiumPerf&bots=linux-release...

[Xianzhu, 2015-03-11 16:19:26]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/997113004/ by wangxianzhu@chromium.org.

The reason for reverting is: It seems that the presumption that contextDisabled
is just a SP counterpart of non-SP's SkNullCanvas is wrong..

[Xianzhu, 2015-03-11 16:45:10]

PTAL. The latest patch set is basically the same as Patch Set 1, with fmalita's
comments addressed.

The SkNullCanvas is still created in the constructor. If we make it in
beginRecording, we'll need to manage the stack which will be an extra cost that
we don't want to be included.

https://codereview.chromium.org/987383003/diff/1/Source/platform/graphics/Gra...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/987383003/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.cpp:1114: void
GraphicsContext::drawBitmap(const SkBitmap& bitmap, SkScalar left, SkScalar top,
const SkPaint* paint)
On 2015/03/10 19:34:24, f(malita) wrote:
> I don't think this method is used at all - we should probably get rid of it.

I'd leave this to another change.

https://codereview.chromium.org/987383003/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.cpp:1116: if (contextDisabled())
On 2015/03/10 19:34:24, f(malita) wrote:
> Already checked below. Relocate the assert?

Done.

https://codereview.chromium.org/987383003/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContext.cpp:1154: ASSERT(m_canvas);
On 2015/03/10 19:34:24, f(malita) wrote:
> Since m_disabledState is effectively const for the lifetime of a GC (we should
> probably mark it as such too), we could assert once in the ctor instead of
> everywhere.
> 
> Although the latter may be desirable for documentation purposes, so up to you.

This assert is also for m_disabledState==false. m_canvas can change during the
lifetime of GC on beginRecording/endRecording/resetCanvas.

[f(malita), 2015-03-11 18:20:51]

LGTM, but I'll defer to Stephen for final say.

https://codereview.chromium.org/987383003/diff/100001/Source/platform/graphic...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/987383003/diff/100001/Source/platform/graphic...
Source/platform/graphics/GraphicsContext.cpp:111: if (contextDisabled()) {
Would it be a lot of duplication to do this at call sites and just assert here?

https://codereview.chromium.org/987383003/diff/100001/Source/platform/graphic...
Source/platform/graphics/GraphicsContext.cpp:113: m_canvas = nullCanvas.get();
If we always allocate a (SkNull) canvas when contextDisabled(), I'm guessing we
don't *have to* relocate the asserts below, no? But I do think the new order
makes more sense, so I'm fine with that.

[chrishtr, 2015-03-11 18:25:46]

lgtm

[Stephen Chennney, 2015-03-11 18:30:55]

On 2015/03/11 18:25:46, chrishtr wrote:
> lgtm

lgtm from me too. Moving the asserts gives us a little future-proofing, so I'm
happy with it.

I never liked having an "if (contextDisabled())" at every call site, but I could
never come up with a better way. Maybe with Slimming Paint we will be able to.

[Xianzhu, 2015-03-11 18:44:06]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2015-03-11 18:44:25]

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

[f(malita), 2015-03-11 18:49:32]

On 2015/03/11 18:30:55, Stephen Chenney wrote:
> I never liked having an "if (contextDisabled())" at every call site, but I
could
> never come up with a better way. Maybe with Slimming Paint we will be able to.

+1000

IIRC, the checks were needed historically to support some obscure code path
(custom scrollbar themes abusing the paint() code for some hysterical reason I
don't recall?). But that doesn't seem to be the case anymore - so yeah, keeping
this clutter around just for benchmark instrumentation is not ideal.

[Xianzhu, 2015-03-11 19:10:55]

On 2015/03/11 18:49:32, f(malita) wrote:
> On 2015/03/11 18:30:55, Stephen Chenney wrote:
> > I never liked having an "if (contextDisabled())" at every call site, but I
> could
> > never come up with a better way. Maybe with Slimming Paint we will be able
to.
> 
> +1000
> 
> IIRC, the checks were needed historically to support some obscure code path
> (custom scrollbar themes abusing the paint() code for some hysterical reason I
> don't recall?). But that doesn't seem to be the case anymore - so yeah,
keeping
> this clutter around just for benchmark instrumentation is not ideal.

The SkNullCanvas used when contextDisabled() in SP mode is actually for native
themes. Without it we'll crash on null canvas in chromium code.

Currently the contextDisabled() checks are for excluding the cost from
GraphicsContext itself before calling SkCanvas methods.

> In Slimming Paint mode the plan was to use "context disabled" as a mode where
no
> display items are emitted. There's a line item for it on the spreadsheet. The
> "null canvas" mode will be something where we use a null canvas for final
> raster, maybe, or where we use SkPictureRecorders that don't record anything
> (but we still create display items).

Currently there seem still works to do to achieve the goals ("context disabled
generating no display items" and "null canvas with all display items"):
- When contextDisabled(), we still create display items other than
DrawingDisplayItems;
- When we use SkNullCanvas, because SkNullCanvas doesn't support recording, we
can't create DrawingDisplayItems.

[Xianzhu, 2015-03-11 19:40:47]

Late publishing of drafts:

https://codereview.chromium.org/987383003/diff/100001/Source/platform/graphic...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/987383003/diff/100001/Source/platform/graphic...
Source/platform/graphics/GraphicsContext.cpp:111: if (contextDisabled()) {
On 2015/03/11 18:20:51, f(malita) wrote:
> Would it be a lot of duplication to do this at call sites and just assert
here?

Looked at several call sites and that seems more complex.

https://codereview.chromium.org/987383003/diff/100001/Source/platform/graphic...
Source/platform/graphics/GraphicsContext.cpp:113: m_canvas = nullCanvas.get();
On 2015/03/11 18:20:51, f(malita) wrote:
> If we always allocate a (SkNull) canvas when contextDisabled(), I'm guessing
we
> don't *have to* relocate the asserts below, no?

Right.

> But I do think the new order makes more sense, so I'm fine with that.

I think the same.

[commit-bot: I haz the power, 2015-03-11 21:10:39]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191734