Make WebGL extensions get lost when context is lost.

Make WebGL extensions get lost when context is lost.

BUG=244750
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152264

[greggman, 2013-05-30 06:36:22]

Hey Ken, PTAL

It's not nearly as bad as it looks ;-)

[greggman, 2013-06-04 21:24:30]

On 2013/05/30 06:36:22, greggman wrote:
> Hey Ken, PTAL
> 
> It's not nearly as bad as it looks ;-)

ping

[Ken Russell (switch to Gerrit), 2013-06-05 00:33:08]

Sorry, missed this. Reviewing now...

[Ken Russell (switch to Gerrit), 2013-06-05 00:46:37]

LGTM overall; couple of questions. See WebGLRenderingContext.h comment first
before looking at individual extensions' comments.

https://codereview.chromium.org/15876011/diff/1/Source/core/html/canvas/OESSt...
File Source/core/html/canvas/OESStandardDerivatives.cpp (right):

https://codereview.chromium.org/15876011/diff/1/Source/core/html/canvas/OESSt...
Source/core/html/canvas/OESStandardDerivatives.cpp:36:
ScriptWrappable::init(this);
Missing call to enable()?

https://codereview.chromium.org/15876011/diff/1/Source/core/html/canvas/OESTe...
File Source/core/html/canvas/OESTextureHalfFloat.cpp (right):

https://codereview.chromium.org/15876011/diff/1/Source/core/html/canvas/OESTe...
Source/core/html/canvas/OESTextureHalfFloat.cpp:36: ScriptWrappable::init(this);
Missing call to enable()?

https://codereview.chromium.org/15876011/diff/1/Source/core/html/canvas/OESVe...
File Source/core/html/canvas/OESVertexArrayObject.cpp (right):

https://codereview.chromium.org/15876011/diff/1/Source/core/html/canvas/OESVe...
Source/core/html/canvas/OESVertexArrayObject.cpp:39:
ScriptWrappable::init(this);
Missing call to enable()?

https://codereview.chromium.org/15876011/diff/1/Source/core/html/canvas/WebGL...
File Source/core/html/canvas/WebGLExtension.h (right):

https://codereview.chromium.org/15876011/diff/1/Source/core/html/canvas/WebGL...
Source/core/html/canvas/WebGLExtension.h:64: void restore()
Is a restore() method needed? As I understand it, when the context is lost (even
via WEBGL_lose_context), all of the extension objects (except for
WEBGL_lose_context) are permanently lost, and further, disassociated from the
WebGLRenderingContext -- so if the context is restored, and the extension is
fetched again, a different JavaScript object will be returned. If this is
correct then restore() can be removed.

https://codereview.chromium.org/15876011/diff/1/Source/core/html/canvas/WebGL...
Source/core/html/canvas/WebGLExtension.h:84: bool m_lost;
Should m_lost be private instead?

https://codereview.chromium.org/15876011/diff/1/Source/core/html/canvas/WebGL...
File Source/core/html/canvas/WebGLRenderingContext.h (right):

https://codereview.chromium.org/15876011/diff/1/Source/core/html/canvas/WebGL...
Source/core/html/canvas/WebGLRenderingContext.h:570: m_extension =
T::create(context);
Should this also call m_extension->enable()? That would avoid calling enable()
in the constructors of all of the extensions. Calling virtual functions from a
constructor is dangerous in general.

[greggman, 2013-06-05 01:10:31]

On 2013/06/05 00:46:37, kbr wrote:
> LGTM overall; couple of questions. See WebGLRenderingContext.h comment first
> before looking at individual extensions' comments.
> 
>
https://codereview.chromium.org/15876011/diff/1/Source/core/html/canvas/OESSt...
> File Source/core/html/canvas/OESStandardDerivatives.cpp (right):
> 
>
https://codereview.chromium.org/15876011/diff/1/Source/core/html/canvas/OESSt...
> Source/core/html/canvas/OESStandardDerivatives.cpp:36:
> ScriptWrappable::init(this);
> Missing call to enable()?
> 
>
https://codereview.chromium.org/15876011/diff/1/Source/core/html/canvas/OESTe...
> File Source/core/html/canvas/OESTextureHalfFloat.cpp (right):
> 
>
https://codereview.chromium.org/15876011/diff/1/Source/core/html/canvas/OESTe...
> Source/core/html/canvas/OESTextureHalfFloat.cpp:36:
ScriptWrappable::init(this);
> Missing call to enable()?
> 
>
https://codereview.chromium.org/15876011/diff/1/Source/core/html/canvas/OESVe...
> File Source/core/html/canvas/OESVertexArrayObject.cpp (right):
> 
>
https://codereview.chromium.org/15876011/diff/1/Source/core/html/canvas/OESVe...
> Source/core/html/canvas/OESVertexArrayObject.cpp:39:
> ScriptWrappable::init(this);
> Missing call to enable()?
> 
>
https://codereview.chromium.org/15876011/diff/1/Source/core/html/canvas/WebGL...
> File Source/core/html/canvas/WebGLExtension.h (right):
> 
>
https://codereview.chromium.org/15876011/diff/1/Source/core/html/canvas/WebGL...
> Source/core/html/canvas/WebGLExtension.h:64: void restore()
> Is a restore() method needed? As I understand it, when the context is lost
(even
> via WEBGL_lose_context), all of the extension objects (except for
> WEBGL_lose_context) are permanently lost, and further, disassociated from the
> WebGLRenderingContext -- so if the context is restored, and the extension is
> fetched again, a different JavaScript object will be returned. If this is
> correct then restore() can be removed.

