The default bug view has changed. See this FAQ.

program-test.html test failures

RESOLVED FIXED in mozilla10

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: drs, Assigned: drs)

Tracking

Trunk
mozilla10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 12 obsolete attachments)

9.98 KB, patch
drs
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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.
Assignee: nobody → dsherk
Attachment #557058 - Flags: review?(bjacob)
(Assignee)

Comment 2

6 years ago
Created attachment 557060 [details] [diff] [review]
Patch v1.0, program-test.html error fix and changes to deletion marking behavior.
Attachment #557058 - Attachment is obsolete: true
Attachment #557058 - Flags: review?(bjacob)
Attachment #557060 - Flags: review?(bjacob)
(Assignee)

Comment 3

6 years ago
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.
Attachment #557060 - Attachment is obsolete: true
Attachment #557060 - Flags: review?(bjacob)
Attachment #557062 - Flags: review?(bjacob)
(Assignee)

Comment 4

6 years ago
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.
(Assignee)

Comment 5

6 years ago
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.
Attachment #557062 - Attachment is obsolete: true
Attachment #557062 - Flags: review?(bjacob)
Attachment #557064 - Flags: review?(bjacob)
(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.
(Assignee)

Comment 7

6 years ago
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.
Attachment #557064 - Attachment is obsolete: true
Attachment #557064 - Flags: review?(bjacob)
Attachment #557393 - Flags: review?(bjacob)
(Assignee)

Comment 8

6 years ago
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.
Blocks: 680721
(Assignee)

Comment 9

6 years ago
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.
Attachment #557393 - Attachment is obsolete: true
Attachment #557393 - Flags: review?(bjacob)
Attachment #557733 - Flags: review?(bjacob)
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
Attachment #557733 - Flags: review?(bjacob) → review+
(Assignee)

Comment 11

6 years ago
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.
Attachment #557733 - Attachment is obsolete: true
(Assignee)

Comment 12

6 years ago
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).
Attachment #558693 - Attachment is obsolete: true
Attachment #566092 - Flags: review?(bjacob)
(Assignee)

Comment 13

6 years ago
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.
Attachment #566092 - Attachment is obsolete: true
Attachment #566092 - Flags: review?(bjacob)
Attachment #566106 - Flags: review?(bjacob)
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?
Attachment #566106 - Flags: review?(bjacob) → review-
(Assignee)

Comment 15

6 years ago
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.
Attachment #566106 - Attachment is obsolete: true
Attachment #567965 - Flags: review?(bjacob)
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
(Assignee)

Comment 17

6 years ago
Created attachment 569392 [details] [diff] [review]
Patch v2.1, program-test.html error fix and changes to deletion marking behavior.

Addressed code review comments.
Attachment #567965 - Attachment is obsolete: true
Attachment #567965 - Flags: review?(bjacob)
Attachment #569392 - Flags: review?(bjacob)
Attachment #569392 - Flags: review?(bjacob) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/d9da4de6579c
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)
(Assignee)

Comment 20

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

Fixed memory leaks.
Attachment #569392 - Attachment is obsolete: true
Attachment #569994 - Flags: review?(bjacob)
(Assignee)

Comment 21

6 years ago
Running on Try: https://tbpl.mozilla.org/?tree=Try&rev=20a50709f71c
(Assignee)

Comment 22

6 years ago
More recent try push: https://tbpl.mozilla.org/?tree=Try&rev=69bb43191112
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.
Attachment #569994 - Flags: review?(bjacob) → review+
(Assignee)

Updated

6 years ago
Blocks: 697211
(Assignee)

Comment 24

6 years ago
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.
Attachment #569994 - Attachment is obsolete: true
Attachment #570912 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/7da669483434
https://hg.mozilla.org/mozilla-central/rev/7da669483434
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.