710163 - EXT_lose_context semantics are no longer valid

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Doug Sherk (:drs) (inactive)]

The WEBGL_EXT_lose_context extension's semantics are no longer valid with the updated spec. 

See:
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/specs/latest/index-context-lost.html#5.15

and...:
http://people.mozilla.org/~dsherk/webgl/sdk/tests/conformance/context/context-lost-restored.html

This is a patched version of the Khronos trunk version, which can be found at:
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/context/context-lost-restored.html

Comment 1

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.0, fix EXT_context_loss semantics

I believe that the new context-loss.html test may be impossible to pass in its current state. I sent out an email to public_webgl:

"I don't believe right now that the Khronos version of context-lost.html can pass with the semantics described in this thread. The following check seems to me impossible to pass:

shouldGenerateGLError(gl, gl.NO_ERROR, "extension.loseContext()");

I'm assuming the following:

* extension.loseContext() should trigger a context loss immediately
* as long as a context is lost, the first getError() call should return CONTEXT_LOST_WEBGL

Note that shouldGenerateGLError eval()'s the statement then calls getError() immediately afterwards if no exception was thrown.

This should always trigger either an INVALID_OPERATION or CONTEXT_LOST_WEBGL. Perhaps I'm missing something?"

I "fixed" this test (the change was trivial) and also fixed context-lost-restored.html, which had some typos (but otherwise I didn't change any functionality). The tests can be found here:
http://people.mozilla.org/~dsherk/webgl/sdk/tests/conformance/context/context-lost.html
http://people.mozilla.org/~dsherk/webgl/sdk/tests/conformance/context/context-lost-restored.html

This patch fixes our EXT_context_loss implementation to pass these two tests. 

It also renames WEBGL_EXT_lose_context to the new standard, MOZ_WEBGL_lose_context.

This shouldn't be landed until we update our conformance suite.

Comment 2

[Doug Sherk (:drs) (inactive)]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=ee416a3c7174

Note that there are M1 failures. These are completely expected as this shouldn't be landed until the conformance suite is updated; that is, the tests that we're performing are invalid.

Comment 3

[Doug Sherk (:drs) (inactive)]

Also, I tested the GLX, WGL and new EGL robustness implementations and they seemed to all work as we would expect them to.

Comment 4

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.1, fix EXT_context_loss semantics

I broke this up into two separate patches because it was impossible to distinguish boilerplate mContextLost->!IsContextStable() changes from actual logic.

Comment 5

[Doug Sherk (:drs) (inactive)]

Attachment: (part 2) Patch v1.1, fix EXT_context_loss semantics.

Second part, just tons of replacements of "mContextLost" with "!IsContextStable()". There should be no signficant logic changes here.

Comment 6

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.1, fix EXT_context_loss semantics

I suck at bugzilla. Please refer to previous comments.

Comment 7

[Doug Sherk (:drs) (inactive)]

To answer the age-old question, "oh god what is this", I'm writing up some docs on what happened here.

The previous iteration of the context loss testing extension was very poorly defined and didn't actually really help with testing that scripts indeed handled context loss correctly. The WebGL WG decided to update the spec, as you can see here:
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/specs/latest/index-context-lost.html#5.15.2

The changes are largely in how a context loss should be handled asynchronously rather than synchronously. We used to send a webglcontextlost event immediately when we detected a context loss, or when extension.loseContext() was called. Now, we use the robustness timer to send it on the next Notify().

The script then has the option of handling it, just as it did before. However, one change is that a script must handle the webglcontextlost event if it triggered loseContext(), otherwise it can't use restoreContext(). This change was done to more closely mirror real functionality. Real functionality, i.e. when a DoS happens, is identical in this regard.

After the script has handled it, on the next Notify, we then try to restore the context. This is in contrast to again doing this synchronously. It used to be that a context loss detection, the webglcontextlost event, the actual restore, and the webglcontextrestored event were fired all at the same time, but now they happen over 3 Notify iterations.

This required a big change in how we mark a context as lost. Instead of just being "lost" or "not lost", we now have states. These states are as follows (also described in the patch/code):
1) Stable, i.e. we don't know of any problems or there just aren't any.
2) We know it's lost, but haven't sent out the asynchronous webglcontextlost event.
3) It is lost and we have sent out the event; the script has either not handled the event or we aren't going to restore it for some other reason.
4) It is lost and waiting to be restored on the next Notify.

Another small change I made was that I noticed it was flipping the height/width dimensions on context restore (no idea how I missed this before, probably because I was working entirely working with square canvases and I've never seen a dimension specified as width x height rather than height x width). I swapped these so it should restore correctly now.

Comment 8

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.1, fix EXT_context_loss semantics

Comment 9

[Doug Sherk (:drs) (inactive)]

Attachment: (part 2) Patch v1.1, fix EXT_context_loss semantics.

Comment 10

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

Comment on attachment 582163 [details] [diff] [review]
Patch v1.1, fix EXT_context_loss semantics

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

Looks wonderful!

Comment 11

[Doug Sherk (:drs) (inactive)]

Attachment: (part 3) Patch v1.1, fix EXT_lose_context semantics.

With this patch and bug 711592's, this entire patch stack should now work. Unfortunately, I can't push it to try or inbound because my moco account has been deleted and my permissions seem to be tied to that.

Comment 12

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

(In reply to Doug Sherk (:dRdR) from comment #11)
> Created attachment 582756 [details] [diff] [review]
> (part 3) Patch v1.1, fix EXT_lose_context semantics.
> 
> With this patch and bug 711592's, this entire patch stack should now work.
> Unfortunately, I can't push it to try or inbound because my moco account has
> been deleted and my permissions seem to be tied to that.

Tying your hg permissions to your employment status seems unlikely. Should be just an accident. File an IT bug (like when you got access) to explain the problem, there should be an easy solution.

Comment 13

[Doug Sherk (:drs) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7dd5039ca2e7

Comment 14

[Ed Morley [:emorley]]

This cset was empty:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/7dd5039ca2e7

Comment 15

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/7dd5039ca2e7
https://hg.mozilla.org/mozilla-central/rev/23f3b97f655a
https://hg.mozilla.org/mozilla-central/rev/8e3d4ad9e413

leaving open for the empty changeset to be fixed.

Comment 16

[Doug Sherk (:drs) (inactive)]

Please close; empty cset was ok, the changes made in that page were made by someone else, so it bitrotted and I accidentally pushed it as empty anyways.