I don't think that's what the spec currently says does it?

First off I don't think there is any special wording for WEBGL_lose_context not
getting lost but clearly it wouldn't work otherwise. The specs probably need to
be updated to make that clear.

The spec says when you ask for an extension you get the same extension object.
The getExtension spec doesn't mention context lost. In fact the current spec
5.14 makes it sound like extensions just magically start working again after
getting context restored which is clearly wrong since it might not be the same
GPU.

Getting different objects is fine with me. That means the old objects have to be
"lost" forever. If I call ext.drawBuffersEXT(...) on a lost extension it is
basically a no-op forever. I'm fine with that as well but the spec should be
updated to make that clear that the extensions from getExtension are not the
same object after the context is restored. That seems like a cleaner direction
than returning the same objects after restore since some objects might not even
be available. It also seems better in that certain objects might look up limits
at initialization time and creating new ones is cleaner than re-initializing old
ones.

On the other hand the same could have been said of WebGLRenderingContext. After
context lost you should just call getContext to get a new object. 

I'll hold off on this CL until all this is figured out.


> 
>
https://codereview.chromium.org/15876011/diff/1/Source/core/html/canvas/WebGL...
> Source/core/html/canvas/WebGLExtension.h:84: bool m_lost;
> Should m_lost be private instead?
> 
>
https://codereview.chromium.org/15876011/diff/1/Source/core/html/canvas/WebGL...
> File Source/core/html/canvas/WebGLRenderingContext.h (right):
> 
>
https://codereview.chromium.org/15876011/diff/1/Source/core/html/canvas/WebGL...
> Source/core/html/canvas/WebGLRenderingContext.h:570: m_extension =
> T::create(context);
> Should this also call m_extension->enable()? That would avoid calling enable()
> in the constructors of all of the extensions. Calling virtual functions from a
> constructor is dangerous in general.

[greggman, 2013-06-11 01:06:53]

Hey Ken, PTAL

I updated this to what I think we talked about. On context lost all extensions
are lost and disassociated from the WebGLRenderingContext. They are all
RefCounted now. The WebGLRenderingContext keeps a ref. Does v8? Since the
WebGLRenderingContext keeps a ref V8 will GC that first which will free all the
references to the extensions which V8 can then GC as well.

-g

[Ken Russell (switch to Gerrit), 2013-06-11 21:50:54]

This mostly looks great; one issue that needs to be solved.

https://codereview.chromium.org/15876011/diff/9001/Source/bindings/v8/custom/...
File Source/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp (right):

https://codereview.chromium.org/15876011/diff/9001/Source/bindings/v8/custom/...
Source/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:355:
v8SetReturnValue(args, toV8Object(extension.get(), args.Holder(),
args.GetIsolate()));
This could possibly use extension.release() since the extension RefPtr isn't
used afterward.

https://codereview.chromium.org/15876011/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLLoseContext.cpp (right):

https://codereview.chromium.org/15876011/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLLoseContext.cpp:46: // WebGLLostContext is special
in that it is never lost.
We need to be very careful here. If the WebGLRenderingContext is actually GC'd
but we are still holding on to the WebGLLoseContext object then m_context can
become garbage. We must solve this somehow -- maybe with a boolean argument to
lose() indicating whether the context is "really" being lost.

[greggman, 2013-06-11 21:55:20]

https://codereview.chromium.org/15876011/diff/9001/Source/bindings/v8/custom/...
File Source/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp (right):

https://codereview.chromium.org/15876011/diff/9001/Source/bindings/v8/custom/...
Source/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:355:
v8SetReturnValue(args, toV8Object(extension.get(), args.Holder(),
args.GetIsolate()));
On 2013/06/11 21:50:54, kbr wrote:
> This could possibly use extension.release() since the extension RefPtr isn't
> used afterward.

