1038899 - Early detachShader causes getAttribLocation to fail

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Jon Morton [:jmorton]]

Attachment: 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.

Comment 1

[u480271]

Walter,

Are you able to take a lot and verify that this might have been introduced by the ANGLE update?

Comment 2

[Walter Litwinczyk [:walter]]

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

Comment 3

[Walter Litwinczyk [:walter]]

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.

Comment 4

[Walter Litwinczyk [:walter]]

Attachment: 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().

Comment 5

[Kelsey Gilbert [: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"?

Comment 6

[Kelsey Gilbert [:jgilbert]]

(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.

Comment 7

[Walter Litwinczyk [:walter]]

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.

Comment 8

[Walter Litwinczyk [:walter]]

(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.

Comment 9

[Walter Litwinczyk [:walter]]

Attachment: detach_shader_patch

Added back the HashMapIdentifier() function

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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 :)

Comment 11

[Walter Litwinczyk [:walter]]

Try passes: https://tbpl.mozilla.org/?tree=Try&rev=6e1685bb164e (a little ugly, but try has been being iffy lately)

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2422647220de

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if 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.

Comment 14

[Walter Litwinczyk [:walter]]

Attachment: 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

Comment 15

[Walter Litwinczyk [:walter]]

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.

Comment 16

[Walter Litwinczyk [:walter]]

Attachment: 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)

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a5c05669cda

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a5c05669cda