fast/canvas/canvas-crash.html doesn't test what it intends to on iOS
Created attachment 434850 [details] Patch
<rdar://problem/81423634>
Comment on attachment 434850 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434850&action=review > Source/WebCore/html/HTMLCanvasElement.cpp:115 > +static size_t maxCanvasAreaForTesting; > +static size_t maxActivePixelMemoryForTesting; Could these be Markable<>? > Source/WebCore/testing/Internals.cpp:604 > + HTMLCanvasElement::setMaxPixelMemoryForTesting(0); > + HTMLCanvasElement::setMaxCanvasAreaForTesting(0); Ideally these would take std::optionals
Created attachment 434868 [details] Patch
windows newlines oO
Heh, I think the configuration of the stress bot means it runs the test in the same process multiple times, and we hit the limit after a few tries :|
Committed r280630 (240243@main): <https://commits.webkit.org/240243@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 434868 [details].
Re-opened since this is blocked by bug 228788
(In reply to Tim Horton from comment #6) > Heh, I think the configuration of the stress bot means it runs the test in > the same process multiple times, and we hit the limit after a few tries :| Except the same thing happens on the real testers too!
(In reply to Tim Horton from comment #9) > (In reply to Tim Horton from comment #6) > > Heh, I think the configuration of the stress bot means it runs the test in > > the same process multiple times, and we hit the limit after a few tries :| > > Except the same thing happens on the real testers too! Leak?
(In reply to Sam Weinig from comment #10) > (In reply to Tim Horton from comment #9) > > (In reply to Tim Horton from comment #6) > > > Heh, I think the configuration of the stress bot means it runs the test in > > > the same process multiple times, and we hit the limit after a few tries :| > > > > Except the same thing happens on the real testers too! > > Leak? (or rather, depending on GC to cleanup something big?)
Yes, that. And we reset the max down to the default between each test so it's immediately TOO MUCH CANVAS
(In reply to Tim Horton from comment #12) > Yes, that. And we reset the max down to the default between each test so > it's immediately TOO MUCH CANVAS There are two options I can see here. 1) Remove the canvas from the document and force a GC using GCController.collect() to ensure the canvas is full destroyed before moving on to the next test. 2) Implement ActiveDOMObject::stop() for HTMLCanvasElement, and ensure the ImageBuffer is destroyed there. Ideally, we would do #2, but it is a bit more risk, so doing #1 first, and then doing #2 in another change may be the best option here.
(In reply to Sam Weinig from comment #13) > (In reply to Tim Horton from comment #12) > > Yes, that. And we reset the max down to the default between each test so > > it's immediately TOO MUCH CANVAS > > There are two options I can see here. > > 1) Remove the canvas from the document and force a GC using > GCController.collect() to ensure the canvas is full destroyed before moving > on to the next test. > 2) Implement ActiveDOMObject::stop() for HTMLCanvasElement, and ensure the > ImageBuffer is destroyed there. > > > Ideally, we would do #2, but it is a bit more risk, so doing #1 first, and > then doing #2 in another change may be the best option here. https://bugs.webkit.org/show_bug.cgi?id=224500 tracks #2
(In reply to Sam Weinig from comment #13) > (In reply to Tim Horton from comment #12) > > Yes, that. And we reset the max down to the default between each test so > > it's immediately TOO MUCH CANVAS > > There are two options I can see here. > > 1) Remove the canvas from the document and force a GC using > GCController.collect() to ensure the canvas is full destroyed before moving > on to the next test. Oddly, this did not work, but I found a similar-but-even-less-flaky solution that does: resize the canvas before ending the test.
(In reply to Tim Horton from comment #15) > (In reply to Sam Weinig from comment #13) > > (In reply to Tim Horton from comment #12) > > > Yes, that. And we reset the max down to the default between each test so > > > it's immediately TOO MUCH CANVAS > > > > There are two options I can see here. > > > > 1) Remove the canvas from the document and force a GC using > > GCController.collect() to ensure the canvas is full destroyed before moving > > on to the next test. > > Oddly, this did not work, but I found a similar-but-even-less-flaky solution > that does: resize the canvas before ending the test. Oh, it's not odd: the test doesn't actually use its <canvas> at all, it uses getCSSCanvasContext()
Created attachment 435038 [details] Patch
(In reply to Tim Horton from comment #16) > (In reply to Tim Horton from comment #15) > > (In reply to Sam Weinig from comment #13) > > > (In reply to Tim Horton from comment #12) > > > > Yes, that. And we reset the max down to the default between each test so > > > > it's immediately TOO MUCH CANVAS > > > > > > There are two options I can see here. > > > > > > 1) Remove the canvas from the document and force a GC using > > > GCController.collect() to ensure the canvas is full destroyed before moving > > > on to the next test. > > > > Oddly, this did not work, but I found a similar-but-even-less-flaky solution > > that does: resize the canvas before ending the test. > > Oh, it's not odd: the test doesn't actually use its <canvas> at all, it uses > getCSSCanvasContext() Ah, that would makes sense. Those live for the lifetime of the document due to being "name" addressable. Your solution here seems great.
Stress bot is happy this time, I should have listened to it last time!!
Committed r280715 (240306@main): <https://commits.webkit.org/240306@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 435038 [details].