I copied this from getAttachedShadersMethodCustom. Is that wrong too? Or is
there some difference between the 2 cases?

https://codereview.chromium.org/15876011/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLLoseContext.cpp (right):

https://codereview.chromium.org/15876011/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLLoseContext.cpp:46: // WebGLLostContext is special
in that it is never lost.
On 2013/06/11 21:50:54, kbr wrote:
> We need to be very careful here. If the WebGLRenderingContext is actually GC'd
> but we are still holding on to the WebGLLoseContext object then m_context can
> become garbage. We must solve this somehow -- maybe with a boolean argument to
> lose() indicating whether the context is "really" being lost.

This is already handled. ~WebGLRenderingContext loses all the extensions AND
removes all references.

[Ken Russell (switch to Gerrit), 2013-06-11 22:07:31]

https://codereview.chromium.org/15876011/diff/9001/Source/bindings/v8/custom/...
File Source/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp (right):

https://codereview.chromium.org/15876011/diff/9001/Source/bindings/v8/custom/...
Source/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:355:
v8SetReturnValue(args, toV8Object(extension.get(), args.Holder(),
args.GetIsolate()));
On 2013/06/11 21:55:21, greggman wrote:
> On 2013/06/11 21:50:54, kbr wrote:
> > This could possibly use extension.release() since the extension RefPtr isn't
> > used afterward.
> 
> I copied this from getAttachedShadersMethodCustom. Is that wrong too? Or is
> there some difference between the 2 cases?

getAttachedShadersMethodCustom could be changed too, but it isn't important.
This code is correct as is.

https://codereview.chromium.org/15876011/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLLoseContext.cpp (right):

https://codereview.chromium.org/15876011/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLLoseContext.cpp:46: // WebGLLostContext is special
in that it is never lost.
On 2013/06/11 21:55:21, greggman wrote:
> On 2013/06/11 21:50:54, kbr wrote:
> > We need to be very careful here. If the WebGLRenderingContext is actually
GC'd
> > but we are still holding on to the WebGLLoseContext object then m_context
can
> > become garbage. We must solve this somehow -- maybe with a boolean argument
to
> > lose() indicating whether the context is "really" being lost.
> 
> This is already handled. ~WebGLRenderingContext loses all the extensions AND
> removes all references.

I think there is still a problem:

1) JS code gets a WebGLRenderingContext and calls
getExtension("WEBGL_lose_context")
2) JS code releases its reference to the WebGLRenderingContext, canvas, etc. --
but not the WebGLLoseContext extension object -- and causes a GC
3) The WebGLRenderingContext is collected, but the WebGLLoseContext object is
not, because there is still a JS wrapper for it
4) JS calls either loseContext() or restoreContext() against the
WebGLLoseContext extension object, which causes m_context to be dereferenced

Even though at step (3) the WebGLRenderingContext destructor is called, all that
does is decrement the reference count of each extension object by one. They
might still be kept alive by JavaScript.

One way to solve this would be to add a non-virtual clearWeakContextReference()
to WebGLExtension, have WebGLExtension::lose() call that by default, and have
WebGLLoseContext override that behavior. ExtensionTracker's destructor could
call clearWeakContextReference(). WebGLLoseContext will need some guards around
the dereferencing of m_context.

[greggman, 2013-06-11 22:47:06]

PTAL

https://codereview.chromium.org/15876011/diff/9001/Source/core/html/canvas/We...
File Source/core/html/canvas/WebGLLoseContext.cpp (right):

https://codereview.chromium.org/15876011/diff/9001/Source/core/html/canvas/We...
Source/core/html/canvas/WebGLLoseContext.cpp:46: // WebGLLostContext is special
in that it is never lost.
Fixed

made lose() take a bool for "force". on context lost false is passed in, on
WebGLRenderingContext destruction true is passed in.

[Ken Russell (switch to Gerrit), 2013-06-11 23:08:33]

Excellent. LGTM

[commit-bot: I haz the power, 2013-06-11 23:11:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gman@chromium.org/15876011/16002

[commit-bot: I haz the power, 2013-06-12 01:10:02]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[Ken Russell (switch to Gerrit), 2013-06-12 01:16:09]

abarth, haraken, yurys: OWNERS review of Source/bindings/ please.

[haraken, 2013-06-12 01:19:32]

LGTM for bindings/

[commit-bot: I haz the power, 2013-06-12 01:22:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gman@chromium.org/15876011/16002

[commit-bot: I haz the power, 2013-06-12 06:38:00]

Change committed as 152264