Bug 58788

Summary: When flushing GrContext in ~PlatformContextSkia be sure correct GL context is set
Product: WebKit Reporter: Brian Salomon <bsalomon>
Component: PlatformAssignee: Brian Salomon <bsalomon>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, bsalomon, commit-queue, eric, jamesr, kbr, reed, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
Patch
none
Patch none

Description Brian Salomon 2011-04-18 08:07:32 PDT
When flushing GrContext in ~PlatformContextSkia be sure correct GL context is set
Comment 1 Brian Salomon 2011-04-18 08:21:06 PDT
Created attachment 90039 [details]
Patch
Comment 2 Brian Salomon 2011-04-18 08:30:43 PDT
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.
Comment 3 Eric Seidel (no email) 2011-04-18 08:45:56 PDT
Comment on attachment 90039 [details]
Patch

How do we test this?  Why are no tests required? (all changes require testing if possible/feasible).
Comment 4 Eric Seidel (no email) 2011-04-18 08:46:21 PDT
Comment on attachment 90039 [details]
Patch

Marking r- for now.  Please feel to mark r? again with explanation.
Comment 5 Kenneth Russell 2011-04-18 13:28:40 PDT
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.
Comment 6 James Robinson 2011-04-18 13:54:55 PDT
Can you confirm that at least one test progresses with this patch when SKIA_GPU is on and cite that test in the changelog?
Comment 7 Brian Salomon 2011-04-18 14:00:36 PDT
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.
Comment 8 Kenneth Russell 2011-04-18 15:21:21 PDT
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?
Comment 9 Eric Seidel (no email) 2011-04-18 15:22:33 PDT
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.
Comment 10 Brian Salomon 2011-04-19 05:55:19 PDT
Created attachment 90186 [details]
Patch
Comment 11 Brian Salomon 2011-04-19 05:56:53 PDT
(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.
Comment 12 Eric Seidel (no email) 2011-04-19 07:18:05 PDT
Comment on attachment 90186 [details]
Patch

Thanks.
Comment 13 WebKit Commit Bot 2011-04-19 09:25:25 PDT
Comment on attachment 90186 [details]
Patch

Clearing flags on attachment: 90186

Committed r84257: <http://trac.webkit.org/changeset/84257>
Comment 14 WebKit Commit Bot 2011-04-19 09:25:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 WebKit Review Bot 2011-04-19 10:42:00 PDT
http://trac.webkit.org/changeset/84257 might have broken Windows 7 Release (Tests)