Remove WebGLObject maps from WebGLRenderingContextBase and WebGLContextGroup.

Remove WebGLObject maps from WebGLRenderingContextBase and WebGLContextGroup.

There are two high-level goals for the handling of WebGLObjects:

1) When they are GC'd, the underlying graphics resource is deleted
promptly.

2) When the context is destroyed, all of the underlying graphics
resources are reclaimed.

The WebGLContextObject and WebGLSharedObject maps were an attempt to
implement (2) before WebGL contexts were in their own client-side
share group, separate from the compositor. Unfortunately, with the
introduction of Oilpan, it's become clear that there are longstanding
bugs in the lifetime management of WebGLObjects. These bugs recently
began manifesting frequently on the bots.

To fix these bugs, WebGLObjects are now eagerly finalized, rather than
the WebGLRenderingContextBase. WebGLObjects maintain a strong
reference either to their context or share group, depending on whether
they're per-context or shared objects. WebGLContextGroup becomes an
Oilpan object which maintains weak references to all of its contexts
(currently, just one). WebGLRenderingContextBase maintains a strong
reference to its context group.

Because WebGLObjects can no longer be enumerated, their invalidation
during context loss needs to be rewritten. This is handled by tracking
the number of context losses on a per-share-group basis, and comparing
this with a cached version in each WebGLObject instance during its
validation.

As a consequence of this change, most of the work in
WebGLRenderingContextBase's destructor, as well as in the destructors
of corollary types like WebGLExtension, becomes useless, and it has
been removed. The destructors of the WebGLObject subclasses have been
made more uniform to prevent errors.

More fixes around wrapper tracing are probably both needed and enabled
by this CL, but they can follow this necessary fix.

BUG=537054, 666061
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
TBR=zmo@chromium.org

Committed: https://crrev.com/f5e91b15573820ff14396b6be246c54ca6755a13
Cr-Commit-Position: refs/heads/master@{#436080}

[Ken Russell (switch to Gerrit), 2016-12-02 08:24:09]

Description was changed from

==========
Remove WebGLObject maps from WebGLRenderingContextBase and WebGLContextGroup.

There are two high-level goals for the handling of WebGLObjects:

1) When they are GC'd, the underlying graphics resource is deleted
promptly.

2) When the context is destroyed, all of the underlying graphics
resources are reclaimed.

The WebGLContextObject and WebGLSharedObject maps were an attempt to
implement (2) before WebGL contexts were in their own client-side
share group, separate from the compositor. Unfortunately, with the
introduction of Oilpan, it's become clear that there are longstanding
bugs in the lifetime management of WebGLObjects. These bugs recently
began manifesting frequently on the bots.

To fix these bugs, WebGLObjects are now eagerly finalized, rather than
the WebGLRenderingContextBase. WebGLObjects maintain a strong
reference either to their context or share group, depending on whether
they're per-context or shared objects. WebGLContextGroup becomes an
Oilpan object which maintains weak references to all of its contexts
(currently, just one). WebGLRenderingContextBase maintains a strong
reference to its context group.

Because WebGLObjects can no longer be enumerated, their invalidation
during context loss needs to be rewritten. This is handled by tracking
the number of context losses on a per-share-group basis, and comparing
this with a cached version in each WebGLObject instance during its
validation.

As a consequence of this change, most of the work in
WebGLRenderingContextBase's destructor, as well as in the destructors
of corollary types like WebGLExtension, becomes useless, and it has
been removed. The destructors of the WebGLObject subclasses have been
made more uniform to prevent errors.

More fixes around wrapper tracing are probably both needed and enabled
by this CL, but they can follow this necessary fix.

BUG=537054, 666061
==========

to

==========
Remove WebGLObject maps from WebGLRenderingContextBase and WebGLContextGroup.

There are two high-level goals for the handling of WebGLObjects:

1) When they are GC'd, the underlying graphics resource is deleted
promptly.

2) When the context is destroyed, all of the underlying graphics
resources are reclaimed.

The WebGLContextObject and WebGLSharedObject maps were an attempt to
implement (2) before WebGL contexts were in their own client-side
share group, separate from the compositor. Unfortunately, with the
introduction of Oilpan, it's become clear that there are longstanding
bugs in the lifetime management of WebGLObjects. These bugs recently
began manifesting frequently on the bots.

To fix these bugs, WebGLObjects are now eagerly finalized, rather than
the WebGLRenderingContextBase. WebGLObjects maintain a strong
reference either to their context or share group, depending on whether
they're per-context or shared objects. WebGLContextGroup becomes an
Oilpan object which maintains weak references to all of its contexts
(currently, just one). WebGLRenderingContextBase maintains a strong
reference to its context group.

