Last Comment Bug 682496 - program-test.html test failures
: program-test.html test failures
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Doug Sherk (:drs) (inactive)
:
:
Mentors:
Depends on:
Blocks: webgl-conformance 697211
  Show dependency treegraph
 
Reported: 2011-08-26 18:39 PDT by Doug Sherk (:drs) (inactive)
Modified: 2011-11-03 13:09 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1.0, program-test.html error fix and changes to deletion marking behavior. (8.39 KB, patch)
2011-08-30 18:28 PDT, Doug Sherk (:drs) (inactive)
no flags Details | Diff | Splinter Review
Patch v1.0, program-test.html error fix and changes to deletion marking behavior. (9.33 KB, patch)
2011-08-30 18:30 PDT, Doug Sherk (:drs) (inactive)
no flags Details | Diff | Splinter Review
Patch v1.0, program-test.html error fix and changes to deletion marking behavior. (9.33 KB, patch)
2011-08-30 18:45 PDT, Doug Sherk (:drs) (inactive)
no flags Details | Diff | Splinter Review
Patch v1.0, program-test.html error fix and changes to deletion marking behavior. (8.73 KB, patch)
2011-08-30 18:51 PDT, Doug Sherk (:drs) (inactive)
no flags Details | Diff | Splinter Review
Patch v1.1, partial fix for comments. (9.53 KB, patch)
2011-08-31 19:26 PDT, Doug Sherk (:drs) (inactive)
no flags Details | Diff | Splinter Review
Patch v1.2, program-test.html error fix and changes to deletion marking behavior. (8.14 KB, patch)
2011-09-01 18:31 PDT, Doug Sherk (:drs) (inactive)
jacob.benoit.1: review+
Details | Diff | Splinter Review
Patch v1.3, program-test.html error fix and changes to deletion marking behavior. (7.72 KB, patch)
2011-09-06 18:24 PDT, Doug Sherk (:drs) (inactive)
no flags Details | Diff | Splinter Review
Patch v1.4, program-test.html error fix and changes to deletion marking behavior. (6.85 KB, patch)
2011-10-10 17:38 PDT, Doug Sherk (:drs) (inactive)
no flags Details | Diff | Splinter Review
Patch v1.5, program-test.html error fix and changes to deletion marking behavior. (7.41 KB, patch)
2011-10-10 18:30 PDT, Doug Sherk (:drs) (inactive)
jacob.benoit.1: review-
Details | Diff | Splinter Review
Patch v2.0, program-test.html error fix and changes to deletion marking behavior. (9.57 KB, patch)
2011-10-18 21:45 PDT, Doug Sherk (:drs) (inactive)
no flags Details | Diff | Splinter Review
Patch v2.1, program-test.html error fix and changes to deletion marking behavior. (9.20 KB, patch)
2011-10-25 08:53 PDT, Doug Sherk (:drs) (inactive)
jacob.benoit.1: review+
Details | Diff | Splinter Review
Patch v2.2, program-test.html error fix and changes to deletion marking behavior. (10.00 KB, patch)
2011-10-27 09:02 PDT, Doug Sherk (:drs) (inactive)
jacob.benoit.1: review+
Details | Diff | Splinter Review
Patch v2.3, program-test.html error fix and changes to deletion marking behavior. (9.98 KB, patch)
2011-10-31 19:41 PDT, Doug Sherk (:drs) (inactive)
bugzilla: review+
Details | Diff | Splinter Review

Description Doug Sherk (:drs) (inactive) 2011-08-26 18:39:27 PDT
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) 2011-08-30 18:28:23 PDT
Created attachment 557058 [details] [diff] [review]
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) 2011-08-30 18:30:44 PDT
Created attachment 557060 [details] [diff] [review]
Patch v1.0, program-test.html error fix and changes to deletion marking behavior.
Comment 3 Doug Sherk (:drs) (inactive) 2011-08-30 18:45:44 PDT
Created attachment 557062 [details] [diff] [review]
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) 2011-08-30 18:46:11 PDT
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) 2011-08-30 18:51:59 PDT
Created attachment 557064 [details] [diff] [review]
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) 2011-08-31 14:20:44 PDT
(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) 2011-08-31 19:26:26 PDT
Created attachment 557393 [details] [diff] [review]
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) 2011-08-31 19:27:35 PDT
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) 2011-09-01 18:31:42 PDT
Created attachment 557733 [details] [diff] [review]
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) 2011-09-06 15:28:20 PDT
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) 2011-09-06 18:24:04 PDT
Created attachment 558693 [details] [diff] [review]
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) 2011-10-10 17:38:42 PDT
Created attachment 566092 [details] [diff] [review]
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) 2011-10-10 18:30:20 PDT
Created attachment 566106 [details] [diff] [review]
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) 2011-10-16 20:06:00 PDT
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) 2011-10-18 21:45:09 PDT
Created attachment 567965 [details] [diff] [review]
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) 2011-10-24 14:08:24 PDT
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) 2011-10-25 08:53:59 PDT
Created attachment 569392 [details] [diff] [review]
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) 2011-10-26 13:03:31 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/d9da4de6579c
Comment 19 Marco Bonardo [::mak] 2011-10-26 15:15:42 PDT
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) 2011-10-27 09:02:07 PDT
Created attachment 569994 [details] [diff] [review]
Patch v2.2, program-test.html error fix and changes to deletion marking behavior.

Fixed memory leaks.
Comment 21 Doug Sherk (:drs) (inactive) 2011-10-27 21:41:13 PDT
Running on Try: https://tbpl.mozilla.org/?tree=Try&rev=20a50709f71c
Comment 22 Doug Sherk (:drs) (inactive) 2011-10-31 18:43:41 PDT
More recent try push: https://tbpl.mozilla.org/?tree=Try&rev=69bb43191112
Comment 23 Benoit Jacob [:bjacob] (mostly away) 2011-10-31 18:45:22 PDT
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) 2011-10-31 19:41:13 PDT
Created attachment 570912 [details] [diff] [review]
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) 2011-11-03 07:58:57 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/7da669483434
Comment 26 Ed Morley [:emorley] 2011-11-03 13:09:51 PDT
https://hg.mozilla.org/mozilla-central/rev/7da669483434

Note You need to log in before you can comment on or make changes to this bug.