Bug 75053 – Fix semantics of WEBKIT_WEBGL_lose_context

• Component: WebGL
• Status: RESOLVED FIXED

[Kenneth Russell]

After recent WebGL specification updates, the behavior of the WEBGL_lose_context extension in WebKit is out of date, causing conformance test failures. Specifically, calling loseContext() must cause the context to be immediately lost, but should not dispatch the webglcontextlost event until the flow of control returns from JavaScript. Calling restoreContext() must defer the restoration of the context, and dispatch of the associated webglcontextrestored event, until the flow of control returns from JavaScript.

[Kenneth Russell]

Created attachment 120420 [details]
Patch

[James Robinson]

Comment on attachment 120420 [details]
Patch

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

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:542
> +    // Cancel any pending timers.
> +    m_dispatchContextLostEventTimer.stop();
> +    m_restoreTimer.stop();

You don't need to do this, ~TimerBase() does. I think having this here will just confuse readers.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4126
> +    // Always defer the dispatch of the context lost event to make sure that any
> +    // JavaScript code on the stack has returned.

this comment isn't fully accurate, using a timer here only defers until the event loop is pumped which does not imply that all JavaScript on the stack has returned - consider what happens if script pops a modal dialog, for example. the logic actually coded here has the same effect of spec code that says "queue a task"

if you care about making sure that all JavaScript code on the stack (if there is any) has returned, you need to look into the active DOM object handling. if you don't, then just updating this comment is sufficient.

> Source/WebCore/html/canvas/WebGLRenderingContext.h:375
> +    class DispatchContextLostEventTimer : public TimerBase {

This isn't the way we normally do timers and seems very verbose for the problem it is trying to solve. The more standard way is for WebGLRenderingContext to have a member of type Timer<WebGLRenderingContext> initialized to refer to a member function that does the right thing. Example:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/dom/Document.cpp&exact_package=chromium&q=Document%20m_styleRecalcTimer&type=cs&l=386

with that solution you need one member variable and one member function which should save you a lot of lines of code

[Kenneth Russell]

Created attachment 121165 [details]
Patch

[Kenneth Russell]

(In reply to comment #2)
> (From update of attachment 120420 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120420&action=review
> 
> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:542
> > +    // Cancel any pending timers.
> > +    m_dispatchContextLostEventTimer.stop();
> > +    m_restoreTimer.stop();
> 
> You don't need to do this, ~TimerBase() does. I think having this here will just confuse readers.

Thanks, that was leftover code from a diagnosis of failures of one of the test cases. Removed.

> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4126
> > +    // Always defer the dispatch of the context lost event to make sure that any
> > +    // JavaScript code on the stack has returned.
> 
> this comment isn't fully accurate, using a timer here only defers until the event loop is pumped which does not imply that all JavaScript on the stack has returned - consider what happens if script pops a modal dialog, for example. the logic actually coded here has the same effect of spec code that says "queue a task"

Thanks. Updated the comment rather than changing the logic, since it sounds like the new code is spec compliant.

> if you care about making sure that all JavaScript code on the stack (if there is any) has returned, you need to look into the active DOM object handling. if you don't, then just updating this comment is sufficient.
> 
> > Source/WebCore/html/canvas/WebGLRenderingContext.h:375
> > +    class DispatchContextLostEventTimer : public TimerBase {
> 
> This isn't the way we normally do timers and seems very verbose for the problem it is trying to solve. The more standard way is for WebGLRenderingContext to have a member of type Timer<WebGLRenderingContext> initialized to refer to a member function that does the right thing. Example:
> http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/dom/Document.cpp&exact_package=chromium&q=Document%20m_styleRecalcTimer&type=cs&l=386
> 
> with that solution you need one member variable and one member function which should save you a lot of lines of code

Thanks very much for this tip. Changed; this simplified the code significantly.

[James Robinson]

Comment on attachment 121165 [details]
Patch

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

R=me. there are a number of setTimeout(..., 1)s which worry me a bit, we normally construct tests so the timeout value doesn't matter and pass 0 in just to be clear. commented on them inline. otherwise looks great

> LayoutTests/fast/canvas/webgl/WebGLContextEvent.html:40
> +        }, 1);

the choice of '1' here is a little surprising, normally we use 0 delays for tests to get a clean callstack. if the '1' is significant then i worry this is flaky

> LayoutTests/fast/canvas/webgl/WebGLContextEvent.html:55
> +        }, 1);

same here - should probably be 0

> LayoutTests/fast/canvas/webgl/context-lost-restored.html:1
>  <html>

why doesn't this test have an HTML5 doctype "<!DOCTYPE html>" ? our tests normally do unless they're specifically for quirks mode

> LayoutTests/fast/canvas/webgl/context-lost-restored.html:3
> +<meta charset="utf-8">

i don't think we normally do this

> LayoutTests/fast/canvas/webgl/context-lost-restored.html:70
> +       }, 1);

0 here, please

> LayoutTests/fast/canvas/webgl/context-lost-restored.html:108
> +      }, 1);

0

> LayoutTests/fast/canvas/webgl/context-lost.html:3
> +<meta charset="utf-8">

this is unusual. can this test have a <!DOCTYPE html> please?

[Kenneth Russell]

(In reply to comment #5)
> (From update of attachment 121165 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121165&action=review
> 
> R=me. there are a number of setTimeout(..., 1)s which worry me a bit, we normally construct tests so the timeout value doesn't matter and pass 0 in just to be clear. commented on them inline. otherwise looks great
> 
> > LayoutTests/fast/canvas/webgl/WebGLContextEvent.html:40
> > +        }, 1);
> 
> the choice of '1' here is a little surprising, normally we use 0 delays for tests to get a clean callstack. if the '1' is significant then i worry this is flaky

Yes, there's no good reason this isn't 0. I believe I changed it to 1 while debugging an issue with one of the tests. Changed back to 0 here and below.

> > LayoutTests/fast/canvas/webgl/WebGLContextEvent.html:55
> > +        }, 1);
> 
> same here - should probably be 0
> 
> > LayoutTests/fast/canvas/webgl/context-lost-restored.html:1
> >  <html>
> 
> why doesn't this test have an HTML5 doctype "<!DOCTYPE html>" ? our tests normally do unless they're specifically for quirks mode

No good reason; I didn't know this was the convention for layout tests. Fixed.

> > LayoutTests/fast/canvas/webgl/context-lost-restored.html:3
> > +<meta charset="utf-8">
> 
> i don't think we normally do this

Removed in both tests.

> > LayoutTests/fast/canvas/webgl/context-lost-restored.html:70
> > +       }, 1);
> 
> 0 here, please
> 
> > LayoutTests/fast/canvas/webgl/context-lost-restored.html:108
> > +      }, 1);
> 
> 0
> 
> > LayoutTests/fast/canvas/webgl/context-lost.html:3
> > +<meta charset="utf-8">
> 
> this is unusual. can this test have a <!DOCTYPE html> please?

Done.

[Kenneth Russell]

Committed r104102: <http://trac.webkit.org/changeset/104102>