Because WebGLObjects can no longer be enumerated, their invalidation
during context loss needs to be rewritten. This is handled by tracking
the number of context losses on a per-share-group basis, and comparing
this with a cached version in each WebGLObject instance during its
validation.

As a consequence of this change, most of the work in
WebGLRenderingContextBase's destructor, as well as in the destructors
of corollary types like WebGLExtension, becomes useless, and it has
been removed. The destructors of the WebGLObject subclasses have been
made more uniform to prevent errors.

More fixes around wrapper tracing are probably both needed and enabled
by this CL, but they can follow this necessary fix.

BUG=537054, 666061
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Ken Russell (switch to Gerrit), 2016-12-02 08:26:22]

Description was changed from

==========
Remove WebGLObject maps from WebGLRenderingContextBase and WebGLContextGroup.

There are two high-level goals for the handling of WebGLObjects:

1) When they are GC'd, the underlying graphics resource is deleted
promptly.

2) When the context is destroyed, all of the underlying graphics
resources are reclaimed.

The WebGLContextObject and WebGLSharedObject maps were an attempt to
implement (2) before WebGL contexts were in their own client-side
share group, separate from the compositor. Unfortunately, with the
introduction of Oilpan, it's become clear that there are longstanding
bugs in the lifetime management of WebGLObjects. These bugs recently
began manifesting frequently on the bots.

To fix these bugs, WebGLObjects are now eagerly finalized, rather than
the WebGLRenderingContextBase. WebGLObjects maintain a strong
reference either to their context or share group, depending on whether
they're per-context or shared objects. WebGLContextGroup becomes an
Oilpan object which maintains weak references to all of its contexts
(currently, just one). WebGLRenderingContextBase maintains a strong
reference to its context group.

Because WebGLObjects can no longer be enumerated, their invalidation
during context loss needs to be rewritten. This is handled by tracking
the number of context losses on a per-share-group basis, and comparing
this with a cached version in each WebGLObject instance during its
validation.

As a consequence of this change, most of the work in
WebGLRenderingContextBase's destructor, as well as in the destructors
of corollary types like WebGLExtension, becomes useless, and it has
been removed. The destructors of the WebGLObject subclasses have been
made more uniform to prevent errors.

More fixes around wrapper tracing are probably both needed and enabled
by this CL, but they can follow this necessary fix.

BUG=537054, 666061
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Remove WebGLObject maps from WebGLRenderingContextBase and WebGLContextGroup.

There are two high-level goals for the handling of WebGLObjects:

1) When they are GC'd, the underlying graphics resource is deleted
promptly.

2) When the context is destroyed, all of the underlying graphics
resources are reclaimed.

The WebGLContextObject and WebGLSharedObject maps were an attempt to
implement (2) before WebGL contexts were in their own client-side
share group, separate from the compositor. Unfortunately, with the
introduction of Oilpan, it's become clear that there are longstanding
bugs in the lifetime management of WebGLObjects. These bugs recently
began manifesting frequently on the bots.

To fix these bugs, WebGLObjects are now eagerly finalized, rather than
the WebGLRenderingContextBase. WebGLObjects maintain a strong
reference either to their context or share group, depending on whether
they're per-context or shared objects. WebGLContextGroup becomes an
Oilpan object which maintains weak references to all of its contexts
(currently, just one). WebGLRenderingContextBase maintains a strong
reference to its context group.

Because WebGLObjects can no longer be enumerated, their invalidation
during context loss needs to be rewritten. This is handled by tracking
the number of context losses on a per-share-group basis, and comparing
this with a cached version in each WebGLObject instance during its
validation.

As a consequence of this change, most of the work in
WebGLRenderingContextBase's destructor, as well as in the destructors
of corollary types like WebGLExtension, becomes useless, and it has
been removed. The destructors of the WebGLObject subclasses have been
made more uniform to prevent errors.

More fixes around wrapper tracing are probably both needed and enabled
by this CL, but they can follow this necessary fix.

BUG=537054, 666061
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
==========

[Ken Russell (switch to Gerrit), 2016-12-02 08:26:40]

The CQ bit was checked by kbr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 08:26:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Russell (switch to Gerrit), 2016-12-02 09:58:47]

kbr@chromium.org changed reviewers:
+ haraken@chromium.org, mlippautz@chromium.org, sigbjornf@opera.com,
zmo@chromium.org

[Ken Russell (switch to Gerrit), 2016-12-02 09:58:48]

I think the tryjobs will all come back green, so sending this for review.

zmo: please review. kainino: feel free. Let's discuss the whole CL tomorrow.

