Closed Bug 1038899 Opened 6 years ago Closed 5 years ago

Early detachShader causes getAttribLocation to fail

Categories

(Core :: Canvas: WebGL, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: jmorton, Assigned: wlitwinczyk)

References

Details

Attachments

(2 files, 3 obsolete files)

There is a behavior change in nightly right now which doesn't allow one to call detachShader right after program link (and continue to use the program). In OpenGL at least, this is valid, but the WebGL and GLES specs weren't clear to me on this.

Detaching early now causes getAttribLocation calls to fail, but getUniform* still seems to work.

I've attached a test case which throws an opengl error on Nightly, but not on Aurora and later or on chrome.

Calling deleteShader early does still work.

This may have been caused by the ANGLE update - bug 1010371.
Walter,

Are you able to take a lot and verify that this might have been introduced by the ANGLE update?
Assignee: nobody → wlitwinczyk
It appears to have been introduced by the angle update:

Broken revision:
https://hg.mozilla.org/mozilla-central/rev/401bc7471c24

Working revision:
https://hg.mozilla.org/mozilla-central/rev/0c408ede302a
Blocks: 1010371
The issue is actually present in the 'working' revision. It only works by coincidence. The problem is with the lazily created maps for the identifiers: 

http://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLProgram.cpp?from=MapIdentifier#138
http://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLProgram.cpp?from=MapIdentifier#183

When detachShader() is called it removes the shader from the mAttachedShaders member variable in WebGLProgram:

http://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLContextGL.cpp#773

Then when GetAttribLocation and friends are called it tries to initialize the map, but there are no elements in the mAttachedShaders array and so nothing is added to the map. It then falls through to the bottom and returns the input name as the mappedName:

http://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLProgram.cpp?from=MapIdentifier#178

This worked because in the previous ANGLE version it did nothing special with the variable names, the mapped name and input name were the same:

http://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLContextGL.cpp?from=CompileShader&case=true#3261

Which then worked when calling the fGetAttrib... methods. The updated ANGLE version actually changes the variable names and the mapped name is something like "webgl_f238fca840224". Now when the user calls GetAttribLocation() with the variable they have e.g. "position" the map function returns "position" due to the above problem. Next "position" is passed to the fGetAttrib... call and returns -1 because the variable is actually named webgl_XXXXX.
Attached patch detach_shader_patch (obsolete) — Splinter Review
Marking vlad as reviewer because he made the original change.

I am unsure of what the HashMapIdentifier() function was added for. I am also thinking about moving the calls to the Initialize/Clear map functions to SetLinkStatus() as these should probably happen when a program becomes valid/invalid. But on the other side, SetLinkStatus is called multiple times in WebGLContext::LinkProgram().
Attachment #8467323 - Flags: review?(vladimir)
Attachment #8467323 - Flags: feedback?(jgilbert)
Comment on attachment 8467323 [details] [diff] [review]
detach_shader_patch

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

