Early detachShader causes getAttribLocation to fail

RESOLVED FIXED in mozilla34

Status

()

RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: jmorton, Assigned: wlitwinczyk)

Tracking

Trunk
mozilla34
x86_64
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 8456397 [details]
webgl-early-shader-detach.html

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
(Assignee)

Comment 2

5 years ago
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
(Reporter)

Updated

4 years ago
Blocks: 1010371
(Assignee)

Comment 3

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

Comment 4

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

Comment 7

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

Comment 8

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

Comment 9

4 years ago
Created attachment 8467417 [details] [diff] [review]
detach_shader_patch

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+
(Assignee)

Comment 11

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

Comment 14

4 years ago
Created attachment 8470101 [details] [diff] [review]
detach_shader_patch

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)
(Assignee)

Comment 15

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

Comment 16

4 years ago
Created attachment 8471116 [details] [diff] [review]
detach_shader_patch

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+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1a5c05669cda
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.