haraken or sigbjornf: one question for you in WebGLContextGroup.cpp please.

mlippautz: one question for you in WebGLContextGroup.cpp please.

Thanks.

https://codereview.chromium.org/2547813002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp (right):

https://codereview.chromium.org/2547813002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp:36: // removed
from the HeapHashSet?
haraken@ and sigbjornf@: could you answer this question?

https://codereview.chromium.org/2547813002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp:57: // TODO(kbr,
mlippautz): need to use the manual write barrier since we
mlippautz: am I correct in my thinking here?

[haraken, 2016-12-02 10:14:34]

https://codereview.chromium.org/2547813002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp (right):

https://codereview.chromium.org/2547813002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp:36: // removed
from the HeapHashSet?
On 2016/12/02 09:58:48, Ken Russell wrote:
> haraken@ and sigbjornf@: could you answer this question?

The weak processing removes the dead entry from the HashSet. So it's guaranteed
that this does not return null.

[sof, 2016-12-02 10:39:06]

I do hope this one works out & lands safely, swashbuckling simplifications.

https://codereview.chromium.org/2547813002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp (right):

https://codereview.chromium.org/2547813002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp:36: // removed
from the HeapHashSet?
On 2016/12/02 10:14:34, haraken wrote:
> On 2016/12/02 09:58:48, Ken Russell wrote:
> > haraken@ and sigbjornf@: could you answer this question?
> 
> The weak processing removes the dead entry from the HashSet. So it's
guaranteed
> that this does not return null.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/HashTable....
is where this happens as part of weak reference processing during GC.

[commit-bot: I haz the power, 2016-12-02 13:11:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 13:11:40]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Kai Ninomiya, 2016-12-02 18:51:28]

kainino@chromium.org changed reviewers:
+ kainino@chromium.org

[Kai Ninomiya, 2016-12-02 18:51:29]

Seems good, not sure what's going on with the failures. One question.

https://codereview.chromium.org/2547813002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLProgram.cpp (right):

https://codereview.chromium.org/2547813002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLProgram.cpp:56: if
(!destructionInProgress()) {
Does this depend on the assumption that there is only one context per share
group? In theory, if there were multiple contexts per share group, could this
cause a GL resource leak?

[Zhenyao Mo, 2016-12-02 18:58:40]

Mostly looks good with one question.

https://codereview.chromium.org/2547813002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLContextObject.h (right):

https://codereview.chromium.org/2547813002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLContextObject.h:43: return context
== m_context;
Do we also want to check the numberOfContextLosses here?

https://codereview.chromium.org/2547813002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLSharedObject.cpp (right):

https://codereview.chromium.org/2547813002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLSharedObject.cpp:42:
cachedNumberOfContextLosses() == contextGroup->numberOfContextLosses();
Can you add a comment here saying for WebGL, there is only one context per
context group here? Otherwise the logic is confusing here.

[Ken Russell (switch to Gerrit), 2016-12-02 20:38:49]

Thanks for your reviews.

The test failures are unrelated. The webkit_tests timeouts on Windows are
http://crbug.com/637478 . The content_browsertests failures are
http://crbug.com/670844 .

Submitting this to the CQ now to try to stem the large number of flaky WebGL
conformance test failures seen recently.

https://codereview.chromium.org/2547813002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp (right):

https://codereview.chromium.org/2547813002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp:36: // removed
from the HeapHashSet?
On 2016/12/02 10:39:06, sof wrote:
> On 2016/12/02 10:14:34, haraken wrote:
> > On 2016/12/02 09:58:48, Ken Russell wrote:
> > > haraken@ and sigbjornf@: could you answer this question?
> > 
> > The weak processing removes the dead entry from the HashSet. So it's
> guaranteed
> > that this does not return null.
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/HashTable....
> is where this happens as part of weak reference processing during GC.

Thanks for the feedback. I'll include this information in the comment.

https://codereview.chromium.org/2547813002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLProgram.cpp (right):

https://codereview.chromium.org/2547813002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLProgram.cpp:56: if
(!destructionInProgress()) {
On 2016/12/02 18:51:28, Kai Ninomiya wrote:
> Does this depend on the assumption that there is only one context per share
> group? In theory, if there were multiple contexts per share group, could this
> cause a GL resource leak?

No, no such assumption and no such leak. When the WebGLShaders themselves are
GC'd, they will call detachAndDelete in their destructor, which will delete
their shader objects.

https://codereview.chromium.org/2547813002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLSharedObject.cpp (right):

https://codereview.chromium.org/2547813002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLSharedObject.cpp:42:
cachedNumberOfContextLosses() == contextGroup->numberOfContextLosses();
On 2016/12/02 18:58:40, Zhenyao Mo wrote:
> Can you add a comment here saying for WebGL, there is only one context per
> context group here? Otherwise the logic is confusing here.

There's actually no such assumption in this code, and the logic should be
correct even if there was more than one context. The number of context losses is
a concept per share group. Contexts delegate the reporting of that number to
their share group. Objects record the number of context losses from their share
group at their creation time.

I think we should consider removing the support for shared objects, which was
never exposed to JS, since we're likely to produce a radically new low-level
WebGL which will eliminate the need for this concept. It would make the lifetime
semantics even simpler.

[Ken Russell (switch to Gerrit), 2016-12-02 20:38:59]

The CQ bit was checked by kbr@chromium.org

[commit-bot: I haz the power, 2016-12-02 20:39:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 20:39:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 20:39:59]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Ken Russell (switch to Gerrit), 2016-12-02 21:50:24]

I suspect Mo intended to give a thumbs-up on this so I'm TBR'ing it. It's the
cause of significant flakiness and we need time to make sure that this fixes it.

[Ken Russell (switch to Gerrit), 2016-12-02 21:50:35]

Description was changed from

==========
Remove WebGLObject maps from WebGLRenderingContextBase and WebGLContextGroup.

There are two high-level goals for the handling of WebGLObjects:

1) When they are GC'd, the underlying graphics resource is deleted
promptly.

