Bug 182503

Created attachment 333116 [details] Patch

Created attachment 333117 [details] Patch

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.

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

Created attachment 333158 [details] Patch

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.

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

Created attachment 333174 [details] Patch

Created attachment 334801 [details] Patch

Created attachment 379889 [details] Patch

Created attachment 380315 [details] Patch

Created attachment 380326 [details] Patch

Created attachment 380331 [details] Patch

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?

Created attachment 380619 [details] Patch

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.

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.

(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.

(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(?)

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.

(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.

Created attachment 380739 [details] Patch

Created attachment 381075 [details] Patch

Created attachment 381182 [details] Patch

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.

(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).

Created attachment 382064 [details] Patch

Comment on attachment 382064 [details] Patch Removing cq?, mac-debug-wk1 failures look related to this patch.

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.

Created attachment 382295 [details] Patch

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?

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!

(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.

(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.

Created attachment 382471 [details] Patch

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.

Comment on attachment 382471 [details] Patch Ok, looking good :)

Comment on attachment 382471 [details] Patch Clearing flags on attachment: 382471 Committed r251874: <https://trac.webkit.org/changeset/251874>

All reviewed patches have been landed. Closing bug.

<rdar://problem/56793658>