::: dom/canvas/WebGLProgram.h
@@ +81,5 @@
>      /* Getters for cached program info */
>      bool IsAttribInUse(unsigned i) const { return mAttribsInUse[i]; }
>  
> +    /* Initialize the maps used by MapIdentifier and ReverseMapIdentifier. This should
> +     * be called after the program has been successfully linked.

"after each time" or "after the first time"?
Attachment #8467323 - Flags: feedback?(jgilbert) → feedback+
(In reply to Walter Litwinczyk [:walter] from comment #4)
> Created attachment 8467323 [details] [diff] [review]
> detach_shader_patch
> 
> Marking vlad as reviewer because he made the original change.
> 
> I am unsure of what the HashMapIdentifier() function was added for. I am
> also thinking about moving the calls to the Initialize/Clear map functions
> to SetLinkStatus() as these should probably happen when a program becomes
> valid/invalid. But on the other side, SetLinkStatus is called multiple times
> in WebGLContext::LinkProgram().

SetLinkStatus might need to be called more than once if we need its result to make changes (bindAttribLoc?) before relinking. After relinking, we'll need to update the info again.
Well I answered my (In reply to Walter Litwinczyk [:walter] from comment #4)
> Created attachment 8467323 [details] [diff] [review]
> detach_shader_patch
>
> I am unsure of what the HashMapIdentifier() function was added for.

Well I'm answering my own question. It's because BindAttribLocation() can be called before linking (well the only useful time)... whoops, need to add it back.
(In reply to Jeff Gilbert [:jgilbert] from comment #5)
> Comment on attachment 8467323 [details] [diff] [review]
> detach_shader_patch
> 
> Review of attachment 8467323 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLProgram.h
> @@ +81,5 @@
> >      /* Getters for cached program info */
> >      bool IsAttribInUse(unsigned i) const { return mAttribsInUse[i]; }
> >  
> > +    /* Initialize the maps used by MapIdentifier and ReverseMapIdentifier. This should
> > +     * be called after the program has been successfully linked.
> 
> "after each time" or "after the first time"?

It should be after every time it's linked, as the map needs to cleared/updated with the new attribute and uniform names.
Attached patch detach_shader_patch (obsolete) — Splinter Review
Added back the HashMapIdentifier() function
Attachment #8467323 - Attachment is obsolete: true
Attachment #8467323 - Flags: review?(vladimir)
Attachment #8467417 - Flags: review?(vladimir)
Comment on attachment 8467417 [details] [diff] [review]
detach_shader_patch

Looks good to me, but you should get Jeff or Dan to glance at it as well as they own WebGL :)
Attachment #8467417 - Flags: review?(vladimir)
Attachment #8467417 - Flags: review?(jgilbert)
Attachment #8467417 - Flags: review+
Attachment #8467417 - Flags: review?(jgilbert) → review+
Try passes: https://tbpl.mozilla.org/?tree=Try&rev=6e1685bb164e (a little ugly, but try has been being iffy lately)
Keywords: checkin-needed
This had to be backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d1865a8e16d3 for mochitest-2 failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=45490202&tree=Mozilla-Inbound


There was also an e10s-mochitest-3 failure that started on the same checkin-needed push https://tbpl.mozilla.org/php/getParsedLog.php?id=45489853&tree=Mozilla-Inbound that I think was also caused by this patch.
Flags: needinfo?(wlitwinczyk)
Flags: needinfo?(jgilbert)
Attached patch detach_shader_patch (obsolete) — Splinter Review
Yeah, those are caused by this patch. There was a code path that should have hit one of the MOZ_ASSERTS, but for some reason didn't. Anyway it is a bug in the patch, the mIdentifierMap/mIdentifierReverse maps were still null when they shouldn't have been if the user calls one of the GetAttrib/Uniform methods before the first linking of the program. Fixed by creating the maps when the WebGLProgram object is created.

Thanks for the catch!

Try: https://tbpl.mozilla.org/?tree=Try&rev=54964270780c
Attachment #8467417 - Attachment is obsolete: true
Attachment #8470101 - Flags: review?(jgilbert)
Flags: needinfo?(wlitwinczyk)
Comment on attachment 8470101 [details] [diff] [review]
detach_shader_patch

Ok, wow, so I completely messed up this patch. The new try failure led me to:

http://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLContextValidate.cpp?from=UpdateInfo#161

Which does all kinds of things I was unaware of. So I'm going to have to completely redo this patch.
Attachment #8470101 - Flags: review?(jgilbert)
Try: https://tbpl.mozilla.org/?tree=Try&rev=a1c134e2c683

This patch centralizes the map initialization into one place inside of UpdateInfo()

(Also clearing the needinfo for jgilbert)
Attachment #8470101 - Attachment is obsolete: true
Attachment #8471116 - Flags: review?(jgilbert)
Flags: needinfo?(jgilbert)
Attachment #8471116 - Flags: review?(jgilbert) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1a5c05669cda
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.