Bug 182503 – Move ImageBuffer-related functionality from HTMLCanvasElement to CanvasBase

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Zan Dobersek]

Move ImageBuffer-related functionality from HTMLCanvasElement to CanvasBase

[Zan Dobersek]

Created attachment 333116 [details]
Patch

[Zan Dobersek]

Created attachment 333117 [details]
Patch

[EWS Watchlist]

Comment on attachment 333117 [details]
Patch

Attachment 333117 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6372436

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 333130 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Zan Dobersek]

Created attachment 333158 [details]
Patch

[EWS Watchlist]

Comment on attachment 333158 [details]
Patch

Attachment 333158 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6379587

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 333162 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Zan Dobersek]

Created attachment 333174 [details]
Patch

[Zan Dobersek]

Created attachment 334801 [details]
Patch

[Chris Lord]

Created attachment 379889 [details]
Patch

[Chris Lord]

Created attachment 380315 [details]
Patch

[Chris Lord]

Created attachment 380326 [details]
Patch

[Chris Lord]

Created attachment 380331 [details]
Patch

[Chris Lord]

Comment on attachment 380331 [details]
Patch

I don't think the failures there have anything to do with this patch (which only touches Canvas-related things), also marking cq?

[Chris Lord]

Created attachment 380619 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 380619 [details]
Patch

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

> Source/WebCore/html/CanvasBase.cpp:222
> +    s_activePixelMemory -= std::min(previousMemoryCost, s_activePixelMemory);

How is this thread safe?

> Source/WebCore/html/CanvasBase.cpp:234
> +    s_activePixelMemory += currentMemoryCost;

Ditto.

[Zan Dobersek]

Comment on attachment 380619 [details]
Patch

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

>> Source/WebCore/html/CanvasBase.cpp:222
>> +    s_activePixelMemory -= std::min(previousMemoryCost, s_activePixelMemory);
> 
> How is this thread safe?

It's not, but in the scope of these changes alone it doesn't have to be since it will still only be called only from HTMLCanvasElement.

This can be made thread-safe in a separate patch.

[Ryosuke Niwa]

