Bug 74978 – [Skia] [Coverity] Fix an uninitialized member variable

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Layout and Rendering

[Rachel Blum]

[Coverity] Fixed uninitialized member var

[Rachel Blum]

Created attachment 120120 [details]
Patch

[Simon Hausmann]

Comment on attachment 120120 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120120&action=review

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:125
> +    , m_clip(SkRect::MakeEmpty())

It sounds like it would be better to call m_clip.SetEmpty() instead of doing a double construction.

[Nikolas Zimmermann]

Comment on attachment 120120 [details]
Patch

A quick grep shows following users of m_clip:

PlatformContextSkia.cpp:    canvas()->getTotalMatrix().mapRect(&m_state->m_clip);
PlatformContextSkia.cpp:        applyClipFromImage(m_state->m_clip, m_state->m_imageBufferClip);
which are relevant.
The first place, sets m_clip, but the restore() call actually reads from m_clip, if m_imageBufferClip is non null.
Is it possible to create a layout test triggering this? Anyhow, still r=me, this is straight-forward. But if it had a test, it would be superb.

[Nikolas Zimmermann]

(In reply to comment #2)
> (From update of attachment 120120 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120120&action=review
> 
> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:125
> > +    , m_clip(SkRect::MakeEmpty())
> 
> It sounds like it would be better to call m_clip.SetEmpty() instead of doing a double construction.

You're right. Theres a nicer variant for SKIRect:

    static const SkIRect& EmptyIRect() {
        static const SkIRect gEmpty = { 0, 0, 0, 0 };
        return gEmpty;
    }

That using a SkRect is what we really want.

[Rachel Blum]

Happy to try to get that into Skia - meanwhile, may I cq?

As for layout tests, I haven't been able to come up with any way to actually trigger a crash based on this - this is preventive, not reactive.