Bug 68420 - [chromium win] Creating lots of temporary canvas contexts will crash.
Summary: [chromium win] Creating lots of temporary canvas contexts will crash.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Unspecified
: P2 Normal
Assignee: Justin Novosad
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-19 19:18 PDT by Ben Wells
Modified: 2012-01-09 15:53 PST (History)
9 users (show)

See Also:


Attachments
Test case (1.16 KB, text/html)
2011-09-19 19:19 PDT, Ben Wells
no flags Details
Test case (1.09 KB, text/html)
2011-09-19 19:20 PDT, Ben Wells
no flags Details
Proof of concept, not for landing (2.50 KB, patch)
2011-09-19 19:22 PDT, Ben Wells
no flags Details | Formatted Diff | Diff
Patch (3.46 KB, patch)
2011-11-28 14:50 PST, Justin Novosad
no flags Details | Formatted Diff | Diff
Patch (3.46 KB, patch)
2011-11-28 15:07 PST, Justin Novosad
no flags Details | Formatted Diff | Diff
Patch (3.62 KB, patch)
2012-01-06 13:13 PST, Justin Novosad
no flags Details | Formatted Diff | Diff
Patch (3.67 KB, patch)
2012-01-09 13:19 PST, Justin Novosad
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Wells 2011-09-19 19:18:33 PDT
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.
Comment 1 Ben Wells 2011-09-19 19:19:09 PDT
Created attachment 107957 [details]
Test case
Comment 2 Ben Wells 2011-09-19 19:20:00 PDT
Created attachment 107958 [details]
Test case
Comment 3 Ben Wells 2011-09-19 19:22:19 PDT
Created attachment 107959 [details]
Proof of concept, not for landing
Comment 4 Ben Wells 2011-09-19 19:30:14 PDT
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.
Comment 5 James Robinson 2011-09-19 19:34:49 PDT
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?
Comment 6 Early Warning System Bot 2011-09-19 19:41:14 PDT
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
Comment 7 Ben Wells 2011-09-19 20:17:47 PDT
(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).
Comment 8 Vangelis Kokkevis 2011-09-19 21:38:01 PDT
(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?
Comment 9 Ben Wells 2011-09-19 21:57:46 PDT
> > - 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.
Comment 10 Brian Salomon 2011-09-20 07:40:59 PDT
Is there every a need for native drawing in a canvas2d? Could we just not allocate GDI backing store for canvases?
Comment 11 Ben Wells 2011-09-22 00:40:10 PDT
After some discussion, we're going with approach 1 - not using GDI objects at all. They should not be needed any more.
Comment 12 Justin Novosad 2011-11-28 14:50:10 PST
Created attachment 116826 [details]
Patch
Comment 13 Justin Novosad 2011-11-28 14:57:37 PST
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?
Comment 14 Stephen White 2011-11-28 15:00:52 PST
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().
Comment 15 Justin Novosad 2011-11-28 15:07:30 PST
Created attachment 116831 [details]
Patch
Comment 16 Justin Novosad 2012-01-06 13:13:16 PST
Created attachment 121482 [details]
Patch
Comment 17 James Robinson 2012-01-06 13:35:44 PST
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.
Comment 18 Justin Novosad 2012-01-06 13:53:45 PST
(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...
Comment 19 Stephen White 2012-01-06 14:10:23 PST
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".
Comment 20 Mike Reed 2012-01-09 04:57:30 PST
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.
Comment 21 Justin Novosad 2012-01-09 13:19:09 PST
Created attachment 121718 [details]
Patch
Comment 22 Justin Novosad 2012-01-09 14:00:40 PST
(In reply to comment #19)
> "Force use plain" seems grammatically awkward.  I'd just say "Use plain".

Done.
Comment 23 Stephen White 2012-01-09 14:27:30 PST
Looks good.  r=me
Comment 24 WebKit Review Bot 2012-01-09 15:53:52 PST
Comment on attachment 121718 [details]
Patch

Clearing flags on attachment: 121718

Committed r104501: <http://trac.webkit.org/changeset/104501>
Comment 25 WebKit Review Bot 2012-01-09 15:53:58 PST
All reviewed patches have been landed.  Closing bug.