Fix expando-loss-2.html test

Fix expando-loss-2.html test

Use V8HiddenValue to link the JS wrappers of transform
feedback/sampler/query objects, so that any expando properties added to
these wrappers aren't lost.

BUG=295792
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/61a1c3acf8dd8455bbd5c939c76582de9572445b
Cr-Commit-Position: refs/heads/master@{#395804}

[qiankun, 2016-05-23 06:19:04]

Description was changed from

==========
Fix expando-loss-2.html test

Use V8HiddenValue to link the JS wrappers of transform
feedback/sampler/query objects, so that any expando properties added to
these wrappers aren't lost.

BUG=295792
==========

to

==========
Fix expando-loss-2.html test

Use V8HiddenValue to link the JS wrappers of transform
feedback/sampler/query objects, so that any expando properties added to
these wrappers aren't lost.

BUG=295792
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[qiankun, 2016-05-23 06:19:05]

Description was changed from

==========
Fix expando-loss-2.html test

Use V8HiddenValue to link the JS wrappers of transform
feedback/sampler/query objects, so that any expando properties added to
these wrappers aren't lost.

BUG=295792
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Fix expando-loss-2.html test

Use V8HiddenValue to link the JS wrappers of transform
feedback/sampler/query objects, so that any expando properties added to
these wrappers aren't lost.

BUG=295792
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[qiankun, 2016-05-23 07:46:32]

qiankun.miao@intel.com changed reviewers:
+ bajones@chromium.org, kbr@chromium.org, yunchao.he@intel.com, zmo@chromium.org

[qiankun, 2016-05-23 07:46:32]

PTAL. This CL fixes expando-loss-2.html.

[Ken Russell (switch to Gerrit), 2016-05-23 21:30:41]

Thanks Qiankun. A few changes are needed.

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

https://codereview.chromium.org/2006583002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:1767:
preserveObjectWrapper(scriptState, this,
V8HiddenValue::webglMisc(scriptState->isolate()), &m_miscWrappers,
static_cast<uint32_t>(PreservedQuery), query);
This isn't correct. It's legal to have two active queries at the same time, but
the fact that this uses the "PreservedQuery" slot in the m_miscWrappers array
means that one will overwrite the other. For correctness, a new cache like
"m_queryWrappers" needs to be added, and it should be indexed by "target".

https://codereview.chromium.org/2006583002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:1927:
preserveObjectWrapper(scriptState, this,
V8HiddenValue::webglMisc(scriptState->isolate()), &m_miscWrappers, unit,
sampler);
The m_miscWrappers cache must only be indexed by the enums in
PreservedWrapperIndex to keep the code maintainable. You should add a new
m_samplerWrappers cache and index it similarly to the other texture-related
ones.

[qiankun, 2016-05-24 03:04:49]

Thanks ken. I updated this CL.

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

https://codereview.chromium.org/2006583002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:1767:
preserveObjectWrapper(scriptState, this,
V8HiddenValue::webglMisc(scriptState->isolate()), &m_miscWrappers,
static_cast<uint32_t>(PreservedQuery), query);
On 2016/05/23 21:30:41, Ken Russell wrote:
> This isn't correct. It's legal to have two active queries at the same time,
but
> the fact that this uses the "PreservedQuery" slot in the m_miscWrappers array
> means that one will overwrite the other. For correctness, a new cache like
> "m_queryWrappers" needs to be added, and it should be indexed by "target".

Done.

https://codereview.chromium.org/2006583002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:1927:
preserveObjectWrapper(scriptState, this,
V8HiddenValue::webglMisc(scriptState->isolate()), &m_miscWrappers, unit,
sampler);
On 2016/05/23 21:30:41, Ken Russell wrote:
> The m_miscWrappers cache must only be indexed by the enums in
> PreservedWrapperIndex to keep the code maintainable. You should add a new
> m_samplerWrappers cache and index it similarly to the other texture-related
> ones.

Done.

[Ken Russell (switch to Gerrit), 2016-05-24 23:15:10]

Great. Thanks Qiankun for fixing this. lgtm

Please note though that the earlier, incorrect, version of this fix should have
caused -- or at least allowed -- test failures. Please help dig into the
expando-loss-2 test and try to expand it to catch some of the earlier bugs.
Thanks.

[qiankun, 2016-05-25 01:57:20]

qiankun.miao@intel.com changed reviewers:
+ haraken@chromium.org

[qiankun, 2016-05-25 01:57:20]

haraken: could you do owner review for Source/bindings/?

[qiankun, 2016-05-25 01:59:07]

On 2016/05/24 23:15:10, Ken Russell wrote:
> Great. Thanks Qiankun for fixing this. lgtm
> 
> Please note though that the earlier, incorrect, version of this fix should
have
> caused -- or at least allowed -- test failures. Please help dig into the
> expando-loss-2 test and try to expand it to catch some of the earlier bugs.
> Thanks.

Thanks for pointing this. I will enhance the expando-loss-2 test to catch the
earlier bugs.

[haraken, 2016-05-25 02:19:23]

LGTM

[qiankun, 2016-05-25 02:48:25]

The CQ bit was checked by qiankun.miao@intel.com

[commit-bot: I haz the power, 2016-05-25 02:49:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2006583002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2006583002/40001

[commit-bot: I haz the power, 2016-05-25 05:08:00]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-25 05:09:19]

Description was changed from

==========
Fix expando-loss-2.html test

Use V8HiddenValue to link the JS wrappers of transform
feedback/sampler/query objects, so that any expando properties added to
these wrappers aren't lost.

BUG=295792
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Fix expando-loss-2.html test

Use V8HiddenValue to link the JS wrappers of transform
feedback/sampler/query objects, so that any expando properties added to
these wrappers aren't lost.

BUG=295792
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/61a1c3acf8dd8455bbd5c939c76582de9572445b
Cr-Commit-Position: refs/heads/master@{#395804}
==========

[commit-bot: I haz the power, 2016-05-25 05:09:20]

Patchset 3 (id:??) landed as
https://crrev.com/61a1c3acf8dd8455bbd5c939c76582de9572445b
Cr-Commit-Position: refs/heads/master@{#395804}