927969 - WebGL conformance test error in conformance/context/context-lost-restored.html

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Jukka Jylänki]

On my laptop with the specs

Windows 7 Home Premium 64-bit
Intel(R) Core(TM) i5-2557M CPU @ 1.70GHz (4 CPUs), ~1.7GHz
4096MB RAM
Intel(R) HD Graphics 3000
Intel(R) GPU driver version 9.17.10.2932
Intel(R) GPU driver date 12/14/2012 (up-to-date)

running https://www.khronos.org/registry/webgl/sdk/tests/conformance/context/context-lost-restored.html 
with both Firefox 24 stable and Firefox 27.0a1 Nightly (2013-10-17) gives output:



Tests behavior under a restored context.

On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
Test losing a context and inability to restore it.
Test valid context
PASS gl.isContextLost() is false
PASS gl.getError() is gl.NO_ERROR
PASS shouldBe 255,10,20
PASS gl.getError() is gl.NO_ERROR

PASS gl.isContextLost() is true
PASS gl.getError() is gl.CONTEXT_LOST_WEBGL
PASS gl.getError() is gl.NO_ERROR
PASS gl.blendFunc(gl.TEXTURE_2D, gl.TEXTURE_CUBE_MAP) was expected value: NO_ERROR.
PASS contextLostEventFired is false
Test lost context
PASS contextLostEventFired is false
PASS gl.isContextLost() is true
PASS gl.getError() is gl.NO_ERROR

PASS WEBGL_lose_context.restoreContext() was expected value: INVALID_OPERATION.

Test losing and restoring a context.
Test valid context
PASS gl.isContextLost() is false
PASS gl.getError() is gl.NO_ERROR
PASS shouldBe 255,10,20
PASS gl.getError() is gl.NO_ERROR

PASS gl.isContextLost() is true
PASS gl.getError() is gl.CONTEXT_LOST_WEBGL
PASS gl.getError() is gl.NO_ERROR
PASS gl.blendFunc(gl.TEXTURE_2D, gl.TEXTURE_CUBE_MAP) was expected value: NO_ERROR.
PASS contextLostEventFired is false
Test lost context
PASS contextLostEventFired is false
PASS gl.isContextLost() is true
PASS gl.getError() is gl.NO_ERROR

PASS WEBGL_lose_context.restoreContext() was expected value: NO_ERROR.
PASS gl.isContextLost() is true
PASS gl.getError() is gl.NO_ERROR
PASS gl.blendFunc(gl.TEXTURE_2D, gl.TEXTURE_CUBE_MAP) was expected value: NO_ERROR.
Test restored context
PASS contextRestoredEventFired is false
PASS gl.isContextLost() is false
PASS gl.getError() is gl.NO_ERROR
PASS gl.bindTexture(gl.TEXTURE_2D, texture) was expected value: INVALID_OPERATION.
PASS gl.useProgram(program) was expected value: INVALID_OPERATION.
PASS gl.bindBuffer(gl.ARRAY_BUFFER, bufferObjects[0]) was expected value: INVALID_OPERATION.
PASS shouldBe 255,10,20
PASS gl.getError() is gl.NO_ERROR
PASS gl.bindTexture(gl.TEXTURE_2D, texture) was expected value: NO_ERROR.
PASS gl.useProgram(program) was expected value: NO_ERROR.
PASS gl.bindBuffer(gl.ARRAY_BUFFER, bufferObjects[0]) was expected value: NO_ERROR.
FAIL gl.texImage2D(gl.TEXTURE_2D, 0, gl.RGBA, 1, 1, 0, gl.RGBA, gl.FLOAT, null) expected: INVALID_ENUM. Was NO_ERROR.
PASS newExtension != null is true
FAIL newExtension.webglTestProperty === undefined should be true. Was false.
PASS gl.texImage2D(gl.TEXTURE_2D, 0, gl.RGBA, 1, 1, 0, gl.RGBA, gl.FLOAT, null) was expected value: NO_ERROR.
PASS newExtension != null is true
PASS newExtension.webglTestProperty === true is true

PASS successfullyParsed is true

TEST COMPLETE

Comment 1

[guozhu cheng]

please assign this conformance bug to me if no one have take it.

Comment 2

[guozhu cheng]

Attachment: bug-927969-fix-1.patch

disable extensions when WebGL context lose

Comment 3

[guozhu cheng]

ping~

Comment 4

[u480271]

Comment on attachment 827300 [details] [diff] [review]
bug-927969-fix-1.patch

Review of attachment 827300 [details] [diff] [review]:
-----------------------------------------------------------------

Please replace tabs by 4 spaces and remove all trailing whitespace to keep consistent with the rest of the file.

::: content/canvas/src/WebGLContext.cpp
@@ +273,5 @@
>      }
>  
> +	// disable all extensions. see bug #927969
> +	// spec: http://www.khronos.org/registry/webgl/specs/latest/1.0/#5.15.2
> +	for (size_t i = 0; i < size_t(WebGLExtensionID_max); i++) {

I see there is precedence for the size_t in WebGLContext::GetExtension, but given the first thing in the loop is a cast to enum WebGLExtensionID, I'd prefer replacing size_t with int and remove the cast.

@@ +276,5 @@
> +	// spec: http://www.khronos.org/registry/webgl/specs/latest/1.0/#5.15.2
> +	for (size_t i = 0; i < size_t(WebGLExtensionID_max); i++) {
> +		WebGLExtensionID extension = WebGLExtensionID(i);
> +		
> +		// "WEBGL_lose_context" extension will remain active through any loss of context. 

These two lines have trailing whitespace.

Comment 5

[u480271]

Given that all extensions except WEBGL_lose_context, couldn't the loop just be:

    for (int i = 0; i < WebGLExtensionID_max; i++) {
        WebGLExtensionID extension = WebGLExtensionID(i);
	
        if (extension == WEBGL_lost_context)
            continue;

        mExtensions[extension] = nullptr;
    }

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 827300 [details] [diff] [review]
bug-927969-fix-1.patch

Review of attachment 827300 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch; I think that Dan's suggestion in comment 5 is a good one, as it makes this code a bit more readable. Could you submit that for review? One nit though, I really prefer size_t (as in your patch) over int (as in comment 5) for the reason given below:

::: content/canvas/src/WebGLContext.cpp
@@ +273,5 @@
>      }
>  
> +	// disable all extensions. see bug #927969
> +	// spec: http://www.khronos.org/registry/webgl/specs/latest/1.0/#5.15.2
> +	for (size_t i = 0; i < size_t(WebGLExtensionID_max); i++) {

I support size_t here:

 1. size_t means exactly "this is an array index" which is descriptive here.

 2. The cast will be needed anyway once WebGLExtensionID is made a MFBT typed enum (MOZ_BEGIN_ENUM_CLASS) which I think it should be.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

I agree with the refactoring proposed in comment 5.

Comment 8

[u480271]

> One nit though, I really prefer size_t (as in your patch) over int
> (as in comment 5) for the reason given below:

Ugly cast is ugly, but c'est la vie. I defer to Benoit.

Comment 9

[guozhu cheng]

Attachment: bug-927969-fix-2.patch

follow review

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d67377d22e38

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d67377d22e38