Prevent Null input in texSubImage2D(3D)

Prevent Null input in texSubImage2D(3D)

In a previous CL which packs some repeated code into helper functions,
I made a mistake so that tex(Sub)Image2D(3D) can always take null input
for DOMArrayBufferView. However, I took a look at the original code
before my change, it turns out that texImage2D and texImage3D is allowed
to take null input, but texSubImage2D and texSubImage3D is not allowed.
The stack trace in the bug also suggests that it crashes
because the input is null.

This CL makes change for that.

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

Committed: https://crrev.com/422466ba6888ba92906331f2adfe56123d722415
Cr-Commit-Position: refs/heads/master@{#398891}

[xidachen, 2016-06-08 20:48:31]

Description was changed from

==========
Prevent Null input in texSubImage2D(3D)

In a previous CL which packs some repeated code into helper functions,
I made a mistake so that tex(Sub)Image2D(3D) can always take null input
for DOMArrayBufferView. However, I took a look at the original code
before my change, it turns out that texImage2D and texImage3D is allowed
to take null input, but texSubImage2D and texSubImage3D is not allowed.

This CL makes change for that.

BUG=618299
==========

to

==========
Prevent Null input in texSubImage2D(3D)

In a previous CL which packs some repeated code into helper functions,
I made a mistake so that tex(Sub)Image2D(3D) can always take null input
for DOMArrayBufferView. However, I took a look at the original code
before my change, it turns out that texImage2D and texImage3D is allowed
to take null input, but texSubImage2D and texSubImage3D is not allowed.

This CL makes change for that.

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

[xidachen, 2016-06-08 20:49:34]

Description was changed from

==========
Prevent Null input in texSubImage2D(3D)

In a previous CL which packs some repeated code into helper functions,
I made a mistake so that tex(Sub)Image2D(3D) can always take null input
for DOMArrayBufferView. However, I took a look at the original code
before my change, it turns out that texImage2D and texImage3D is allowed
to take null input, but texSubImage2D and texSubImage3D is not allowed.

This CL makes change for that.

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

to

==========
Prevent Null input in texSubImage2D(3D)

In a previous CL which packs some repeated code into helper functions,
I made a mistake so that tex(Sub)Image2D(3D) can always take null input
for DOMArrayBufferView. However, I took a look at the original code
before my change, it turns out that texImage2D and texImage3D is allowed
to take null input, but texSubImage2D and texSubImage3D is not allowed. The
stack trace in the bug also suggests that it crashes
because the input is null.

This CL makes change for that.

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

[xidachen, 2016-06-08 20:49:48]

Description was changed from

==========
Prevent Null input in texSubImage2D(3D)

In a previous CL which packs some repeated code into helper functions,
I made a mistake so that tex(Sub)Image2D(3D) can always take null input
for DOMArrayBufferView. However, I took a look at the original code
before my change, it turns out that texImage2D and texImage3D is allowed
to take null input, but texSubImage2D and texSubImage3D is not allowed. The
stack trace in the bug also suggests that it crashes
because the input is null.

This CL makes change for that.

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

to

==========
Prevent Null input in texSubImage2D(3D)

In a previous CL which packs some repeated code into helper functions,
I made a mistake so that tex(Sub)Image2D(3D) can always take null input
for DOMArrayBufferView. However, I took a look at the original code
before my change, it turns out that texImage2D and texImage3D is allowed
to take null input, but texSubImage2D and texSubImage3D is not allowed.
The stack trace in the bug also suggests that it crashes
because the input is null.

This CL makes change for that.

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

[xidachen, 2016-06-08 20:50:11]

xidachen@chromium.org changed reviewers:
+ bajones@chromium.org, kbr@chromium.org, zmo@chromium.org

[xidachen, 2016-06-08 20:50:11]

PTAL

[Ken Russell (switch to Gerrit), 2016-06-08 21:13:07]

LGTM with comment -- but *please* look into the conformance tests and figure out
how your original CL made it through the commit queue. Some conformance test
should have caught the original bug. Please put up a pull request adding a test
if one is missing. Thanks.

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

https://codereview.chromium.org/2046223003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4127: ||
(functionType == TexSubImage && !validateTexFuncData(funcName, sourceType,
level, width, height, depth, format, type, pixels, NullNotAllowed)))
It would be better to add a "switch" statement here with TexImage and
TexSubImage as the only two arms, so that if someone accidentally adds another
enum value to TexImageFunctionType, a compile failure is triggered and we don't
accidentally fall through here.

[xidachen, 2016-06-09 01:04:13]

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