2) When the context is destroyed, all of the underlying graphics
resources are reclaimed.

The WebGLContextObject and WebGLSharedObject maps were an attempt to
implement (2) before WebGL contexts were in their own client-side
share group, separate from the compositor. Unfortunately, with the
introduction of Oilpan, it's become clear that there are longstanding
bugs in the lifetime management of WebGLObjects. These bugs recently
began manifesting frequently on the bots.

To fix these bugs, WebGLObjects are now eagerly finalized, rather than
the WebGLRenderingContextBase. WebGLObjects maintain a strong
reference either to their context or share group, depending on whether
they're per-context or shared objects. WebGLContextGroup becomes an
Oilpan object which maintains weak references to all of its contexts
(currently, just one). WebGLRenderingContextBase maintains a strong
reference to its context group.

Because WebGLObjects can no longer be enumerated, their invalidation
during context loss needs to be rewritten. This is handled by tracking
the number of context losses on a per-share-group basis, and comparing
this with a cached version in each WebGLObject instance during its
validation.

As a consequence of this change, most of the work in
WebGLRenderingContextBase's destructor, as well as in the destructors
of corollary types like WebGLExtension, becomes useless, and it has
been removed. The destructors of the WebGLObject subclasses have been
made more uniform to prevent errors.

More fixes around wrapper tracing are probably both needed and enabled
by this CL, but they can follow this necessary fix.

BUG=537054, 666061
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
==========

to

==========
Remove WebGLObject maps from WebGLRenderingContextBase and WebGLContextGroup.

There are two high-level goals for the handling of WebGLObjects:

1) When they are GC'd, the underlying graphics resource is deleted
promptly.

2) When the context is destroyed, all of the underlying graphics
resources are reclaimed.

The WebGLContextObject and WebGLSharedObject maps were an attempt to
implement (2) before WebGL contexts were in their own client-side
share group, separate from the compositor. Unfortunately, with the
introduction of Oilpan, it's become clear that there are longstanding
bugs in the lifetime management of WebGLObjects. These bugs recently
began manifesting frequently on the bots.

To fix these bugs, WebGLObjects are now eagerly finalized, rather than
the WebGLRenderingContextBase. WebGLObjects maintain a strong
reference either to their context or share group, depending on whether
they're per-context or shared objects. WebGLContextGroup becomes an
Oilpan object which maintains weak references to all of its contexts
(currently, just one). WebGLRenderingContextBase maintains a strong
reference to its context group.

Because WebGLObjects can no longer be enumerated, their invalidation
during context loss needs to be rewritten. This is handled by tracking
the number of context losses on a per-share-group basis, and comparing
this with a cached version in each WebGLObject instance during its
validation.

As a consequence of this change, most of the work in
WebGLRenderingContextBase's destructor, as well as in the destructors
of corollary types like WebGLExtension, becomes useless, and it has
been removed. The destructors of the WebGLObject subclasses have been
made more uniform to prevent errors.

More fixes around wrapper tracing are probably both needed and enabled
by this CL, but they can follow this necessary fix.

BUG=537054, 666061
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
TBR=zmo@chromium.org
==========

[Ken Russell (switch to Gerrit), 2016-12-02 21:50:41]

