Bug 151825 – Place an upper bound on canvas pixel counts

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

[Brent Fulgham]

Malformed JavaScript can attempt to allocate massive numbers of canvas buffers, slowing down the user's system. We should place an upper sanity bound on this value (similar to what Blink does).

[Brent Fulgham]

<rdar://problem/23324916>

[Brent Fulgham]

Created attachment 266563 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 266563 [details]
Patch

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

> Source/WebCore/html/HTMLCanvasElement.cpp:109
> +        unsigned pixelsReleased = width() * height();

Why not use the existing HTMLCanvasElement::memoryCost() ?

I think this should also only do decrease the number if there is a m_imageBuffer.

> Source/WebCore/html/HTMLCanvasElement.cpp:195
> +    return ramSize() / (4 * 8);

Why?

> Source/WebCore/html/HTMLCanvasElement.cpp:219
> +            size_t requestedPixels = width() * height();
> +            if (numActivePixels + requestedPixels > maxActivePixels())

Use the same math as memoryCost().

[Brent Fulgham]

Comment on attachment 266563 [details]
Patch

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

>> Source/WebCore/html/HTMLCanvasElement.cpp:109
>> +        unsigned pixelsReleased = width() * height();
> 
> Why not use the existing HTMLCanvasElement::memoryCost() ?
> 
> I think this should also only do decrease the number if there is a m_imageBuffer.

memoryCost only returns a value once the image buffer is created. I want to know if I'm going to exceed the limit before then.

>> Source/WebCore/html/HTMLCanvasElement.cpp:195
>> +    return ramSize() / (4 * 8);
> 
> Why?

It's arbitrary, but based on the following thoughts: Blink limits the process to 1 GB. Assuming our typical-to-lower-end systems are using 8 GB, this calculation gives the number of 32 bit pixels that match that 1 GB limit.

>> Source/WebCore/html/HTMLCanvasElement.cpp:219
>> +            if (numActivePixels + requestedPixels > maxActivePixels())
> 
> Use the same math as memoryCost().

To use memoryCost we need to create the image buffer first.

[Brent Fulgham]

Comment on attachment 266563 [details]
Patch

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

>>> Source/WebCore/html/HTMLCanvasElement.cpp:219
>>> +            if (numActivePixels + requestedPixels > maxActivePixels())
>> 
>> Use the same math as memoryCost().
> 
> To use memoryCost we need to create the image buffer first.

... but, I could use memoryCost once the canvas context is created (and memoryCost when the context is destroyed) to keep track of actual memory use. That way, I don't improperly mark memory in use for a canvas that didn't actually use memory (if such things ever happen).

[Geoffrey Garen]

Comment on attachment 266563 [details]
Patch

I think we should record explicitly the number of pixels we added to numActivePixels so that we subtract that number back later. Relying on width and height seems risky, since they can change.

[Simon Fraser (smfr)]

(In reply to comment #6)
> Comment on attachment 266563 [details]
> Patch
> 
> I think we should record explicitly the number of pixels we added to
> numActivePixels so that we subtract that number back later. Relying on width
> and height seems risky, since they can change.

size changes are handled via setSurfaceSize(), and this code needs to account for them.

[Brent Fulgham]

Created attachment 266580 [details]
Patch

[Build Bot]

Comment on attachment 266580 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 266583 [details]
Archive of layout-test-results from ews114 for mac-yosemite

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

[Simon Fraser (smfr)]

Comment on attachment 266580 [details]
Patch

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

> Source/WebCore/html/HTMLCanvasElement.cpp:618
> +    if (m_imageBuffer)
> +        removeFromActivePixelMemory(memoryCost());

You could tie the book-keeping more closely with the buffer allocation/deallocation by adding HTMLCanvasElement::setImageBuffer(), and doing the add/remove pixel count stuff inside it.

[Brent Fulgham]

Comment on attachment 266580 [details]
Patch

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

>> Source/WebCore/html/HTMLCanvasElement.cpp:618
>> +        removeFromActivePixelMemory(memoryCost());
> 
> You could tie the book-keeping more closely with the buffer allocation/deallocation by adding HTMLCanvasElement::setImageBuffer(), and doing the add/remove pixel count stuff inside it.

Sure!

[Brent Fulgham]

Note that the actual memory cost of canvas elements is difficult to assess, because there are code paths where CanvasRenderingContext2D creates and returns blank image buffers. So, the actual memory use is more than just the internal image buffer.

[Brent Fulgham]

Created attachment 266666 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 266666 [details]
Patch

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

> Source/WebCore/html/HTMLCanvasElement.cpp:123
> +    // Do not manually delete the image buffer -- it will be cleaned up by the JSC collector.
> +    removeFromActivePixelMemory(memoryCost());
> +
>      m_context = nullptr; // Ensure this goes away before the ImageBuffer.
>  }

Why not setImageBuffer(nullptr) at the end of the function, so you don't need to call removeFromActivePixelMemory() explicitly.

[Brent Fulgham]

Comment on attachment 266666 [details]
Patch

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

>> Source/WebCore/html/HTMLCanvasElement.cpp:123
>>  }
> 
> Why not setImageBuffer(nullptr) at the end of the function, so you don't need to call removeFromActivePixelMemory() explicitly.

For the same reason as the comment in 'getContext':
    // FIXME: The code depends on the context not going away once created, to prevent JS from
    // seeing a dangling pointer. So for now we will disallow the context from being changed"

If I call 'setImageBuffer(nullptr)' here, JSC gets very upset that the image buffer went away without it's control.

I can add a comment to that effect.

> Source/WebCore/html/HTMLCanvasElement.cpp:204
> +        maxPixelMemory = std::max(ramSize() / 4, 1024 * MB);

