682496 - program-test.html test failures

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Doug Sherk (:drs) (inactive)]

Running program-test.html gives the following error:
FAIL an attached shader shouldn't be deleted
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/programs/program-test.html

There's no checks within deleteShader() to see if the shader is attached to anything. Curiously enough, there's a function AttachCount() which seems to work (at least logically; I haven't debugged it) but even when gating on that being 0 before deleting, the error still occurs, and another new one appears too.

Comment 1

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.0, program-test.html error fix and changes to deletion marking behavior.

Will comment on changes in next comment.

Comment 2

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.0, program-test.html error fix and changes to deletion marking behavior.

Comment 3

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.0, program-test.html error fix and changes to deletion marking behavior.

Failed twice at uploading my changes.

Comment 4

[Doug Sherk (:drs) (inactive)]

What basically ended up happening was that programs and shaders weren't deleting themselves correctly, nor were programs deleting shaders at all. The assumptions I'm making in this patch are the following:

1) When programs and shaders have delete called on them, but are in use (attached for shaders, current program for programs), they should be marked as needing to be deleted. They will then be deleted when they are no longer in use.
2) If a shader is marked as pending deletion, compilation should be silently ignored, which is based on my interpretation of a comment from the WebKit people here: 
http://trac.webkit.org/browser/trunk/WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp?rev=68434#L678
3) If a shader becomes marked for deletion but is still attached to a program, but then this program becomes no longer current, this shader should be deleted too.

>@@ -378,16 +378,18 @@ public:
>                                              : mBoundCubeMapTextures[mActiveTexture];
>     }
> 
>     already_AddRefed<CanvasLayer> GetCanvasLayer(nsDisplayListBuilder* aBuilder,
>                                                  CanvasLayer *aOldLayer,
>                                                  LayerManager *aManager);
>     void MarkContextClean() { mInvalidated = PR_FALSE; }
> 
>+    void DetachShaders(WebGLProgram *program);

This was a result of DetachShaders() needing access to some data stored on the context.

>@@ -1157,16 +1159,22 @@ WebGLContext::DeleteShader(nsIWebGLShade
>     if (!GetConcreteObjectAndGLName("deleteShader", sobj, &shader, &shadername, &isNull, &isDeleted))
>         return NS_OK;
> 
>     if (isNull || isDeleted)
>         return NS_OK;
> 
>     MakeContextCurrent();
> 
>+    if (shader->AttachCount() > 0)
>+    {
>+        shader->SetDeletePending();
>+        return NS_OK;
>+    }

This seemed to be the fix to the original problem, but just adding this actually caused 2 more errors.

Comment 5

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.0, program-test.html error fix and changes to deletion marking behavior.

Failed again! Forgot to qref before I copied it.

Comment 6

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

(In reply to Doug Sherk from comment #4)
> What basically ended up happening was that programs and shaders weren't
> deleting themselves correctly, nor were programs deleting shaders at all.
> The assumptions I'm making in this patch are the following:
> 
> 1) When programs and shaders have delete called on them, but are in use
> (attached for shaders, current program for programs), they should be marked
> as needing to be deleted. They will then be deleted when they are no longer
> in use.

True

> 2) If a shader is marked as pending deletion, compilation should be silently
> ignored, which is based on my interpretation of a comment from the WebKit
> people here: 
> http://trac.webkit.org/browser/trunk/WebKit/chromium/src/
> WebGraphicsContext3DDefaultImpl.cpp?rev=68434#L678

Notice that is a very old WebKit revision.

I don't understand why compilation of a shader pending deletion should be silently ignored. Either a shader is deleted in which case attempting to compile it should generate GL_INVALID_OPERATION or it's not deleted in which case attempting to compile it should actually compile it. My understanding is that a shader pending deletion is not deleted hence it should still be compiled regardless of the pending-deletion status.

> 3) If a shader becomes marked for deletion but is still attached to a
> program, but then this program becomes no longer current, this shader should
> be deleted too.

Yes, unless of course it's attached to yet another program.

> 
> >@@ -378,16 +378,18 @@ public:
> >                                              : mBoundCubeMapTextures[mActiveTexture];
> >     }
> > 
> >     already_AddRefed<CanvasLayer> GetCanvasLayer(nsDisplayListBuilder* aBuilder,
> >                                                  CanvasLayer *aOldLayer,
> >                                                  LayerManager *aManager);
> >     void MarkContextClean() { mInvalidated = PR_FALSE; }
> > 
> >+    void DetachShaders(WebGLProgram *program);
> 
> This was a result of DetachShaders() needing access to some data stored on
> the context.

Still, why redeclare it here? All the other WebGL functions also access data store on the context and don't need to be redeclared here.

Comment 7

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.1, partial fix for comments.

Partially fixes comments by bjacob. Only addresses object design for DetachShaders(), does not address compilation of delete-marked shaders. This will be done pending discussion. Patch comment needs update.

Comment 8

[Doug Sherk (:drs) (inactive)]

Comment on attachment 557393 [details] [diff] [review]
Patch v1.1, partial fix for comments.

Also note that I have to delete the test from the failing_tests_* list again.

