Fix the crash of compressedTexImage3D for the WebGL2 conformance test page.

Fix the crash setLevelInfo for the WebGL2 conformance test page.

We observe crashes in WebView when blink::WebGLTexture::setLevelInfo
is called. It need to check validation of level
in the WebGL2RenderingContextBase::compressedTexImage3D.
And fix the fail for the compressedteximage3d_neg_level test.

BUG=560119
Committed: https://crrev.com/12c53bfc00537988a571e285914849daec9bfc77
Cr-Commit-Position: refs/heads/master@{#361473}

[cyzero.kim, 2015-11-23 05:28:28]

cyzero.kim@samsung.com changed reviewers:
+ kbr@chromium.org, zmo@chromium.org

[cyzero.kim, 2015-11-23 05:28:28]

Please review this one.

[qiankun, 2015-11-23 06:18:59]

qiankun.miao@intel.com changed reviewers:
+ qiankun.miao@intel.com

[qiankun, 2015-11-23 06:19:00]

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

https://codereview.chromium.org/1468883002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLTexture.cpp:178: return;
See the above comments. level should be validated already when entering
setLevelInfo. I think you should do level validation in
WebGL2RenderingContextBase::compressedTexImage3D.

[cyzero.kim, 2015-11-23 08:53:31]

On 2015/11/23 06:19:00, qiankun wrote:
>
https://codereview.chromium.org/1468883002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/webgl/WebGLTexture.cpp (right):
> 
>
https://codereview.chromium.org/1468883002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/webgl/WebGLTexture.cpp:178: return;
> See the above comments. level should be validated already when entering
> setLevelInfo. I think you should do level validation in
> WebGL2RenderingContextBase::compressedTexImage3D.

Thanks for fast review.
You are right.
I have moved it.
Please take another look.

[qiankun, 2015-11-23 09:44:27]

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

https://codereview.chromium.org/1468883002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:932: if
(internalformat != tex->getInternalFormat(target, level)) {
CompressedTexImage3D is used to specify a three-dimensional texture image in a
compressed format. So no previously defined texture. This check should always
fail. Also ES3 doesn't have such limitation on internalformat matching. I think
you should do level validation here according your first CL.

[cyzero.kim, 2015-11-23 12:00:58]

On 2015/11/23 09:44:27, qiankun wrote:
>
https://codereview.chromium.org/1468883002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
> (right):
> 
>
https://codereview.chromium.org/1468883002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:932: if
> (internalformat != tex->getInternalFormat(target, level)) {
> CompressedTexImage3D is used to specify a three-dimensional texture image in a
> compressed format. So no previously defined texture. This check should always
> fail. Also ES3 doesn't have such limitation on internalformat matching. I
think
> you should do level validation here according your first CL.

Yes, I've changed for only to check validation of level.
Please take another look.

[qiankun, 2015-11-23 12:06:54]

https://codereview.chromium.org/1468883002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/1468883002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:932: if
(!tex->isValidLevel(target, level)) {
Please use validateTexFuncLevel(). You can refer the level validation in
WebGLRenderingContextBase::compressedTexImage2D().

[cyzero.kim, 2015-11-23 13:26:44]

On 2015/11/23 12:06:54, qiankun wrote:
>
https://codereview.chromium.org/1468883002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
> (right):
> 
>
https://codereview.chromium.org/1468883002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:932: if
> (!tex->isValidLevel(target, level)) {
> Please use validateTexFuncLevel(). You can refer the level validation in
> WebGLRenderingContextBase::compressedTexImage2D().

I agree your opinion.
Please take another look.

[zmo, 2015-11-23 19:07:13]

zmo@google.com changed reviewers:
+ zmo@google.com

[zmo, 2015-11-23 19:07:14]

https://codereview.chromium.org/1468883002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/1468883002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:932: if
(!validateTexFuncLevel("compressedTexImage3D", target, level))
I agree with Qiankun this is the right thing to do.  However, you need to update
getMaxTextureLevelForTarget() to handle 3D texture targets to make it work.

[cyzero.kim, 2015-11-24 00:57:49]

On 2015/11/23 19:07:14, zmo wrote:
>
https://codereview.chromium.org/1468883002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
> (right):
> 
>
https://codereview.chromium.org/1468883002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:932: if
> (!validateTexFuncLevel("compressedTexImage3D", target, level))
> I agree with Qiankun this is the right thing to do.  However, you need to
update
> getMaxTextureLevelForTarget() to handle 3D texture targets to make it work.

There was a missing point.
Everything has a problem that using it.(texImage3D, texSubImage3D,
copyTexSubImage3D)
I've update it.
So, I've solve crash problem also got the pass test as well.

Start testcase: negativeTextureApi.compressedteximage3d_neg_level
PASS negativeTextureApi.compressedteximage3d_neg_level: Passed

Start testcase: negativeTextureApi.compressedteximage3d_max_level
PASS negativeTextureApi.compressedteximage3d_max_level: Passed

Please take another look.

[zmo, 2015-11-24 01:09:04]

On 2015/11/24 00:57:49, cyzero.kim wrote:
> On 2015/11/23 19:07:14, zmo wrote:
> >
>
https://codereview.chromium.org/1468883002/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/1468883002/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:932:
if
> > (!validateTexFuncLevel("compressedTexImage3D", target, level))
> > I agree with Qiankun this is the right thing to do.  However, you need to
> update
> > getMaxTextureLevelForTarget() to handle 3D texture targets to make it work.
> 
> There was a missing point.
> Everything has a problem that using it.(texImage3D, texSubImage3D,
> copyTexSubImage3D)
> I've update it.
> So, I've solve crash problem also got the pass test as well.
> 
> Start testcase: negativeTextureApi.compressedteximage3d_neg_level
> PASS negativeTextureApi.compressedteximage3d_neg_level: Passed
> 
> Start testcase: negativeTextureApi.compressedteximage3d_max_level
> PASS negativeTextureApi.compressedteximage3d_max_level: Passed
> 
> Please take another look.

My apology to mislead you.  Actually your original CL (patch set 4) is good. The
getMaxTextureLevelForTarget() is overridden in WebGL2RenderingContextBase to
handle the 3D textures.

Could you please delete patch set 5 and land patch set 4?

Patch set 4 LGTM

[cyzero.kim, 2015-11-24 01:50:34]

Patchset #5 (id:80001) has been deleted

[cyzero.kim, 2015-11-24 01:55:52]

The CQ bit was checked by cyzero.kim@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-24 02:00:06]

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

[commit-bot: I haz the power, 2015-11-24 02:00:07]

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

[commit-bot: I haz the power, 2015-11-24 02:00:07]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
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.

[cyzero.kim, 2015-11-24 02:04:43]

On 2015/11/24 01:09:04, zmo wrote:
> On 2015/11/24 00:57:49, cyzero.kim wrote:
> > On 2015/11/23 19:07:14, zmo wrote:
> > >
> >
>
https://codereview.chromium.org/1468883002/diff/60001/third_party/WebKit/Sour...
> > > File
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1468883002/diff/60001/third_party/WebKit/Sour...
> > >
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:932:
> if
> > > (!validateTexFuncLevel("compressedTexImage3D", target, level))
> > > I agree with Qiankun this is the right thing to do.  However, you need to
> > update
> > > getMaxTextureLevelForTarget() to handle 3D texture targets to make it
work.
> > 
> > There was a missing point.
> > Everything has a problem that using it.(texImage3D, texSubImage3D,
> > copyTexSubImage3D)
> > I've update it.
> > So, I've solve crash problem also got the pass test as well.
> > 
> > Start testcase: negativeTextureApi.compressedteximage3d_neg_level
> > PASS negativeTextureApi.compressedteximage3d_neg_level: Passed
> > 
> > Start testcase: negativeTextureApi.compressedteximage3d_max_level
> > PASS negativeTextureApi.compressedteximage3d_max_level: Passed
> > 
> > Please take another look.
> 
> My apology to mislead you.  Actually your original CL (patch set 4) is good.
The
> getMaxTextureLevelForTarget() is overridden in WebGL2RenderingContextBase to
> handle the 3D textures.
> 
> Could you please delete patch set 5 and land patch set 4?
> 
> Patch set 4 LGTM

I've deleted patch set 5.
Please check this one.

[cyzero.kim, 2015-11-24 02:06:51]

The CQ bit was checked by cyzero.kim@samsung.com

[commit-bot: I haz the power, 2015-11-24 02:13:26]

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

[commit-bot: I haz the power, 2015-11-24 02:13:27]

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

[commit-bot: I haz the power, 2015-11-24 02:13:27]

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
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), 2015-11-24 02:43:35]

On 2015/11/24 02:13:27, commit-bot: I haz the power wrote:
> No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
> 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.

Oops, looks like Mo reviewed this from the wrong account by accident. LGTM

[qiankun, 2015-11-24 03:13:39]

Could you update the tile and description of this CL? The origin description is
a bit inaccurate for current code.

[cyzero.kim, 2015-11-24 04:26:27]

The CQ bit was checked by cyzero.kim@samsung.com

[commit-bot: I haz the power, 2015-11-24 04:28:14]

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

[commit-bot: I haz the power, 2015-11-24 07:21:53]

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

[commit-bot: I haz the power, 2015-11-24 07:21:54]

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

[cyzero.kim, 2015-11-24 07:28:14]

Description was changed from

==========
Fix the crash setLevelInfo for the WebGL2 conformance test page.

We observe crashes in WebView when blink::WebGLTexture::setLevelInfo
is called. It need to check valid of level.

BUG=560119
==========

to

==========
Fix the crash setLevelInfo for the WebGL2 conformance test page.

We observe crashes in WebView when blink::WebGLTexture::setLevelInfo
is called. It need to check validation of level
in the WebGL2RenderingContextBase::compressedTexImage3D.
And fix the fail for the compressedteximage3d_neg_level test.

BUG=560119
==========

[cyzero.kim, 2015-11-24 08:57:01]

On 2015/11/24 07:21:54, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

I've updated description.
But I got the CQ's trybots problem.
This would not be relevant. Please check this one.

[zmo, 2015-11-24 17:20:06]

The CQ bit was checked by zmo@google.com

[commit-bot: I haz the power, 2015-11-24 17:21:30]

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

[commit-bot: I haz the power, 2015-11-24 20:01:59]

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

[commit-bot: I haz the power, 2015-11-24 20:02:00]

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

[Ken Russell (switch to Gerrit), 2015-11-24 21:18:12]

The CQ bit was checked by kbr@chromium.org

[commit-bot: I haz the power, 2015-11-24 21:20:18]

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

[commit-bot: I haz the power, 2015-11-24 22:37:11]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-24 22:38:00]

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