Bug 79735 - [chromium] Work around IOSurface-related corruption during readback
Summary: [chromium] Work around IOSurface-related corruption during readback
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.7
: P2 Normal
Assignee: Kenneth Russell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-27 19:07 PST by Kenneth Russell
Modified: 2012-02-28 17:17 PST (History)
9 users (show)

See Also:


Attachments
Patch (12.91 KB, patch)
2012-02-27 19:16 PST, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (5.08 KB, patch)
2012-02-28 16:23 PST, Kenneth Russell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2012-02-27 19:07:58 PST
In http://crbug.com/99393 , crashes have been observed during ReadPixels operations in Chromium's GPU process on 10.7. Per the analysis in that bug, it has been demonstrated that the ReadPixels call performed by the thumbnailer causes corruption of the output of later ReadPixels calls (in different OpenGL contexts). The compositor's context renders into an FBO with an IOSurface-backed color attachment, and the ReadPixels from the IOSurface appears to be the problem. I haven't been able to reproduce the crashes, but suspect that they are caused by the later ReadPixels calls writing past the end of the input buffer and corrupting the heap.

jbauman@ suggested to use copyTexImage2D to get the IOSurface's contents into a temporary texture, and then do the ReadPixels operation from an FBO with that texture bound. This workaround successfully avoids the underlying bug and has the significant advantage that the associated code is contained within one method.
Comment 1 Kenneth Russell 2012-02-27 19:16:33 PST
Created attachment 129166 [details]
Patch
Comment 2 WebKit Review Bot 2012-02-27 19:18:09 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.

Please wait for approval from fishd@chromium.org, abarth@webkit.org or jamesr@chromium.org before submitting because this patch contains changes to the Chromium platform API.
Comment 3 James Robinson 2012-02-27 19:51:14 PST
Comment on attachment 129166 [details]
Patch

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

Why do we need to plumb this through all of our API layers?  What do you intend will set this and on what criteria?  If this is just for certain versions of OS X we can #ifdef to it mac and use gestalt to limit it at runtime to the version(s) affected.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1073
> +        GLC(context, context->copyTexImage2D(GraphicsContext3D::TEXTURE_2D, 0, GraphicsContext3D::RGBA, 0, 0, rect.maxX(), rect.maxY(), 0));

