Bug 228747 – fast/canvas/canvas-crash.html doesn't test what it intends to on iOS

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

fast/canvas/canvas-crash.html doesn't test what it intends to on iOS

[Tim Horton]

Created attachment 434850 [details]
Patch

[Tim Horton]

<rdar://problem/81423634>

[Simon Fraser (smfr)]

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

[Tim Horton]

Created attachment 434868 [details]
Patch

[Tim Horton]

windows newlines oO

[Tim Horton]

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 :|

[EWS]

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 228788

[Tim Horton]

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

[Sam Weinig]

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

[Sam Weinig]

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

[Tim Horton]

Yes, that. And we reset the max down to the default between each test so it's immediately TOO MUCH CANVAS

[Sam Weinig]

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

[Tim Horton]

(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

[Tim Horton]

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

[Tim Horton]

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

[Tim Horton]

Created attachment 435038 [details]
Patch

[Sam Weinig]

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

[Tim Horton]

Stress bot is happy this time, I should have listened to it last time!!

[EWS]

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