Make WebGL's texImage2D and texSubImage2D handle cross-origin ImageBitmap

Make WebGL's texImage2D and texSubImage2D handle cross-origin ImageBitmap

Now that ImageBitmap has an origin-clean flag, this CL changes WebGL's
texImage2D(ImageBitmap) and texSubImage2D(ImageBitmap) so that it will
throw a security error when the ImageBitmap is not origin clean.

Also, a small change has been made to texImage2D::validateTexFunc.

BUG=576280
Committed: https://crrev.com/7f6ec1189d09fcf01cf6d4ea41ed43ec91dfdbac
Cr-Commit-Position: refs/heads/master@{#369025}

[xidachen, 2016-01-11 16:17:44]

xidachen@chromium.org changed reviewers:
+ junov@chromium.org, kbr@chromium.org

[xidachen, 2016-01-11 16:17:44]

PTAL

[Justin Novosad, 2016-01-11 17:52:13]

Missing a test.

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

https://codereview.chromium.org/1573173002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4516: if
(!bitmap->originClean()) {
Could you factor this out into a helper, like the other flavors texImage2D? For
example, see "validateHTMLCanvasElement".  While you are at it, the isNeutered
check should be moved there as well.

https://codereview.chromium.org/1573173002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4746: if
(!bitmap->originClean()) {
Ditto. -> use helper to avoid duplicated code (and duplicated message strings).

[xidachen, 2016-01-11 20:37:16]

PTAL. We have to wait until this CL: https://codereview.chromium.org/1532473002/
gets landed.

[Justin Novosad, 2016-01-11 21:08:53]

https://codereview.chromium.org/1573173002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/webgl-cross-origin-ImageBitmap-blocked.html
(right):

https://codereview.chromium.org/1573173002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/webgl-cross-origin-ImageBitmap-blocked.html:6:
description("WebGL's texImage2D should throw security error when the ImageBitmap
is not origin clean.");
Nit: "throw security error" -> "throw a SecurityError exception"

https://codereview.chromium.org/1573173002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/webgl-cross-origin-ImageBitmap-blocked.html:23:
shouldThrow("gl.texImage2D(gl.TEXTURE_2D, 0, gl.RGBA, gl.RGBA, gl.UNSIGNED_BYTE,
bitmap)");
I think you should make a call to texImage2D that would otherwise succeed if it
was not cross-origin. I am not a WebGL expert, but I think this means first
calling genTextures and binding the generated texture to TEXTURE_2D. Also,
please add a second "shouldThrow" that covers texSubImage2D

[Justin Novosad, 2016-01-11 21:11:03]

On 2016/01/11 21:08:53, Justin Novosad wrote:
>  Also,
> please add a second "shouldThrow" that covers texSubImage2D

To do this, you will first need to make a call to texImage2D that succeeds
(non-cross-origin) in order to have a properly initialized texture that you can
"subImage" to.

[xidachen, 2016-01-12 02:35:50]

PTAL.

https://codereview.chromium.org/1573173002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/webgl-cross-origin-ImageBitmap-blocked.html
(right):

https://codereview.chromium.org/1573173002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/webgl-cross-origin-ImageBitmap-blocked.html:6:
description("WebGL's texImage2D should throw security error when the ImageBitmap
is not origin clean.");
On 2016/01/11 21:08:53, Justin Novosad wrote:
> Nit: "throw security error" -> "throw a SecurityError exception"

Acknowledged.

https://codereview.chromium.org/1573173002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/webgl-cross-origin-ImageBitmap-blocked.html:23:
shouldThrow("gl.texImage2D(gl.TEXTURE_2D, 0, gl.RGBA, gl.RGBA, gl.UNSIGNED_BYTE,
bitmap)");
On 2016/01/11 21:08:53, Justin Novosad wrote:
> I think you should make a call to texImage2D that would otherwise succeed if
it
> was not cross-origin. I am not a WebGL expert, but I think this means first
> calling genTextures and binding the generated texture to TEXTURE_2D. Also,
> please add a second "shouldThrow" that covers texSubImage2D

Acknowledged.

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

Nice work overall. Couple of issues with the test.

https://codereview.chromium.org/1573173002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/webgl-cross-origin-ImageBitmap-blocked.html
(right):

https://codereview.chromium.org/1573173002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/webgl-cross-origin-ImageBitmap-blocked.html:23:
shouldNotThrow("gl.texImage2D(gl.TEXTURE_2D, 0, gl.RGBA, gl.RGBA,
gl.UNSIGNED_BYTE, bitmap)");
Follow this up with shouldBe("gl.getError()", "gl.NO_ERROR").

https://codereview.chromium.org/1573173002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/webgl-cross-origin-ImageBitmap-blocked.html:24:
shouldNotThrow("gl.texSubImage2D(gl.TEXTURE_2D, 0, 0, 0, gl.RGBA,
gl.UNSIGNED_BYTE, bitmap)");
Same here.

https://codereview.chromium.org/1573173002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/webgl-cross-origin-ImageBitmap-blocked.html:31:
gl = canvas.getContext("webgl");
You should create a texture object and bind it to the TEXTURE_2D binding point.
Otherwise all of the GL calls can fail for other reasons, and you want to make
sure that only one possible failure can happen because of the tainted
ImageBitmap.

  var texture = gl.createTexture();
  gl.bindTexture(gl.TEXTURE_2D, texture);

https://codereview.chromium.org/1573173002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/webgl-cross-origin-ImageBitmap-blocked.html:34:
shouldThrow("gl.texImage2D(gl.TEXTURE_2D, 0, gl.RGBA, gl.RGBA, gl.UNSIGNED_BYTE,
bitmap)");
Follow this up with shouldBe("gl.getError()", "gl.NO_ERROR").

https://codereview.chromium.org/1573173002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/webgl-cross-origin-ImageBitmap-blocked.html:35:
shouldThrow("gl.texSubImage2D(gl.TEXTURE_2D, 0, 0, 0, gl.RGBA, gl.UNSIGNED_BYTE,
bitmap)");
Same here.

https://codereview.chromium.org/1573173002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/webgl-cross-origin-ImageBitmap-blocked.html:40:
});
Shouldn't this entire second createImageBitmap call and its promise callbacks go
inside the success callback of the first promise (after the second
"shouldNotThrow" for texSubImage2D)? Otherwise the promises may resolve in an
arbitrary order, leading to flakiness of the layout test.

[xidachen, 2016-01-12 13:58:37]

Hi Ken,

Thank you for the comments. I made changes accordingly. But now I am seeing
strange behavior. In the result of the layout test, you can see that the first
two lines of
shouldBe("gl.getError()", "gl.NO_ERROR");
failed. But if I un-comment the
console.log(gl.getError().toString(16));
lines, then the first two lines of gl.getError() would pass. I am not sure
what's happening here.

https://codereview.chromium.org/1573173002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/webgl-cross-origin-ImageBitmap-blocked.html
(right):

https://codereview.chromium.org/1573173002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/webgl-cross-origin-ImageBitmap-blocked.html:23:
shouldNotThrow("gl.texImage2D(gl.TEXTURE_2D, 0, gl.RGBA, gl.RGBA,
gl.UNSIGNED_BYTE, bitmap)");
On 2016/01/12 05:03:22, Ken Russell wrote:
> Follow this up with shouldBe("gl.getError()", "gl.NO_ERROR").

Acknowledged.

https://codereview.chromium.org/1573173002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/webgl-cross-origin-ImageBitmap-blocked.html:24:
shouldNotThrow("gl.texSubImage2D(gl.TEXTURE_2D, 0, 0, 0, gl.RGBA,
gl.UNSIGNED_BYTE, bitmap)");
On 2016/01/12 05:03:22, Ken Russell wrote:
> Same here.

Acknowledged.

https://codereview.chromium.org/1573173002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/webgl-cross-origin-ImageBitmap-blocked.html:31:
gl = canvas.getContext("webgl");
On 2016/01/12 05:03:22, Ken Russell wrote:
> You should create a texture object and bind it to the TEXTURE_2D binding
point.
> Otherwise all of the GL calls can fail for other reasons, and you want to make
> sure that only one possible failure can happen because of the tainted
> ImageBitmap.
> 
>   var texture = gl.createTexture();
>   gl.bindTexture(gl.TEXTURE_2D, texture);

Acknowledged.

https://codereview.chromium.org/1573173002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/webgl-cross-origin-ImageBitmap-blocked.html:34:
shouldThrow("gl.texImage2D(gl.TEXTURE_2D, 0, gl.RGBA, gl.RGBA, gl.UNSIGNED_BYTE,
bitmap)");
On 2016/01/12 05:03:22, Ken Russell wrote:
> Follow this up with shouldBe("gl.getError()", "gl.NO_ERROR").

Acknowledged.

https://codereview.chromium.org/1573173002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/webgl-cross-origin-ImageBitmap-blocked.html:35:
shouldThrow("gl.texSubImage2D(gl.TEXTURE_2D, 0, 0, 0, gl.RGBA, gl.UNSIGNED_BYTE,
bitmap)");
On 2016/01/12 05:03:22, Ken Russell wrote:
> Same here.

Acknowledged.

https://codereview.chromium.org/1573173002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/webgl-cross-origin-ImageBitmap-blocked.html:40:
});
On 2016/01/12 05:03:22, Ken Russell wrote:
> Shouldn't this entire second createImageBitmap call and its promise callbacks
go
> inside the success callback of the first promise (after the second
> "shouldNotThrow" for texSubImage2D)? Otherwise the promises may resolve in an
> arbitrary order, leading to flakiness of the layout test.

Acknowledged.

[Ken Russell (switch to Gerrit), 2016-01-12 19:28:57]

On 2016/01/12 13:58:37, xidachen wrote:
> Hi Ken,
> 
> Thank you for the comments. I made changes accordingly. But now I am seeing
> strange behavior. In the result of the layout test, you can see that the first
> two lines of
> shouldBe("gl.getError()", "gl.NO_ERROR");
> failed. But if I un-comment the
> console.log(gl.getError().toString(16));
> lines, then the first two lines of gl.getError() would pass. I am not sure
> what's happening here.

It looks like an OpenGL error is in fact being generated during the texture
upload. Please read the spec and dig into why that might be happening. Thanks.

[Justin Novosad, 2016-01-12 19:58:06]

FYI: In WebGL2 there is texSubImage3D that needs to be taken care of.

[xidachen, 2016-01-12 20:24:57]

Description was changed from

==========
Make WebGL's texImage2D and texSubImage2D handle cross-origin ImageBitmap

Now that ImageBitmap has an origin-clean flag, this CL changes WebGL's
texImage2D(ImageBitmap) and texSubImage2D(ImageBitmap) so that it will
throw a security error when the ImageBitmap is not origin clean.

BUG=576280
==========

to

==========
Make WebGL's texImage2D and texSubImage2D handle cross-origin ImageBitmap

Now that ImageBitmap has an origin-clean flag, this CL changes WebGL's
texImage2D(ImageBitmap) and texSubImage2D(ImageBitmap) so that it will
throw a security error when the ImageBitmap is not origin clean.

Also, a small change has been made to texImage2D::validateTexFunc.

BUG=576280
==========

[xidachen, 2016-01-12 20:26:02]

Hi Ken,

Please take a look. I made a change in texImage2D::validateTexFunc, and it
should be fine now.

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

https://codereview.chromium.org/1573173002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:4512: if
(isContextLost() || !validateImageBitmap("texImage2D", bitmap.get(),
exceptionState) || !validateTexFunc("texImage2D", TexImage, SourceImageBitmap,
target, level, internalformat, bitmap->width(), bitmap->height(), 1, 0, format,
type, 0, 0, 0))
This is where the problem was, it should pass internalformat where it was 0.

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

Very good. LGTM

[xidachen, 2016-01-12 21:22:27]

The CQ bit was checked by xidachen@chromium.org

[commit-bot: I haz the power, 2016-01-12 21:23:42]

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

[commit-bot: I haz the power, 2016-01-12 23:09:09]

Description was changed from

==========
Make WebGL's texImage2D and texSubImage2D handle cross-origin ImageBitmap

Now that ImageBitmap has an origin-clean flag, this CL changes WebGL's
texImage2D(ImageBitmap) and texSubImage2D(ImageBitmap) so that it will
throw a security error when the ImageBitmap is not origin clean.

Also, a small change has been made to texImage2D::validateTexFunc.

BUG=576280
==========

to

==========
Make WebGL's texImage2D and texSubImage2D handle cross-origin ImageBitmap

Now that ImageBitmap has an origin-clean flag, this CL changes WebGL's
texImage2D(ImageBitmap) and texSubImage2D(ImageBitmap) so that it will
throw a security error when the ImageBitmap is not origin clean.

Also, a small change has been made to texImage2D::validateTexFunc.

BUG=576280
==========

[commit-bot: I haz the power, 2016-01-12 23:09:10]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-12 23:10:04]