this seems to copy more than strictly necessary - could you just copy rect.x() / rect.y() / rect.width() / rect.height() and adjust the rect's offset as appropriate? hopefully we aren't doing enough readbacks where this matters too much
Comment 4 Dana Jansens 2012-02-28 07:23:08 PST
Hm interesting. I've been seeing crashes doing readbacks for background filters in the browser compositor (but not the renderer). I wonder if it is related.
Comment 5 Kenneth Russell 2012-02-28 10:04:58 PST
(In reply to comment #3)
> (From update of attachment 129166 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129166&action=review
> 
> Why do we need to plumb this through all of our API layers?  What do you intend will set this and on what criteria?  If this is just for certain versions of OS X we can #ifdef to it mac and use gestalt to limit it at runtime to the version(s) affected.

The workaround is needed on Mac OS X 10.7 and (presumably) later, and will be set by code in Chrome's src/content/browser/tab_contents/tab_contents.cc . The reason it needs to be plumbed all the way through is that the workaround must be able to be turned off with a command line flag so that Apple can reproduce the original problem and fix the underlying OS bug.

> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1073
> > +        GLC(context, context->copyTexImage2D(GraphicsContext3D::TEXTURE_2D, 0, GraphicsContext3D::RGBA, 0, 0, rect.maxX(), rect.maxY(), 0));
> 
> this seems to copy more than strictly necessary - could you just copy rect.x() / rect.y() / rect.width() / rect.height() and adjust the rect's offset as appropriate? hopefully we aren't doing enough readbacks where this matters too much

This code path is only taken very infrequently -- for the thumbnailer -- and I think it's better to keep the code simple.
Comment 6 Kenneth Russell 2012-02-28 10:05:52 PST
(In reply to comment #4)
> Hm interesting. I've been seeing crashes doing readbacks for background filters in the browser compositor (but not the renderer). I wonder if it is related.

On what platforms are you seeing the readback related crashes? If it isn't Mac OS 10.7 then it's a different problem -- though maybe related.
Comment 7 James Robinson 2012-02-28 10:19:54 PST
(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 129166 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=129166&action=review
> > 
> > Why do we need to plumb this through all of our API layers?  What do you intend will set this and on what criteria?  If this is just for certain versions of OS X we can #ifdef to it mac and use gestalt to limit it at runtime to the version(s) affected.
> 
> The workaround is needed on Mac OS X 10.7 and (presumably) later, and will be set by code in Chrome's src/content/browser/tab_contents/tab_contents.cc . The reason it needs to be plumbed all the way through is that the workaround must be able to be turned off with a command line flag so that Apple can reproduce the original problem and fix the underlying OS bug.

Can you just spin Apple a build without the workaround or put them at an old version? This is a large wart to have on lots of our code for a one-off that won't help our users.
Comment 8 Kenneth Russell 2012-02-28 12:01:59 PST
(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > (From update of attachment 129166 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=129166&action=review
> > > 
> > > Why do we need to plumb this through all of our API layers?  What do you intend will set this and on what criteria?  If this is just for certain versions of OS X we can #ifdef to it mac and use gestalt to limit it at runtime to the version(s) affected.
> > 
> > The workaround is needed on Mac OS X 10.7 and (presumably) later, and will be set by code in Chrome's src/content/browser/tab_contents/tab_contents.cc . The reason it needs to be plumbed all the way through is that the workaround must be able to be turned off with a command line flag so that Apple can reproduce the original problem and fix the underlying OS bug.
> 
> Can you just spin Apple a build without the workaround or put them at an old version? This is a large wart to have on lots of our code for a one-off that won't help our users.

I've just investigated what would be necessary to do this with inline API calls and the code is not completely trivial. Gestalt() has undesirable side effects (see DarwinMajorVersionInternal in src/base/mac/mac_util.mm) and parsing the release information from uname() is a chunk of code (see the same function).

There are advantages to being able to toggle the workaround on the top of tree builds while the bug is being fixed by Apple. I will make sure to take out the workaround code once it is no longer needed and will file a bug to that effect. I'd like to move forward with the workaround in this form.
Comment 9 James Robinson 2012-02-28 12:44:54 PST
We're currently shipping many pieces of code that uses Gestalt for exactly this, and have been for some time:

http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp#L66
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/mac/FontMac.mm#L55
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm#L227
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/chromium/CrossProcessFontLoading.mm#L44

If you think this is problematic, then we should address it systematically.  I don't think it's a wise idea to do something arbitrarily different here.  It's accumulating unnecessary cruft that somebody else will have to clean up later.
Comment 10 James Robinson 2012-02-28 12:45:19 PST
Comment on attachment 129166 [details]
Patch

You seem really interested in a rubber stamp, so I'm happy to give one.  I think this patch is not wrong, but it could be better.
Comment 11 Kenneth Russell 2012-02-28 16:23:43 PST
Created attachment 129351 [details]
Patch
Comment 12 Kenneth Russell 2012-02-28 16:24:36 PST
(In reply to comment #10)
> (From update of attachment 129166 [details])
> You seem really interested in a rubber stamp, so I'm happy to give one.  I think this patch is not wrong, but it could be better.

Thanks for the feedback. Revised the patch to do the OS version detection in line using the same mechanism as the other code you pointed out. Also filed a Radar and referenced it in the comment.
Comment 13 James Robinson 2012-02-28 16:55:27 PST
Comment on attachment 129351 [details]
Patch

Thanks.
Comment 14 WebKit Review Bot 2012-02-28 17:17:47 PST
Comment on attachment 129351 [details]
Patch

Clearing flags on attachment: 129351

Committed r109173: <http://trac.webkit.org/changeset/109173>
Comment 15 WebKit Review Bot 2012-02-28 17:17:52 PST
All reviewed patches have been landed.  Closing bug.