The CQ bit was checked by kbr@chromium.org

[commit-bot: I haz the power, 2016-12-02 21:51:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Russell (switch to Gerrit), 2016-12-02 22:13:38]

https://codereview.chromium.org/2547813002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLContextObject.h (right):

https://codereview.chromium.org/2547813002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLContextObject.h:43: return context
== m_context;
On 2016/12/02 18:58:39, Zhenyao Mo wrote:
> Do we also want to check the numberOfContextLosses here?

Sorry, forgot to reply to your comment. Yes, thank you for catching this; that
was a bug. Fixed. (Surprising that the WEBGL_lose_context extension test didn't
catch this -- I'll look into improving it.)

[Zhenyao Mo, 2016-12-02 22:39:25]

On 2016/12/02 22:13:38, Ken Russell wrote:
>
https://codereview.chromium.org/2547813002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/webgl/WebGLContextObject.h (right):
> 
>
https://codereview.chromium.org/2547813002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/webgl/WebGLContextObject.h:43: return
context
> == m_context;
> On 2016/12/02 18:58:39, Zhenyao Mo wrote:
> > Do we also want to check the numberOfContextLosses here?
> 
> Sorry, forgot to reply to your comment. Yes, thank you for catching this; that
> was a bug. Fixed. (Surprising that the WEBGL_lose_context extension test
didn't
> catch this -- I'll look into improving it.)

LGTM

[commit-bot: I haz the power, 2016-12-02 23:46:06]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480715441638390,
"parent_rev": "3bf9e1e881d3c146c69aebd2614f0891732a5cc9", "commit_rev":
"81e974a9b0e8ddb7c419cba3fa18c87bbf9991ce"}

[commit-bot: I haz the power, 2016-12-02 23:46:43]

Description was changed from

==========
Remove WebGLObject maps from WebGLRenderingContextBase and WebGLContextGroup.

There are two high-level goals for the handling of WebGLObjects:

1) When they are GC'd, the underlying graphics resource is deleted
promptly.

2) When the context is destroyed, all of the underlying graphics
resources are reclaimed.

The WebGLContextObject and WebGLSharedObject maps were an attempt to
implement (2) before WebGL contexts were in their own client-side
share group, separate from the compositor. Unfortunately, with the
introduction of Oilpan, it's become clear that there are longstanding
bugs in the lifetime management of WebGLObjects. These bugs recently
began manifesting frequently on the bots.

To fix these bugs, WebGLObjects are now eagerly finalized, rather than
the WebGLRenderingContextBase. WebGLObjects maintain a strong
reference either to their context or share group, depending on whether
they're per-context or shared objects. WebGLContextGroup becomes an
Oilpan object which maintains weak references to all of its contexts
(currently, just one). WebGLRenderingContextBase maintains a strong
reference to its context group.

Because WebGLObjects can no longer be enumerated, their invalidation
during context loss needs to be rewritten. This is handled by tracking
the number of context losses on a per-share-group basis, and comparing
this with a cached version in each WebGLObject instance during its
validation.

As a consequence of this change, most of the work in
WebGLRenderingContextBase's destructor, as well as in the destructors
of corollary types like WebGLExtension, becomes useless, and it has
been removed. The destructors of the WebGLObject subclasses have been
made more uniform to prevent errors.

More fixes around wrapper tracing are probably both needed and enabled
by this CL, but they can follow this necessary fix.

BUG=537054, 666061
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
TBR=zmo@chromium.org
==========

to

==========
Remove WebGLObject maps from WebGLRenderingContextBase and WebGLContextGroup.

There are two high-level goals for the handling of WebGLObjects:

1) When they are GC'd, the underlying graphics resource is deleted
promptly.

2) When the context is destroyed, all of the underlying graphics
resources are reclaimed.

The WebGLContextObject and WebGLSharedObject maps were an attempt to
implement (2) before WebGL contexts were in their own client-side
share group, separate from the compositor. Unfortunately, with the
introduction of Oilpan, it's become clear that there are longstanding
bugs in the lifetime management of WebGLObjects. These bugs recently
began manifesting frequently on the bots.

To fix these bugs, WebGLObjects are now eagerly finalized, rather than
the WebGLRenderingContextBase. WebGLObjects maintain a strong
reference either to their context or share group, depending on whether
they're per-context or shared objects. WebGLContextGroup becomes an
Oilpan object which maintains weak references to all of its contexts
(currently, just one). WebGLRenderingContextBase maintains a strong
reference to its context group.

Because WebGLObjects can no longer be enumerated, their invalidation
during context loss needs to be rewritten. This is handled by tracking
the number of context losses on a per-share-group basis, and comparing
this with a cached version in each WebGLObject instance during its
validation.