[commit-bot: I haz the power, 2016-06-09 01:04:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2046223003/20001

[xidachen, 2016-06-09 01:12:08]

I am surprised that the null input case is not caught in my previous CL.

I tried to search in our code base, and the closest I can get is:
conformance/more/functions/texSubImage2DBadArgs.html, that test covers many
things except the null input case. After this CL, I will have a pull request to
add the null input in that test.

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

https://codereview.chromium.org/2046223003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4127: ||
(functionType == TexSubImage && !validateTexFuncData(funcName, sourceType,
level, width, height, depth, format, type, pixels, NullNotAllowed)))
On 2016/06/08 21:13:07, Ken Russell wrote:
> It would be better to add a "switch" statement here with TexImage and
> TexSubImage as the only two arms, so that if someone accidentally adds another
> enum value to TexImageFunctionType, a compile failure is triggered and we
don't
> accidentally fall through here.

Done.

[commit-bot: I haz the power, 2016-06-09 01:13:07]

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

[commit-bot: I haz the power, 2016-06-09 01:13:08]

Dry run: Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Ken Russell (switch to Gerrit), 2016-06-09 05:31:24]

The new code LGTM once it compiles and the comment below addressed.

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

https://codereview.chromium.org/2046223003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4132:
return;
As you've seen the bots failed to compile this because there are enums which
aren't these two. Please figure out how they should be handled without adding a
default: label -- perhaps using NOTREACHED().

[xidachen, 2016-06-09 12:20:34]

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

[commit-bot: I haz the power, 2016-06-09 12:20:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2046223003/40001

[commit-bot: I haz the power, 2016-06-09 13:24:00]

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

[commit-bot: I haz the power, 2016-06-09 13:24:01]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[xidachen, 2016-06-09 13:40:11]

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

[commit-bot: I haz the power, 2016-06-09 13:40:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2046223003/60001

[xidachen, 2016-06-09 15:11:53]

The CQ bit was unchecked by xidachen@chromium.org

[xidachen, 2016-06-09 15:26:56]

The CQ bit was checked by xidachen@chromium.org

[xidachen, 2016-06-09 15:26:56]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/2046223003/#ps60001
(title: "forgot a break line...")

[commit-bot: I haz the power, 2016-06-09 15:27:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2046223003/60001

[commit-bot: I haz the power, 2016-06-09 15:32:41]

Description was changed from

==========
Prevent Null input in texSubImage2D(3D)

In a previous CL which packs some repeated code into helper functions,
I made a mistake so that tex(Sub)Image2D(3D) can always take null input
for DOMArrayBufferView. However, I took a look at the original code
before my change, it turns out that texImage2D and texImage3D is allowed
to take null input, but texSubImage2D and texSubImage3D is not allowed.
The stack trace in the bug also suggests that it crashes
because the input is null.

This CL makes change for that.

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

to

==========
Prevent Null input in texSubImage2D(3D)

In a previous CL which packs some repeated code into helper functions,
I made a mistake so that tex(Sub)Image2D(3D) can always take null input
for DOMArrayBufferView. However, I took a look at the original code
before my change, it turns out that texImage2D and texImage3D is allowed
to take null input, but texSubImage2D and texSubImage3D is not allowed.
The stack trace in the bug also suggests that it crashes
because the input is null.

This CL makes change for that.

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

[commit-bot: I haz the power, 2016-06-09 15:32:42]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-09 15:32:44]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-09 15:34:09]

Description was changed from

==========
Prevent Null input in texSubImage2D(3D)

In a previous CL which packs some repeated code into helper functions,
I made a mistake so that tex(Sub)Image2D(3D) can always take null input
for DOMArrayBufferView. However, I took a look at the original code
before my change, it turns out that texImage2D and texImage3D is allowed
to take null input, but texSubImage2D and texSubImage3D is not allowed.
The stack trace in the bug also suggests that it crashes
because the input is null.

This CL makes change for that.

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

to

==========
Prevent Null input in texSubImage2D(3D)

In a previous CL which packs some repeated code into helper functions,
I made a mistake so that tex(Sub)Image2D(3D) can always take null input
for DOMArrayBufferView. However, I took a look at the original code
before my change, it turns out that texImage2D and texImage3D is allowed
to take null input, but texSubImage2D and texSubImage3D is not allowed.
The stack trace in the bug also suggests that it crashes
because the input is null.

This CL makes change for that.

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

Committed: https://crrev.com/422466ba6888ba92906331f2adfe56123d722415
Cr-Commit-Position: refs/heads/master@{#398891}
==========

[commit-bot: I haz the power, 2016-06-09 15:34:10]

Patchset 4 (id:??) landed as
https://crrev.com/422466ba6888ba92906331f2adfe56123d722415
Cr-Commit-Position: refs/heads/master@{#398891}