Description was changed from

==========
Make WebGL's texImage2D and texSubImage2D handle cross-origin ImageBitmap

Now that ImageBitmap has an origin-clean flag, this CL changes WebGL's
texImage2D(ImageBitmap) and texSubImage2D(ImageBitmap) so that it will
throw a security error when the ImageBitmap is not origin clean.

Also, a small change has been made to texImage2D::validateTexFunc.

BUG=576280
==========

to

==========
Make WebGL's texImage2D and texSubImage2D handle cross-origin ImageBitmap

Now that ImageBitmap has an origin-clean flag, this CL changes WebGL's
texImage2D(ImageBitmap) and texSubImage2D(ImageBitmap) so that it will
throw a security error when the ImageBitmap is not origin clean.

Also, a small change has been made to texImage2D::validateTexFunc.

BUG=576280

Committed: https://crrev.com/7f6ec1189d09fcf01cf6d4ea41ed43ec91dfdbac
Cr-Commit-Position: refs/heads/master@{#369025}
==========

[commit-bot: I haz the power, 2016-01-12 23:10:05]

Patchset 5 (id:??) landed as
https://crrev.com/7f6ec1189d09fcf01cf6d4ea41ed43ec91dfdbac
Cr-Commit-Position: refs/heads/master@{#369025}