As a consequence of this change, most of the work in
WebGLRenderingContextBase's destructor, as well as in the destructors
of corollary types like WebGLExtension, becomes useless, and it has
been removed. The destructors of the WebGLObject subclasses have been
made more uniform to prevent errors.

More fixes around wrapper tracing are probably both needed and enabled
by this CL, but they can follow this necessary fix.

BUG=537054, 666061
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
TBR=zmo@chromium.org
==========

[commit-bot: I haz the power, 2016-12-02 23:46:45]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-02 23:48:45]

Description was changed from

==========
Remove WebGLObject maps from WebGLRenderingContextBase and WebGLContextGroup.

There are two high-level goals for the handling of WebGLObjects:

1) When they are GC'd, the underlying graphics resource is deleted
promptly.

2) When the context is destroyed, all of the underlying graphics
resources are reclaimed.

The WebGLContextObject and WebGLSharedObject maps were an attempt to
implement (2) before WebGL contexts were in their own client-side
share group, separate from the compositor. Unfortunately, with the
introduction of Oilpan, it's become clear that there are longstanding
bugs in the lifetime management of WebGLObjects. These bugs recently
began manifesting frequently on the bots.

To fix these bugs, WebGLObjects are now eagerly finalized, rather than
the WebGLRenderingContextBase. WebGLObjects maintain a strong
reference either to their context or share group, depending on whether
they're per-context or shared objects. WebGLContextGroup becomes an
Oilpan object which maintains weak references to all of its contexts
(currently, just one). WebGLRenderingContextBase maintains a strong
reference to its context group.

Because WebGLObjects can no longer be enumerated, their invalidation
during context loss needs to be rewritten. This is handled by tracking
the number of context losses on a per-share-group basis, and comparing
this with a cached version in each WebGLObject instance during its
validation.

As a consequence of this change, most of the work in
WebGLRenderingContextBase's destructor, as well as in the destructors
of corollary types like WebGLExtension, becomes useless, and it has
been removed. The destructors of the WebGLObject subclasses have been
made more uniform to prevent errors.

More fixes around wrapper tracing are probably both needed and enabled
by this CL, but they can follow this necessary fix.

BUG=537054, 666061
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
TBR=zmo@chromium.org
==========

to

==========
Remove WebGLObject maps from WebGLRenderingContextBase and WebGLContextGroup.

There are two high-level goals for the handling of WebGLObjects:

1) When they are GC'd, the underlying graphics resource is deleted
promptly.

2) When the context is destroyed, all of the underlying graphics
resources are reclaimed.

The WebGLContextObject and WebGLSharedObject maps were an attempt to
implement (2) before WebGL contexts were in their own client-side
share group, separate from the compositor. Unfortunately, with the
introduction of Oilpan, it's become clear that there are longstanding
bugs in the lifetime management of WebGLObjects. These bugs recently
began manifesting frequently on the bots.

To fix these bugs, WebGLObjects are now eagerly finalized, rather than
the WebGLRenderingContextBase. WebGLObjects maintain a strong
reference either to their context or share group, depending on whether
they're per-context or shared objects. WebGLContextGroup becomes an
Oilpan object which maintains weak references to all of its contexts
(currently, just one). WebGLRenderingContextBase maintains a strong
reference to its context group.

Because WebGLObjects can no longer be enumerated, their invalidation
during context loss needs to be rewritten. This is handled by tracking
the number of context losses on a per-share-group basis, and comparing
this with a cached version in each WebGLObject instance during its
validation.

As a consequence of this change, most of the work in
WebGLRenderingContextBase's destructor, as well as in the destructors
of corollary types like WebGLExtension, becomes useless, and it has
been removed. The destructors of the WebGLObject subclasses have been
made more uniform to prevent errors.

More fixes around wrapper tracing are probably both needed and enabled
by this CL, but they can follow this necessary fix.

BUG=537054, 666061
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
TBR=zmo@chromium.org

