Closed Bug 937627 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect

Tracking

(firefox26 unaffected, firefox27 fixed, firefox28 fixed)

RESOLVED FIXED
Firefox 28
Tracking Status
firefox26 --- unaffected
firefox27 --- fixed
firefox28 --- fixed

People

(Reporter: vporof, Assigned: vporof)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file)

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.
Duplicate of this bug: 936893
Duplicate of this bug: 936894
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.
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.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attachment #832102 - Flags: review?(rcampbell)
Attachment #832102 - Flags: feedback?(bjacob)
Priority: -- → P2
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.
Attachment #832102 - Flags: review?(rcampbell) → review+
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.
Attachment #832102 - Flags: feedback?(bjacob) → feedback+
https://hg.mozilla.org/mozilla-central/rev/d8173efa5e8c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
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
Attachment #832102 - Flags: approval-mozilla-aurora?
Blocks: 938549
Attachment #832102 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [qa-]
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.