I started off with ramSize() / 8, but some of our tests want to allocate more than that. (ramSize() / 8) + 1 seems to work, but this might be a slightly less drastic change from our current "allocate whatever you want" approach.

Some test machines are resource constrained, so I set a lower bound on the max memory.

[Brent Fulgham]

Comment on attachment 266666 [details]
Patch

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

>>> Source/WebCore/html/HTMLCanvasElement.cpp:123
>>>  }
>> 
>> Why not setImageBuffer(nullptr) at the end of the function, so you don't need to call removeFromActivePixelMemory() explicitly.
> 
> For the same reason as the comment in 'getContext':
>     // FIXME: The code depends on the context not going away once created, to prevent JS from
>     // seeing a dangling pointer. So for now we will disallow the context from being changed"
> 
> If I call 'setImageBuffer(nullptr)' here, JSC gets very upset that the image buffer went away without it's control.
> 
> I can add a comment to that effect.

Oh! Yes, your way works perfectly. Much better -- I'll change it.

[Brent Fulgham]

Created attachment 266674 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 266674 [details]
Patch

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

> Source/WebCore/html/HTMLCanvasElement.cpp:608
> +        stringBuilder.appendLiteral("Total canvas memory use exceeds the maximum limit ( ");

No space after the paren in the string.

> Source/WebCore/html/HTMLCanvasElement.cpp:611
> +        document().addConsoleMessage(MessageSource::JS, MessageLevel::Warning, stringBuilder.toString());

Does this string look OK in the console?

[Build Bot]

Comment on attachment 266674 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 266678 [details]
Archive of layout-test-results from ews115 for mac-yosemite

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

[Brent Fulgham]

Comment on attachment 266666 [details]
Patch

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

>>>> Source/WebCore/html/HTMLCanvasElement.cpp:123
>>>>  }
>>> 
>>> Why not setImageBuffer(nullptr) at the end of the function, so you don't need to call removeFromActivePixelMemory() explicitly.
>> 
>> For the same reason as the comment in 'getContext':
>>     // FIXME: The code depends on the context not going away once created, to prevent JS from
>>     // seeing a dangling pointer. So for now we will disallow the context from being changed"
>> 
>> If I call 'setImageBuffer(nullptr)' here, JSC gets very upset that the image buffer went away without it's control.
>> 
>> I can add a comment to that effect.
> 
> Oh! Yes, your way works perfectly. Much better -- I'll change it.

Wait -- this is still failing on the debug test bots due to the GC being angry about the ImageBuffer going missing. I'll switch back to my previous version and retry the test.

[Brent Fulgham]

Created attachment 266681 [details]
Patch

[Brent Fulgham]

Created attachment 266686 [details]
Patch

[Brent Fulgham]

Comment on attachment 266686 [details]
Patch

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

> Source/WebCore/html/HTMLCanvasElement.cpp:121
> +    releaseImageBufferAndContext();

Maybe name this 'releaseImageBufferAndContextStateSaver"?

[Brent Fulgham]

(In reply to comment #22)
> > Oh! Yes, your way works perfectly. Much better -- I'll change it.
> 
> Wait -- this is still failing on the debug test bots due to the GC being
> angry about the ImageBuffer going missing. I'll switch back to my previous
> version and retry the test.

Okay -- as we discussed in person, this was due to the graphics context state saver not being cleared when the ImageBuffer was deallocated. This is now fixed, and with that everything works properly.

[Brent Fulgham]

Comment on attachment 266686 [details]
Patch

Actually rubber-stamped by Simon Fraser.

[Brent Fulgham]

Resetting the r+ based on Simon's earlier review (plus in-person discussion of final fix) to allow webkit-patch to do its thing.

[Brent Fulgham]

Committed r193500: <http://trac.webkit.org/changeset/193500>

[Ryan Haddad]

This change seems to be causing the following layout test to fail on El Capitan WK2 Debug:

fast/canvas/canvas-too-large-to-draw.html

<https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r193649%20(1513)/results.html>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 152143

[Brent Fulgham]

The test bots that were failing are 8 GB Mac Minis. This means that we set the upper limit of memory use to 2 GB.

These large canvas tests use ~268 MB canvas sizes. Even if we made two of them that should only reach about 0.5 GB of memory use.

I think the issue is that the canvas elements from previous test runs remain in the JSC heap until a garbage collection runs. This test case gets hit in the midst of a number of canvas tests, which consume enough “pixel memory” that we can’t allocate the gargantuan canvas and we fail the test.

[Brent Fulgham]

Wait. I can't do math.

The tests try to allocate:
(1) a 16384 x 16384 pixel canvas, which consumes (at 4 bytes per pixel) = 1,073,741,824 bytes.
(2) a 16385 x 16384 pixel canvas, which consumes (at 4 bytes per pixel) = 1,073,807,360 bytes.

On an 8 GB machine, we allow 2 GB of RAM to be used: 2,147,483,648 bytes.

(1) + (2) = 2,147,549,184 which exceeds the allowed memory.

I'll adjust the allowed memory minimum to handle this case.

[Brent Fulgham]

Created attachment 267638 [details]
Patch (Revised with memory limit bumped to satisfy test criteria)

[Brent Fulgham]

Comment on attachment 267638 [details]
Patch (Revised with memory limit bumped to satisfy test criteria)

This is a small update to satisfy 'fast/canvas/canvas-stroke-path.html'. I'm uploading it for "review" so that I can get EWS confirmation on a wider range of hardware before I re-land.

[Brent Fulgham]

Tests are green. I'm relanding the change.

[Brent Fulgham]

Committed r194290: <http://trac.webkit.org/changeset/194290>

[Brent Fulgham]

Comment on attachment 267638 [details]
Patch (Revised with memory limit bumped to satisfy test criteria)

Clearing review flag now that this is landed.