Committed: https://crrev.com/f5e91b15573820ff14396b6be246c54ca6755a13
Cr-Commit-Position: refs/heads/master@{#436080}
==========

[commit-bot: I haz the power, 2016-12-02 23:48:46]

Patchset 2 (id:??) landed as
https://crrev.com/f5e91b15573820ff14396b6be246c54ca6755a13
Cr-Commit-Position: refs/heads/master@{#436080}

[haraken, 2016-12-05 05:15:59]

LGTM with comments.

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp (right):

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp:41:
m_contexts.add(context);

You need to add a write barrier here.

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp:47:
++m_numberOfContextLosses;

Shall we add CHECK(m_numberOfContextLosses > 0) to prevent some overflow attack?

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp:59: // no such
TraceWrapperMember (like TraceWrapperWeakMember) exists yet.

You need to add the write barrier to line 41.

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLObject.cpp (right):

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLObject.cpp:52: if
(currentNumberOfContextLosses() != m_cachedNumberOfContextLosses) {

Can we use validate()?

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLObject.h (right):

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLObject.h:94:
DECLARE_EAGER_FINALIZATION_OPERATOR_NEW();

Do you need this macro?

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLSharedObject.cpp (right):

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLSharedObject.cpp:49:
gpu::gles2::GLES2Interface* WebGLSharedObject::getAGLInterface() const {

Don't we need to have a call path that clears m_contextGroup when WebGL object
gets destructed?

[Ken Russell (switch to Gerrit), 2016-12-05 06:03:40]

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp (right):

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp:41:
m_contexts.add(context);
On 2016/12/05 05:15:59, haraken wrote:
> 
> You need to add a write barrier here.

What should it read?

ScriptWrappableVisitor::writeBarrier(this, context)?
ScriptWrappableVisitor::writeBarrier(this, m_contexts)?
ScriptWrappableVisitor::writeBarrier(m_contexts, context)?

From
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...
I don't understand what the rules are when using a collection class holding
Members (or WeakMembers, in this case).

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp:47:
++m_numberOfContextLosses;
On 2016/12/05 05:15:59, haraken wrote:
> 
> Shall we add CHECK(m_numberOfContextLosses > 0) to prevent some overflow
attack?

I don't think it's necessary. In the very unlikely case of overflow, it doesn't
invalidate the algorithm.

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp:59: // no such
TraceWrapperMember (like TraceWrapperWeakMember) exists yet.
On 2016/12/05 05:15:59, haraken wrote:
> 
> You need to add the write barrier to line 41.

Please see question above.

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLObject.cpp (right):

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLObject.cpp:52: if
(currentNumberOfContextLosses() != m_cachedNumberOfContextLosses) {
On 2016/12/05 05:15:59, haraken wrote:
> 
> Can we use validate()?

Sorry, no. We don't have a reference to our context or context group at this
point.

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLObject.h (right):

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLObject.h:94:
DECLARE_EAGER_FINALIZATION_OPERATOR_NEW();
On 2016/12/05 05:15:59, haraken wrote:
> 
> Do you need this macro?

Yes, we absolutely do. The entire point of this CL is to make WebGLObject
eagerly finalized before its context or context group, so that it has the
opportunity to use that context to delete itself, if the context isn't also
being destroyed. This is critical to ensure prompt reclamation of graphics
resources if the programmer loses references to potentially-expensive objects
like WebGLBuffer or WebGLTexture.

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLSharedObject.cpp (right):

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLSharedObject.cpp:49:
gpu::gles2::GLES2Interface* WebGLSharedObject::getAGLInterface() const {
On 2016/12/05 05:15:59, haraken wrote:
> 
> Don't we need to have a call path that clears m_contextGroup when WebGL object
> gets destructed?

Actually, looking at this code again, m_contextGroup is guaranteed to be
non-null. WebGLSharedObject maintains a strong reference to it. We don't need to
zero it out in WebGLSharedObject's destructor.

As we discussed in https://codereview.chromium.org/2548153002/ , the only thing
that might happen is that all of m_contextGroup's weak references might be
cleared.

Should I change this code to:

  return m_contextGroup->getAGLInterface();

?

[haraken, 2016-12-05 06:08:40]

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp (right):

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp:41:
m_contexts.add(context);
On 2016/12/05 06:03:40, Ken Russell wrote:
> On 2016/12/05 05:15:59, haraken wrote:
> > 
> > You need to add a write barrier here.
> 
> What should it read?
> 
> ScriptWrappableVisitor::writeBarrier(this, context)?
> ScriptWrappableVisitor::writeBarrier(this, m_contexts)?
> ScriptWrappableVisitor::writeBarrier(m_contexts, context)?
> 
> From
>
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...
> I don't understand what the rules are when using a collection class holding
> Members (or WeakMembers, in this case).

ScriptWrappableVisitor::writeBarrier(this, context)

Write barrier should be added from an object that has traceWrappers() to an
object that has traceWrappers().

Micheal: Would you update the document?

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLObject.h (right):

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLObject.h:94:
DECLARE_EAGER_FINALIZATION_OPERATOR_NEW();
On 2016/12/05 06:03:40, Ken Russell wrote:
> On 2016/12/05 05:15:59, haraken wrote:
> > 
> > Do you need this macro?
> 
> Yes, we absolutely do. The entire point of this CL is to make WebGLObject
> eagerly finalized before its context or context group, so that it has the
> opportunity to use that context to delete itself, if the context isn't also
> being destroyed. This is critical to ensure prompt reclamation of graphics
> resources if the programmer loses references to potentially-expensive objects
> like WebGLBuffer or WebGLTexture.

EAGERLY_FINALIZE() is needed but DECLARE_EAGER_FINALIZATION_OPERATOR_NEW() won't
be needed.

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLSharedObject.cpp (right):

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLSharedObject.cpp:49:
gpu::gles2::GLES2Interface* WebGLSharedObject::getAGLInterface() const {
On 2016/12/05 06:03:40, Ken Russell wrote:
> On 2016/12/05 05:15:59, haraken wrote:
> > 
> > Don't we need to have a call path that clears m_contextGroup when WebGL
object
> > gets destructed?
> 
> Actually, looking at this code again, m_contextGroup is guaranteed to be
> non-null. WebGLSharedObject maintains a strong reference to it. We don't need
to
> zero it out in WebGLSharedObject's destructor.
> 
> As we discussed in https://codereview.chromium.org/2548153002/ , the only
thing
> that might happen is that all of m_contextGroup's weak references might be
> cleared.
> 
> Should I change this code to:
> 
>   return m_contextGroup->getAGLInterface();
> 
> ?

Sounds nicer.

[Ken Russell (switch to Gerrit), 2016-12-05 06:36:20]

Addressing review feedback in this follow-on CL:
https://codereview.chromium.org/2548273002/

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp (right):

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp:41:
m_contexts.add(context);
On 2016/12/05 06:08:39, haraken wrote:
> On 2016/12/05 06:03:40, Ken Russell wrote:
> > On 2016/12/05 05:15:59, haraken wrote:
> > > 
> > > You need to add a write barrier here.
> > 
> > What should it read?
> > 
> > ScriptWrappableVisitor::writeBarrier(this, context)?
> > ScriptWrappableVisitor::writeBarrier(this, m_contexts)?
> > ScriptWrappableVisitor::writeBarrier(m_contexts, context)?
> > 
> > From
> >
>
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...
> > I don't understand what the rules are when using a collection class holding
> > Members (or WeakMembers, in this case).
> 
> ScriptWrappableVisitor::writeBarrier(this, context)
> 
> Write barrier should be added from an object that has traceWrappers() to an
> object that has traceWrappers().

Thanks, will add.

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLObject.h (right):

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLObject.h:94:
DECLARE_EAGER_FINALIZATION_OPERATOR_NEW();
On 2016/12/05 06:08:40, haraken wrote:
> On 2016/12/05 06:03:40, Ken Russell wrote:
> > On 2016/12/05 05:15:59, haraken wrote:
> > > 
> > > Do you need this macro?
> > 
> > Yes, we absolutely do. The entire point of this CL is to make WebGLObject
> > eagerly finalized before its context or context group, so that it has the
> > opportunity to use that context to delete itself, if the context isn't also
> > being destroyed. This is critical to ensure prompt reclamation of graphics
> > resources if the programmer loses references to potentially-expensive
objects
> > like WebGLBuffer or WebGLTexture.
> 
> EAGERLY_FINALIZE() is needed but DECLARE_EAGER_FINALIZATION_OPERATOR_NEW()
won't
> be needed.

Thanks for the clarification. Will fix. (Is there any documentation for this?
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...
is empty.)

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLSharedObject.cpp (right):

https://codereview.chromium.org/2547813002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLSharedObject.cpp:49:
gpu::gles2::GLES2Interface* WebGLSharedObject::getAGLInterface() const {
On 2016/12/05 06:08:40, haraken wrote:
> On 2016/12/05 06:03:40, Ken Russell wrote:
> > On 2016/12/05 05:15:59, haraken wrote:
> > > 
> > > Don't we need to have a call path that clears m_contextGroup when WebGL
> object
> > > gets destructed?
> > 
> > Actually, looking at this code again, m_contextGroup is guaranteed to be
> > non-null. WebGLSharedObject maintains a strong reference to it. We don't
need
> to
> > zero it out in WebGLSharedObject's destructor.
> > 
> > As we discussed in https://codereview.chromium.org/2548153002/ , the only
> thing
> > that might happen is that all of m_contextGroup's weak references might be
> > cleared.
> > 
> > Should I change this code to:
> > 
> >   return m_contextGroup->getAGLInterface();
> > 
> > ?
> 
> Sounds nicer.

Will fix.