Bug 112156 - New context extensions restored improperly
Summary: New context extensions restored improperly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-12 09:10 PDT by R S
Modified: 2013-03-15 15:56 PDT (History)
7 users (show)

See Also:

Attachments
Patch (1.47 KB, patch)
2013-03-12 09:30 PDT, R S 		no flags Details | Formatted Diff | Diff
Patch (1.47 KB, patch)
2013-03-12 10:38 PDT, R S 		no flags Details | Formatted Diff | Diff
Patch (1.54 KB, patch)
2013-03-13 09:06 PDT, R S 		no flags Details | Formatted Diff | Diff
Patch (1.61 KB, patch)
2013-03-15 15:00 PDT, R S 		no flags Details | Formatted Diff | Diff
Patch (1.90 KB, patch)
2013-03-15 15:10 PDT, R S 		no flags Details | Formatted Diff | Diff
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description R S 2013-03-12 09:10:32 PDT 
When a context is lost and then restored, a new GraphicsContext3D object is created. initializeNewContext() is run to set up the new context, but setupFlags is not run. This means that the extensions are not reloaded and therefore will not work after restoration.
Comment 1 R S 2013-03-12 09:30:30 PDT 
Created attachment 192752 [details]
Patch
Comment 2 R S 2013-03-12 10:38:50 PDT 
Created attachment 192762 [details]
Patch
Comment 3 Mike West 2013-03-13 05:12:50 PDT 
Comment on attachment 192762 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Sets up extensions after new context is created for restoration

Please add a bit more context here: why is this change necessary?

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:5825
> +    setupFlags();

Does this produce any web-visible change? If so, it would be excellent to add a test to ensure that the now-correct behavior doesn't regress.
Comment 4 R S 2013-03-13 09:06:16 PDT 
Created attachment 192933 [details]
Patch
Comment 5 R S 2013-03-13 13:19:21 PDT 
(In reply to comment #3)
> Does this produce any web-visible change? If so, it would be excellent to add a test to ensure that the now-correct behavior doesn't regress.

So far as I can tell it does not produce a web-visible change.
Comment 6 Rob Buis 2013-03-15 14:52:53 PDT 
Comment on attachment 192933 [details]
Patch

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

According to mvujovic and achicu this change makes sense, so this just needs the text explanation before it can go in.

> Source/WebCore/ChangeLog:9
> +        Previous extensions set up is lost when context is initially lost.

This needs a line about why the test can't be made.
Comment 7 R S 2013-03-15 15:00:12 PDT 
Created attachment 193376 [details]
Patch
Comment 8 Rob Buis 2013-03-15 15:06:57 PDT 
Comment on attachment 193376 [details]
Patch

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

Please explain a bit better.

> Source/WebCore/ChangeLog:9
> +        Previous extensions set up is lost when context is initially lost.

Add empty line here.

> Source/WebCore/ChangeLog:10
> +        Cannot create a test as change is not visible from the web.

On irc you gave a more in-depth reason, please combine with this remark.
Comment 9 R S 2013-03-15 15:10:57 PDT 
Created attachment 193380 [details]
Patch
Comment 10 Rob Buis 2013-03-15 15:13:51 PDT 
Comment on attachment 193380 [details]
Patch

LGTM.
Comment 11 WebKit Review Bot 2013-03-15 15:56:42 PDT 
Comment on attachment 193380 [details]
Patch

Clearing flags on attachment: 193380

Committed r145955: <http://trac.webkit.org/changeset/145955>
Comment 12 WebKit Review Bot 2013-03-15 15:56:46 PDT 
All reviewed patches have been landed.  Closing bug.