[PDF] Fix Chrome crash - don't assume that SkDraw.fClipStack != NULL.

[PDF] Fix Chrome crash - don't assume that SkDraw.fClipStack != NULL.

Chrome bug is crbug.com/82198.

Committed: http://code.google.com/p/skia/source/detail?r=1306

[Chris Guillory, 2011-05-11 05:49:42]

http://codereview.appspot.com/4515061/diff/2001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4515061/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1041: bool SkPDFDevice::setUpContentEntry(const
SkClipStack* passedClipStack,
Not following why we need to rename this passedClipStack.

http://codereview.appspot.com/4515061/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1051: // fill it in, do so, bail otherwise.
Not seeing how we "bail".

[Steve VanDeBogart, 2011-05-11 06:01:19]

http://codereview.appspot.com/4515061/diff/2001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4515061/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1041: bool SkPDFDevice::setUpContentEntry(const
SkClipStack* passedClipStack,
On 2011/05/11 05:49:42, Chris Guillory wrote:
> Not following why we need to rename this passedClipStack.

You're right, fixed.

http://codereview.appspot.com/4515061/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1051: // fill it in, do so, bail otherwise.
On 2011/05/11 05:49:42, Chris Guillory wrote:
> Not seeing how we "bail".

The assert is my bail.  I know that won't run in release builds, but the assert
should catch any cases in debug mode first.  clipStack will either be NULL or
not, passed on call site.  The callsite where it's NULL is
vector_platform_device_skia.cc:82.  I would have changed the callsite, but it is
complaint with the interface, so...

[Chris Guillory, 2011-05-11 06:23:33]

May I just need this part explained in person.

http://codereview.appspot.com/4515061/diff/2001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4515061/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1051: // fill it in, do so, bail otherwise.
I take bail to mean that we are not doing something that we would be doing
normally (usually accompanied by not continuing in the function). Are we not
changing to a new clip? Perhaps this comment could be rephrased?

[reed1, 2011-05-11 13:12:40]

Can we reproduce this outside of chrome? Is this a case of SkCanvas not setting
the fClipStack field? If so, then then we may need a broader fix.

[Steve VanDeBogart, 2011-05-11 15:48:45]

On 2011/05/11 13:12:40, reed1 wrote:
> Can we reproduce this outside of chrome? Is this a case of SkCanvas not
setting
> the fClipStack field? If so, then then we may need a broader fix.

No, this is only from Chrome in VectorPlatformDeviceSkia::EndPlatformPaint
(which will not go away, only move, when we remove the platform devices), where
I make up an SkDraw.  I've proposed this fix though because SkDraw.h says that
fClipStack is optional.  If it's actually a required field of SkDraw at this
point, then I'd propose an alternate change: Adding accessors for
fExistingClipStack/Region to SkPDFDevice and using those to populate the SkDraw.

[reed1, 2011-05-11 16:33:21]

Gotcha.

Lets go with your change as is then.

On Wed, May 11, 2011 at 8:48 AM,  <vandebo@chromium.org> wrote:
> On 2011/05/11 13:12:40, reed1 wrote:
>>
>> Can we reproduce this outside of chrome? Is this a case of SkCanvas
>
> not setting
>>
>> the fClipStack field? If so, then then we may need a broader fix.
>
> No, this is only from Chrome in
> VectorPlatformDeviceSkia::EndPlatformPaint (which will not go away, only
> move, when we remove the platform devices), where I make up an SkDraw.
> I've proposed this fix though because SkDraw.h says that fClipStack is
> optional.  If it's actually a required field of SkDraw at this point,
> then I'd propose an alternate change: Adding accessors for
> fExistingClipStack/Region to SkPDFDevice and using those to populate the
> SkDraw.
>
> http://codereview.appspot.com/4515061/
>

[Chris Guillory, 2011-05-11 17:54:13]

LGTM. One nit.

http://codereview.appspot.com/4515061/diff/4/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4515061/diff/4/src/pdf/SkPDFDevice.cpp#newcode1051
src/pdf/SkPDFDevice.cpp:1051: // fill it in, do so, bail otherwise.
Can you update this comment to be more descriptive as in our discussion
(verifying clip region by assert). Also, maybe indicate that undefined behavior
or perhaps eventual crash occurs during SkASSERT failure.

[Steve VanDeBogart, 2011-05-11 18:29:35]

http://codereview.appspot.com/4515061/diff/4/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4515061/diff/4/src/pdf/SkPDFDevice.cpp#newcode1051
src/pdf/SkPDFDevice.cpp:1051: // fill it in, do so, bail otherwise.
On 2011/05/11 17:54:13, Chris Guillory wrote:
> Can you update this comment to be more descriptive as in our discussion
> (verifying clip region by assert). Also, maybe indicate that undefined
behavior
> or perhaps eventual crash occurs during SkASSERT failure.

I just added code to handle the other case.

[Chris Guillory, 2011-05-11 18:42:05]

Great! LGTM with nits.

http://codereview.appspot.com/4515061/diff/9002/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4515061/diff/9002/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1050: // Technically, the clip stack in the SkDraw is
optional.
Nit: Optional: 
(1) in the SkDraw -> can come from a SkDraw where it
(2) in the SkDraw -> which comes from a SkDraw
* Which ever is more true.

http://codereview.appspot.com/4515061/diff/9002/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1056: // GrphicStackState::updateClip expects the clip
stack to have
Nit: GrphicStackState -> GraphicStackState

[Steve VanDeBogart, 2011-05-11 18:45:37]

Thanks.

http://codereview.appspot.com/4515061/diff/9002/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4515061/diff/9002/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1050: // Technically, the clip stack in the SkDraw is
optional.
On 2011/05/11 18:42:05, Chris Guillory wrote:
> Nit: Optional: 
> (1) in the SkDraw -> can come from a SkDraw where it
> (2) in the SkDraw -> which comes from a SkDraw
> * Which ever is more true.

Done.

http://codereview.appspot.com/4515061/diff/9002/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1056: // GrphicStackState::updateClip expects the clip
stack to have
On 2011/05/11 18:42:05, Chris Guillory wrote:
> Nit: GrphicStackState -> GraphicStackState

Done.