(In reply to Zan Dobersek from comment #17)
> Comment on attachment 380619 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380619&action=review
> 
> >> Source/WebCore/html/CanvasBase.cpp:222
> >> +    s_activePixelMemory -= std::min(previousMemoryCost, s_activePixelMemory);
> > 
> > How is this thread safe?
> 
> It's not, but in the scope of these changes alone it doesn't have to be
> since it will still only be called only from HTMLCanvasElement.
> 
> This can be made thread-safe in a separate patch.

Hm... it doesn't seem great to introduce new code that's not thread safe to use concurrency in order to add a feature that requires all relevant code to be thread safe.

[Chris Lord]

(In reply to Ryosuke Niwa from comment #18)
> (In reply to Zan Dobersek from comment #17)
> > Comment on attachment 380619 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=380619&action=review
> > 
> > >> Source/WebCore/html/CanvasBase.cpp:222
> > >> +    s_activePixelMemory -= std::min(previousMemoryCost, s_activePixelMemory);
> > > 
> > > How is this thread safe?
> > 
> > It's not, but in the scope of these changes alone it doesn't have to be
> > since it will still only be called only from HTMLCanvasElement.
> > 
> > This can be made thread-safe in a separate patch.
> 
> Hm... it doesn't seem great to introduce new code that's not thread safe to
> use concurrency in order to add a feature that requires all relevant code to
> be thread safe.

That's fair, I guess an easy fix would be to just replace s_activePixelMemory with an atomic int(?)

[Ryosuke Niwa]

Comment on attachment 380619 [details]
Patch

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

>>>>> Source/WebCore/html/CanvasBase.cpp:222
>>>>> +    s_activePixelMemory -= std::min(previousMemoryCost, s_activePixelMemory);
>>>> 
>>>> How is this thread safe?
>>> 
>>> It's not, but in the scope of these changes alone it doesn't have to be since it will still only be called only from HTMLCanvasElement.
>>> 
>>> This can be made thread-safe in a separate patch.
>> 
>> Hm... it doesn't seem great to introduce new code that's not thread safe to use concurrency in order to add a feature that requires all relevant code to be thread safe.
> 
> That's fair, I guess an easy fix would be to just replace s_activePixelMemory with an atomic int(?)

Atomic int wouldn't work because by the time you've computed min, the value of s_activePixelMemory may have been updated by some other thread.

[Chris Lord]

(In reply to Ryosuke Niwa from comment #20)
> Comment on attachment 380619 [details]
> Atomic int wouldn't work because by the time you've computed min, the value
> of s_activePixelMemory may have been updated by some other thread.

Of course, you're right - I'd like to avoid having a lock here, it seems overkill given the common case and this function is called quite often. I'm a bit dubious of this whole thing, if it's reliable at all then it should never drop below zero. I guess what this is guarding against is if previousMemoryCost is for some reason bigger than what was previously calculated as currentMemoryCost. The alternative would be to store what was previously set, then we could use an atomic int and remove min check altogether.

[Chris Lord]

Created attachment 380739 [details]
Patch

[Chris Lord]

Created attachment 381075 [details]
Patch

[Chris Lord]

Created attachment 381182 [details]
Patch

[Darin Adler]

Comment on attachment 381182 [details]
Patch

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

Was looking this over casually without carefully studying everything. A few comments on the current patch.

> Source/WebCore/html/CanvasBase.h:110
> +    CanvasBase(const IntSize&);

I think it’s better to use IntSize rather than const IntSize&; more efficient the former way because of the small size of this struct.

Also, this constructor should be marked explicit since it’s not intended for the conversion.

> Source/WebCore/html/CanvasBase.h:113
> +    virtual void createImageBuffer() const { };

I think this should be private, not protected. Functions don’t need to be protected to be overridden by derived classes, only if they need to be called by derived classes.

Unnecessary semicolon here.

> Source/WebCore/html/CanvasBase.h:118
> +    static Atomic<size_t> s_activePixelMemory;

We could avoid putting this in the header and avoid needing to add the Atomics.h include by:

- Having this be a global variable private to the source file (internal linkage, using "static" or an anonymous namespace), rather than a static data member.
- Having a protected static member function that simply fetches the value; that is what’s needed in the derived class HTMLCanvasElement.

> Source/WebCore/html/CanvasBase.h:131
> +    mutable IntSize m_size;
> +
> +    mutable Lock m_imageBufferAssignmentLock;
> +
> +    // m_createdImageBuffer means we tried to malloc the buffer. We didn't necessarily get it.
> +    mutable bool m_hasCreatedImageBuffer { false };
> +    mutable bool m_didClearImageBuffer { false };
> +    mutable std::unique_ptr<ImageBuffer> m_imageBuffer;
> +    mutable size_t m_imageBufferCost { 0 };
> +    mutable std::unique_ptr<GraphicsContextStateSaver> m_contextStateSaver;

Typically protected data members are not a good long term design pattern. As many of these as possible should be made private. We can add protected functions for the derived classes to use.

> Source/WebCore/html/CustomPaintCanvas.cpp:41
>  CustomPaintCanvas::CustomPaintCanvas(ScriptExecutionContext& context, unsigned width, unsigned height)

Not new to this patch: Strange that this takes two unsigned values and then converts them to int without checking their values. Seems unsafe and non-obvious that this will change giant value into negative numbers.

> Source/WebCore/html/OffscreenCanvas.cpp:44
>  OffscreenCanvas::OffscreenCanvas(ScriptExecutionContext& context, unsigned width, unsigned height)

Same comment as above.

[Chris Lord]

(In reply to Darin Adler from comment #25)
> Comment on attachment 381182 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381182&action=review
> 
> Was looking this over casually without carefully studying everything. A few
> comments on the current patch.
> 
> > Source/WebCore/html/CanvasBase.h:110
> > +    CanvasBase(const IntSize&);
> 
> I think it’s better to use IntSize rather than const IntSize&; more
> efficient the former way because of the small size of this struct.
> 
> Also, this constructor should be marked explicit since it’s not intended for
> the conversion.

Sounds good to me

> > Source/WebCore/html/CanvasBase.h:113
> > +    virtual void createImageBuffer() const { };
> 
> I think this should be private, not protected. Functions don’t need to be
> protected to be overridden by derived classes, only if they need to be
> called by derived classes.
> 
> Unnecessary semicolon here.

Fair point, and good spot - will change.

> > Source/WebCore/html/CanvasBase.h:118
> > +    static Atomic<size_t> s_activePixelMemory;
> 
> We could avoid putting this in the header and avoid needing to add the
> Atomics.h include by:
> 
> - Having this be a global variable private to the source file (internal
> linkage, using "static" or an anonymous namespace), rather than a static
> data member.
> - Having a protected static member function that simply fetches the value;
> that is what’s needed in the derived class HTMLCanvasElement.

Done.

> > Source/WebCore/html/CanvasBase.h:131
> > +    mutable IntSize m_size;
> > +
> > +    mutable Lock m_imageBufferAssignmentLock;
> > +
> > +    // m_createdImageBuffer means we tried to malloc the buffer. We didn't necessarily get it.
> > +    mutable bool m_hasCreatedImageBuffer { false };
> > +    mutable bool m_didClearImageBuffer { false };
> > +    mutable std::unique_ptr<ImageBuffer> m_imageBuffer;
> > +    mutable size_t m_imageBufferCost { 0 };
> > +    mutable std::unique_ptr<GraphicsContextStateSaver> m_contextStateSaver;
> 
> Typically protected data members are not a good long term design pattern. As
> many of these as possible should be made private. We can add protected
> functions for the derived classes to use.

Will do.

> > Source/WebCore/html/CustomPaintCanvas.cpp:41
> >  CustomPaintCanvas::CustomPaintCanvas(ScriptExecutionContext& context, unsigned width, unsigned height)
> 
> Not new to this patch: Strange that this takes two unsigned values and then
> converts them to int without checking their values. Seems unsafe and
> non-obvious that this will change giant value into negative numbers.
> 
> > Source/WebCore/html/OffscreenCanvas.cpp:44
> >  OffscreenCanvas::OffscreenCanvas(ScriptExecutionContext& context, unsigned width, unsigned height)
> 
> Same comment as above.

I'm going to leave this, for now at least. Like you say, it was there before, and for what it's worth, canvases initialised with negative values is a situation that's checked in layout tests, so at the least this has test coverage and doesn't result in something really bad happening (I don't think).

[Chris Lord]

Created attachment 382064 [details]
Patch

[Chris Lord]

Comment on attachment 382064 [details]
Patch

Removing cq?, mac-debug-wk1 failures look related to this patch.

[Chris Lord]

Going over this patch and the exception that is crashing Mac builds, I don't see how, given the path it's triggered on, this could happen... In the hope that this was caused by an unrelated patch, I'm going to try submitting again.

[Chris Lord]

Created attachment 382295 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 382295 [details]
Patch

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

> Source/WebCore/html/CanvasBase.cpp:39
> +static Atomic<size_t> s_activePixelMemory { 0 };

Use std::atomics instead?

> Source/WebCore/html/CanvasBase.cpp:100
> +size_t CanvasBase::memoryCost() const

We should rename this to memoryCostConcurrently instead of having a lengthly comment below.
Probably in a separate patch though.

> Source/WebCore/html/CanvasBase.cpp:113
> +    // externalMemoryCost() may be invoked concurrently from a GC thread, and we need to be careful

Ditto.

> Source/WebCore/html/CanvasBase.cpp:205
> +    s_activePixelMemory.exchangeAdd(m_imageBufferCost);

?? How does work? By this code runs, s_activePixelMemory's value may have changed, right?
Why can't we just do this?
s_activePixelMemory += m_imageBufferCost - previousMemoryCost

https://en.cppreference.com/w/cpp/atomic/atomic/operator_arith2

> Source/WebCore/html/CanvasBase.h:113
> +

Nit: Remove this plan line?

> Source/WebCore/html/CanvasBase.h:115
> +

Ditto.

> Source/WebCore/html/CanvasBase.h:124
> +    mutable IntSize m_size;
> +
> +    mutable Lock m_imageBufferAssignmentLock;
> +
> +    mutable std::unique_ptr<GraphicsContextStateSaver> m_contextStateSaver;

What's up with all the blank lines here? Just remove them.
Also can't we add protected setter & getter for these instead of making them protected?

[Darin Adler]

This crash does look like it is related to the patch:

https://ews-build.webkit.org/results/macOS-High-Sierra-Debug-WK1-Tests-EWS/r382295-5143/DumpRenderTree-2196-crash-log.txt

It’s a crash inside garbage collection. When the graphics context is destroyed, it has a non-empty save/restore stack and there’s an assertion that says that’s not OK.

I think the assertion is probably incorrect. It seems that for a canvas you could save, not restore, and then let the canvas get garbage collected. Then the assertion would fire. So the assertion seems to be incorrect.

I think the patch may have to remove this assertion!

[Chris Lord]

(In reply to Ryosuke Niwa from comment #31)
> Comment on attachment 382295 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382295&action=review
> 
> > Source/WebCore/html/CanvasBase.cpp:39
> > +static Atomic<size_t> s_activePixelMemory { 0 };
> 
> Use std::atomics instead?

Sure

> > Source/WebCore/html/CanvasBase.cpp:100
> > +size_t CanvasBase::memoryCost() const
> 
> We should rename this to memoryCostConcurrently instead of having a lengthly
> comment below.
> Probably in a separate patch though.
> 
> > Source/WebCore/html/CanvasBase.cpp:113
> > +    // externalMemoryCost() may be invoked concurrently from a GC thread, and we need to be careful
> 
> Ditto.

Right, I left these unchanged but that sounds like a nice follow-up.

> > Source/WebCore/html/CanvasBase.cpp:205
> > +    s_activePixelMemory.exchangeAdd(m_imageBufferCost);
> 
> ?? How does work? By this code runs, s_activePixelMemory's value may have
> changed, right?
> Why can't we just do this?
> s_activePixelMemory += m_imageBufferCost - previousMemoryCost
> 
> https://en.cppreference.com/w/cpp/atomic/atomic/operator_arith2

Using std::atomic we can do +=, Atomic didn't have such an operator. With regards to the change, it doesn't matter if it changes, as long as the addition and subtraction are atomic and guaranteed to happen, but your suggestion is nicer.

> > Source/WebCore/html/CanvasBase.h:113
> > +
> 
> Nit: Remove this plan line?
> 
> > Source/WebCore/html/CanvasBase.h:115
> > +
> 
> Ditto.
> 
> > Source/WebCore/html/CanvasBase.h:124
> > +    mutable IntSize m_size;
> > +
> > +    mutable Lock m_imageBufferAssignmentLock;
> > +
> > +    mutable std::unique_ptr<GraphicsContextStateSaver> m_contextStateSaver;
> 
> What's up with all the blank lines here? Just remove them.

Sure. I think this may be legacy from older versions of the patch where there were more variables, grouped somewhat logically, but some have been removed/moved(?)

> Also can't we add protected setter & getter for these instead of making them
> protected?

I don't see why not (immediately, at least), will do.

[Chris Lord]

(In reply to Darin Adler from comment #32)
> This crash does look like it is related to the patch:
> 
> https://ews-build.webkit.org/results/macOS-High-Sierra-Debug-WK1-Tests-EWS/
> r382295-5143/DumpRenderTree-2196-crash-log.txt
> 
> It’s a crash inside garbage collection. When the graphics context is
> destroyed, it has a non-empty save/restore stack and there’s an assertion
> that says that’s not OK.
> 
> I think the assertion is probably incorrect. It seems that for a canvas you
> could save, not restore, and then let the canvas get garbage collected. Then
> the assertion would fire. So the assertion seems to be incorrect.
> 
> I think the patch may have to remove this assertion!

I don't think this is correct. CanvasRenderingContext2DBase unwinds the stack in its destructor when asserts are enabled, so I guess this would indicate that that destructor isn't running before the GraphicsContext is being destroyed.

I can't reproduce this locally, however, so I'm not sure what's going on here or what this patch has changed to cause that issue. I'll look into it further. If I can't come up with anything I'll see about using a Mac to debug this.

[Chris Lord]

Created attachment 382471 [details]
Patch

[Chris Lord]

I believe/hope this addresses the issues brought up and the Mac assert, but I'd like to see green EWS first rather than cause cq?/r? churn.

[Chris Lord]

Comment on attachment 382471 [details]
Patch

Ok, looking good :)

[WebKit Commit Bot]

Comment on attachment 382471 [details]
Patch

Clearing flags on attachment: 382471

Committed r251874: <https://trac.webkit.org/changeset/251874>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56793658>