1030408 - WebGLFramebuffer.cpp DetachTexture and DetachRenderbuffer not using loop index

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Walter Litwinczyk [:walter]]

In the function WebGLFramebuffer::DetachTexture() the loop index is not being added to the LOCAL_GL_COLOR_ATTACHMENT0 enum when detaching from the framebuffer:

http://dxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLFramebuffer.cpp#545

Similarly in WebGLFramebuffer::DetachRenderbuffer() the loop index is not being used on mColorAttachments, and not being used with LOCAL_GL_COLOR_ATTACHMENT0

http://dxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLFramebuffer.cpp#563

http://dxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLFramebuffer.cpp#564

Comment 1

[Walter Litwinczyk [:walter]]

Attachment: webgl_framebuffer_fix_missing_loop_index

Comment 2

[Kelsey Gilbert [:jgilbert]]

This'll need backports to everything 24+.

Comment 3

[Kelsey Gilbert [:jgilbert]]

I'm pretty sure you'd only have issues with this if the machine supports MRTs.

Comment 4

[Kelsey Gilbert [:jgilbert]]

This also affects WebGL2 stuff, naturally.

Comment 5

[Walter Litwinczyk [:walter]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=6f7c804e65d1

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/59613bf7b6d8

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/59613bf7b6d8

Comment 8

[Sylvestre Ledru [:Sylvestre]]

ESR is for security issues. 
Walter, could you fill an uplift request for beta? Thanks

Comment 9

[Walter Litwinczyk [:walter]]

Comment on attachment 8446191 [details] [diff] [review]
webgl_framebuffer_fix_missing_loop_index

Approval Request Comment
[Feature/regressing bug #]:
  843667
[User impact if declined]:
  Multiple render targets won't work correctly in WebGL.
[Describe test coverage new/current, TBPL]:
  All tests pass
  https://tbpl.mozilla.org/?tree=Try&rev=6f7c804e65d1
[Risks and why]:
  The loop does not detach the proper texture object from the framebuffer, leading to other bugs in the future.
[String/UUID change made/needed]:
  None

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8446191 [details] [diff] [review]
webgl_framebuffer_fix_missing_loop_index

Also approving the patch for aurora.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8446191 [details] [diff] [review]
webgl_framebuffer_fix_missing_loop_index

Actually, I am not sure to see the impact of this. Jeff, could you explain why you think we should have this fixed in 31 & 32 ? Thanks

Comment 12

[Kelsey Gilbert [:jgilbert]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #11)
> Comment on attachment 8446191 [details] [diff] [review]
> webgl_framebuffer_fix_missing_loop_index
> 
> Actually, I am not sure to see the impact of this. Jeff, could you explain
> why you think we should have this fixed in 31 & 32 ? Thanks

It's a bug where it's probably more risky *not* to take the fix than it is to take the fix. It's low-risk all around, but I think we should take this on at the very least Aurora.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/42b898f4f7aa
https://hg.mozilla.org/releases/mozilla-beta/rev/ae4eb0a59cc5

If you think this is bad enough to warrant landing on the B2G release branches, you should request blocking status with an explanation (including impact) for why.

Comment 14

[Kelsey Gilbert [:jgilbert]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #13)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/42b898f4f7aa
> https://hg.mozilla.org/releases/mozilla-beta/rev/ae4eb0a59cc5
> 
> If you think this is bad enough to warrant landing on the B2G release
> branches, you should request blocking status with an explanation (including
> impact) for why.

I don't think it's blocking, but it's worth trying to put anywhere that's low enough friction. We shouldn't respin a release for it. (maybe B2G release branches have criteria different from moz-release?)

The reason to take this patch is that it fixes an obvious bug.
The reason not to take it would be if the branch in question values stability more than minor functionality failures. (moz-release, at least)

Not taking this bug means we might get incorrect behavior. If stability is more important to a branch than correct behavior, this shouldn't land there.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Lawrence, is this something you'd want to see on v1.4 still based on comments 9, 12, and 14?

Comment 16

[u279076]

Marking this [qa-] since this appears to be low risk. If you have some ideas how QA might test this, particularly sites which use multiple WebGL targets, please let us know.

Comment 17

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #15)
> Lawrence, is this something you'd want to see on v1.4 still based on
> comments 9, 12, and 14?

1.4 decisions are being driven by Ivan and Steven. I have nomed this for 1.4? for their input.

Triage - Please see the comments that Ryan referenced above for a description of the issue. To be clear, I am not advocating for this bug, which afaik has not been requested by our partner for 1.4, but wanted to bring this to the attention of triage.

Comment 18

[Ivan Tsay (:ITsay)]

Hi Walter,

Mind if you can provide more information on user impact if we don't take this in v1.4

Comment 19

[Kelsey Gilbert [:jgilbert]]

(In reply to Ivan Tsay (:ITsay) from comment #18)
> Hi Walter,
> 
> Mind if you can provide more information on user impact if we don't take
> this in v1.4

If a user visits a page/uses an app which uses WebGL MRT support, it might not behave properly.

MRT support is pretty important for sophisticated WebGL, but it's not available everywhere. I don't know how many existing B2G devices even support it.

If a device doesn't support MRTs, there would be no change from taking this cset.

Comment 20

[Wayne Chang [:wchang]]

Given above comment we will not take on 1.4 given the target market for dolphin. Kicking it over to 2.0?

Thanks

Comment 21

[bhavana bajaj [:bajaj]]

(In reply to Wayne Chang [:wchang] from comment #20)
> Given above comment we will not take on 1.4 given the target market for
> dolphin. Kicking it over to 2.0?
> 
> Thanks

This could ride the trains for B2g given comment #14.