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.
Created attachment 107957 [details] Test case
Created attachment 107958 [details] Test case
Created attachment 107959 [details] Proof of concept, not for landing
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.
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 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
(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).
(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?
> > - 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.
Is there every a need for native drawing in a canvas2d? Could we just not allocate GDI backing store for canvases?
After some discussion, we're going with approach 1 - not using GDI objects at all. They should not be needed any more.
Created attachment 116826 [details] Patch
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 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().
Created attachment 116831 [details] Patch
Created attachment 121482 [details] Patch
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.
(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 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".
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.
Created attachment 121718 [details] Patch
(In reply to comment #19) > "Force use plain" seems grammatically awkward. I'd just say "Use plain". Done.
Looks good. r=me
Comment on attachment 121718 [details] Patch Clearing flags on attachment: 121718 Committed r104501: <http://trac.webkit.org/changeset/104501>
All reviewed patches have been landed. Closing bug.