937627 - The webgl constructs cache should be per context, not per document global

Status: RESOLVED FIXED

(DevTools Graveyard :: WebGL Shader Editor, defect, P2)

Description

[Victor Porof [:vporof][:vp]]

Because a document may have multiple webgl contexts.

Currently, no boundaries are crossed and information isn't leaked between contexts, but it's very hard to manage this data and remove it when things are destroyed.

Furthermore, if/when we'll need to track more fine grained context state, the current approach is very inefficient.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Great. Having separate caches per context, or at least per-origin, will make this easier to scale up without risking to introduce cross-origin information leakage bugs.

Comment 4

[Victor Porof [:vporof][:vp]]

Attachment: webgl-cache.patch

This is a small-ish refactoring on how things are cached, making sure contexts have individual state stores, which are cleared when their owner windows get destroyed. The functionality is verified by existing tests.

Benoit, feel free to take a look at this as well if you have the time.

Comment 5

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 832102 [details] [diff] [review]
webgl-cache.patch

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

the code changes look lovely to me.

::: toolkit/devtools/server/actors/webgl.js
@@ +857,5 @@
>     * @param WebGLUniformLocation value
>     *        The uniform value.
>     */
> +  addUniform: function(program, name, value) {
> +    this._programs.get(program).uniforms.set(new XPCNativeWrapper(value), {

oog.

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 832102 [details] [diff] [review]
webgl-cache.patch

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

Feedback+ to the idea of per-context cache.

Comment 7

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/d8173efa5e8c

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/d8173efa5e8c

Comment 9

[Victor Porof [:vporof][:vp]]

Comment on attachment 832102 [details] [diff] [review]
webgl-cache.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature (bug 910953)
User impact if declined: No front-facing user impact, this is a small internal refactoring that changes the caching mechanism to be more precise. Fixing this bug will make it easier to uplift other patches that depend on the changes here. For example, bug 938549, for which I'll ask for aurora approval shortly.
Testing completed (on m-c, etc.): fx-team, inbound, m-c
Risk to taking this patch (and alternatives if risky): None
String or IDL/UUID changes made by this patch: None

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a231b9eade56