Bug 58788 – When flushing GrContext in ~PlatformContextSkia be sure correct GL context is set

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Brian Salomon]

When flushing GrContext in ~PlatformContextSkia be sure correct GL context is set

[Brian Salomon]

Created attachment 90039 [details]
Patch

[Brian Salomon]

This fixes corruption that can occur when changing URLS on the same domain in a tab with the skia gpu flag enabled. The flush can happen into the compositor's context causing mayhem.

[Eric Seidel (no email)]

Comment on attachment 90039 [details]
Patch

How do we test this?  Why are no tests required? (all changes require testing if possible/feasible).

[Eric Seidel (no email)]

Comment on attachment 90039 [details]
Patch

Marking r- for now.  Please feel to mark r? again with explanation.

[Kenneth Russell]

Comment on attachment 90039 [details]
Patch

Theoretically this fix should fix failures in pixel layout tests when running with Skia's GPU backend, correct? (We aren't running tests in this configuration on bots yet.) If so then we should just go ahead and r+ this.

[James Robinson]

Can you confirm that at least one test progresses with this patch when SKIA_GPU is on and cite that test in the changelog?

[Brian Salomon]

After talking to Vangelis I think I'll need to add a browser test to exercise this. To test this we have to open a canvas page and then load a different page in the same renderer that uses accelerated compositing.

[Kenneth Russell]

In that case I would like to r+ this patch because we need to get the fix in; we can add a Chromium browser test afterward (and actually can't add such a test until this fix lands). Eric, is this okay with you?

[Eric Seidel (no email)]

Comment on attachment 90039 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests required.

This line remains patently false. :)  But if it's changed to explain how it can't be tested in WebKit but will be tested in Chrome code, that's fine.

[Brian Salomon]

Created attachment 90186 [details]
Patch

[Brian Salomon]

(In reply to comment #9)
> (From update of attachment 90039 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90039&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests required.
> 
> This line remains patently false. :)  But if it's changed to explain how it can't be tested in WebKit but will be tested in Chrome code, that's fine.

The new patch has a comment about how to test this in Chrome.

[Eric Seidel (no email)]

Comment on attachment 90186 [details]
Patch

Thanks.

[WebKit Commit Bot]

Comment on attachment 90186 [details]
Patch

Clearing flags on attachment: 90186

Committed r84257: <http://trac.webkit.org/changeset/84257>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/84257 might have broken Windows 7 Release (Tests)