Bug 68420 – [chromium win] Creating lots of temporary canvas contexts will crash.

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

[Ben Wells]

By creating lots of canvas objects and getting their context, it is easy to run out of Windows GDI objects and crash. Even if the contexts are not referenced, the garbage collector will not startup up unless free memory is running out.

The attachments show two contrived examples which will crash Chromium on Windows. These are reduced from a crashing page which was using the Google Maps API, so these kind of usage patterns can be expected to happen.

There are at least two approaches to fixing the problem as:

1. Make skia on Windows use less GDI objects. Safari on Windows has no problems with the attached test cases; maybe there is something we can do to not use GDI objects, or not use so many.

2. Do garbage collection when we are running low on handles. I've got a proof of context change showing this approach, but it will need some work if we go with this approach. This will only help if the contexts and canvas objects are no longer in use.

[Ben Wells]

Created attachment 107957 [details]
Test case

[Ben Wells]

Created attachment 107958 [details]
Test case

[Ben Wells]

Created attachment 107959 [details]
Proof of concept, not for landing

[Ben Wells]

Comment on attachment 107959 [details]
Proof of concept, not for landing

This fix is not ready for landing, but it shows the second approach we could take to help with this problem.

If this approach is the way to go, these changes (at least) should be made:

- be smarter about when to signal low memory by using the Windows registroy to find the per-process limit (its not always 10000), and using GetGUIResources to find out if we are getting close to that.

- push this change down (e.g. down to skia graphics context layer), to the lowest layer we can put this and still have enough context to tell V8 we are running low on resources.

[James Robinson]

Just to be clear, are you talking about the accelerated case or the non-accelerated case?  Also, are you running out of the per-process GDI limit or the global GDI limit?

[Early Warning System Bot]

Comment on attachment 107959 [details]
Proof of concept, not for landing

Attachment 107959 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9765103

[Ben Wells]

(In reply to comment #5)
> Just to be clear, are you talking about the accelerated case or the non-accelerated case?  Also, are you running out of the per-process GDI limit or the global GDI limit?

- I think this is the accelerated case, the problem has been observed in a standard Chrome version 15 and also a standard Debug build of Chromeium ToT / WebKit ToT

- This is the per-process GDI limit (usually 10000).

[Vangelis Kokkevis]

(In reply to comment #7)
> (In reply to comment #5)
> > Just to be clear, are you talking about the accelerated case or the non-accelerated case?  Also, are you running out of the per-process GDI limit or the global GDI limit?
> 
> - I think this is the accelerated case, the problem has been observed in a standard Chrome version 15 and also a standard Debug build of Chromeium ToT / WebKit ToT

Actually your test case won't trigger the accelerated canvas path as the canvases you create are too small.  

> 
> - This is the per-process GDI limit (usually 10000).

I'm wondering though whether it's not a HTML canvas issue but rather a regular SkCanvas.  Each composited layer, via its layer painter, creates and holds on to an SkCanvas until the layer is destroyed. If a lot of tabs are opened I can see how they will add up.  Any idea how many GDI objects are used per skia canvas?

[Ben Wells]

> > - This is the per-process GDI limit (usually 10000).
> 
> I'm wondering though whether it's not a HTML canvas issue but rather a regular SkCanvas.  Each composited layer, via its layer painter, creates and holds on to an SkCanvas until the layer is destroyed. If a lot of tabs are opened I can see how they will add up.  Any idea how many GDI objects are used per skia canvas?

I haven't checked wrt SkCanvas or RenderLayer, but from observation it seems each HTML canvas context is using one GDI object - for the test cases posted at least.

Each process has a limit of 10000 GDI objects (by default), with a 'theoretical limit' per session of 65,536 (http://msdn.microsoft.com/en-us/library/ms724291(VS.85).aspx).

Different tabs are generally in different processes so opening up lots of tabs might not be such a problem.

[Brian Salomon]

Is there every a need for native drawing in a canvas2d? Could we just not allocate GDI backing store for canvases?

[Ben Wells]

After some discussion, we're going with approach 1 - not using GDI objects at all. They should not be needed any more.

[Justin Novosad]

Created attachment 116826 [details]
Patch

[Justin Novosad]

Just submitted a patch the avoids allocating a GDI object for each ImageBuffer.  Only ImageBuffer associated to HTML canvases are affected because they draw text through skia.  Other use case of ImageBuffer still use GDI to draw text.

Note to reviewers: I was not sure whether it is appropriate to add the test HTML file as a new test. Is it appropriate to use a LayoutTest to verify resource exhaustion behavior/limits? Also the test is not very fast (creates 100000 canvases in a loop).  Do we want this as a test?

[Stephen White]

Comment on attachment 116826 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:96
> +static SkCanvas* createSoftwareCanvas(const IntSize& size)

This function should probably be named createNonGDICanvas().

[Justin Novosad]

Created attachment 116831 [details]
Patch

[Justin Novosad]

Created attachment 121482 [details]
Patch

[James Robinson]

Comment on attachment 121482 [details]
Patch

This looks really nice! I think Stephen should probably double check it, since he's more familiar with this part of the woods.

Related question: Can we use non-GDI backing stores for all ganesh-accelerated skia painting now? That might be useful elsewhere (like in the compositor), especially if it means we get to pick whatever pixel configuration we want.

[Justin Novosad]

(In reply to comment #17)
> Related question: Can we use non-GDI backing stores for all ganesh-accelerated skia painting now? That might be useful elsewhere (like in the compositor), especially if it means we get to pick whatever pixel configuration we want.

Anywhere we may draw GDI text or widgets (e.g. HTML forms, browser UI), we still need to allocate the backing store using GDI :-( Unfortunately, I don't think there are currently many places where it is safe to remove GDI. Like you suggest, there may be opportunities in the compositor...

[Stephen White]

Comment on attachment 121482 [details]
Patch

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

Please clean up the grammar a bit.  Looks good other than that.

> Source/WebCore/platform/graphics/ImageBuffer.h:62
> +        UnacceleratedNonPlatformBuffer, // Force use plain memory allocation rather than platform API to allocate backing store.

"Force use plain" seems grammatically awkward.  I'd just say "Use plain".

[Mike Reed]

There should be no more paths that draw GDI text, but form-controls still use GDI. An effort to simulate those with Skia, or to draw those into a tmp offscreen, and then copy that onto skia via drawBitmap would allow us to remove the GDI-pixel requirement.

[Justin Novosad]

Created attachment 121718 [details]
Patch

[Justin Novosad]

(In reply to comment #19)
> "Force use plain" seems grammatically awkward.  I'd just say "Use plain".

Done.

[Stephen White]

Looks good.  r=me

[WebKit Review Bot]

Comment on attachment 121718 [details]
Patch

Clearing flags on attachment: 121718

Committed r104501: <http://trac.webkit.org/changeset/104501>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.