Bug 79735 – [chromium] Work around IOSurface-related corruption during readback

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

[Kenneth Russell]

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.

[Kenneth Russell]

Created attachment 129166 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

Please wait for approval from [e-mail], [e-mail] or [e-mail] before submitting because this patch contains changes to the Chromium platform API.

[James Robinson]

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

[Dana Jansens]

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.

[Kenneth Russell]

(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.

[Kenneth Russell]

(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.

[James Robinson]

(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.

[Kenneth Russell]

(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.

[James Robinson]

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.

[James Robinson]

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.

[Kenneth Russell]

Created attachment 129351 [details]
Patch

[Kenneth Russell]

(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.

[James Robinson]

Comment on attachment 129351 [details]
Patch

Thanks.

[WebKit Review Bot]

Comment on attachment 129351 [details]
Patch

Clearing flags on attachment: 129351

Committed r109173: <http://trac.webkit.org/changeset/109173>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.