Comment 9

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.2, program-test.html error fix and changes to deletion marking behavior.

bjacob was completely right about my second assumption being wrong. What turned out happening was that, when compileShader() was called, the source code (or rather an untranslated version of it) was reloaded and then the shader was recompiled even if nothing had changed. This code path has been removed.

Comment 10

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

Comment on attachment 557733 [details] [diff] [review]
Patch v1.2, program-test.html error fix and changes to deletion marking behavior.

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

This looks all right! Just 1 trivial change needed below. Please carry forward r+.

Only thing I'm worried about is the complexity to ensure that there's no leak here i.e. that shaders that should be destroyed actually are destroyed. But I don't see any leak introduced by your patch.

::: content/canvas/src/WebGLContextUtils.cpp
@@ +212,5 @@
>      }
>  };
> +
> +void 
> +WebGLProgram::DetachShaders() {

Please keep all WebGLProgram methods inline in WebGLContext.h for now.

I know that's ugly but the real solution will be to have 1 file per class, not to put this in WebGLContextUtils.cpp

Comment 11

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.3, program-test.html error fix and changes to deletion marking behavior.

+r carried (r=bjacob)

This needs more work to actually fix the errors in program-test.html. However, the code was previously more flawed than the conformance tests showed.

Comment 12

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.4, program-test.html error fix and changes to deletion marking behavior.

Unbitrotted the previous patch, fixed other errors that have since cropped up in program-test.html, and removed changes to shader source compilation (someone else committed changes to it that fixed what this patch was supposed to fix). program-test.html now passes all OSX mochitests (haven't tried it elsewhere).

Comment 13

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.5, program-test.html error fix and changes to deletion marking behavior.

Made an accidental addition in the last patch to fix something on my local machine, also noticed another regression that I missed and should now be fixed.

Comment 14

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

Comment on attachment 566106 [details] [diff] [review]
Patch v1.5, program-test.html error fix and changes to deletion marking behavior.

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

Almost r+ but I still have a problem with one hunk:

::: content/canvas/src/WebGLContextGL.cpp
@@ +4143,5 @@
> +            GLint i = 0;
> +            gl->fGetShaderiv(shadername, pname, &i);
> +            wrval->SetAsBool(bool(i));
> +        }
> +            break;

What is the use of this hunk? It seems to be doing exactly the same as the existing code in next case statement.

Or did you want to let it check the WebGL deleted status?

Comment 15

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v2.0, program-test.html error fix and changes to deletion marking behavior.

So a combination of deciding that I didn't like my fix and also finding a regression that was difficult to fix convinced me to rewrite this basically from scratch. I addressed the code review comment, though :p

Also note that, although the previous code worked (except for one conformance error that I found on Linux), it was completely wrong in several areas.

Comment 16

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

Comment on attachment 567965 [details] [diff] [review]
Patch v2.0, program-test.html error fix and changes to deletion marking behavior.

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

::: content/canvas/src/WebGLContext.h
@@ +1419,5 @@
>      WebGLenum ShaderType() { return mType; }
>  
>      PRUint32 AttachCount() { return mAttachCount; }
>      void IncrementAttachCount() { mAttachCount++; }
>      void DecrementAttachCount() { mAttachCount--; }

mAttachCount should really be signed.

@@ +1496,4 @@
>      }
>  
>      void DetachShaders() {
> +        WebGLShader* shader;

make it local to the loop

::: content/canvas/src/WebGLContextGL.cpp
@@ +1184,1 @@
>      shader->Delete();

let's keep the Removes below the Deletes

Comment 17

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v2.1, program-test.html error fix and changes to deletion marking behavior.

Addressed code review comments.

Comment 18

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/d9da4de6579c

Comment 19

[Marco Bonardo [:mak]]

backed out for leaks in mochitest-1

TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 34760 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 44 instances of WebGLProgram with size 240 bytes each (10560 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 88 instances of WebGLShader with size 176 bytes each (15488 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 75 instances of WebGLUniformLocation with size 80 bytes each (6000 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 88 instances of nsStringBuffer with size 8 bytes each (704 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 251 instances of nsTArray_base with size 8 bytes each (2008 bytes total)

Comment 20

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v2.2, program-test.html error fix and changes to deletion marking behavior.

Fixed memory leaks.

Comment 21

[Doug Sherk (:drs) (inactive)]

Running on Try: https://tbpl.mozilla.org/?tree=Try&rev=20a50709f71c

Comment 22

[Doug Sherk (:drs) (inactive)]

More recent try push: https://tbpl.mozilla.org/?tree=Try&rev=69bb43191112

Comment 23

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

Comment on attachment 569994 [details] [diff] [review]
Patch v2.2, program-test.html error fix and changes to deletion marking behavior.

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

I'm not a fan of the name SafeDelete. I would prefer an explicit descriptive name, even if it's going to be long and clunky.

But since I can't think of a good name at the moment, r=me.

Comment 24

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v2.3, program-test.html error fix and changes to deletion marking behavior.

Changed name of SafeDelete() functions. +r carried.

Comment 25

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

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

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7da669483434