Last Comment Bug 728029 - Preserve added JS properties on the JS object returned by webgl.getExtension
: Preserve added JS properties on the JS object returned by webgl.getExtension
Status: RESOLVED FIXED
webgl-conformance
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-16 14:54 PST by Benoit Jacob [:bjacob] (mostly away)
Modified: 2012-05-28 13:37 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
preserve wrappers for webgl extension objects (8.61 KB, patch)
2012-03-06 15:47 PST, Benoit Jacob [:bjacob] (mostly away)
bobbyholley: feedback+
continuation: feedback+
Details | Diff | Review
preserve wrappers for webgl extension objects (10.20 KB, patch)
2012-03-06 20:26 PST, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Review
preserve wrappers for webgl extension objects, v2 (8.36 KB, patch)
2012-03-07 11:26 PST, Benoit Jacob [:bjacob] (mostly away)
peterv: review-
Details | Diff | Review
preserve wrappers for webgl extension objects, v3 (7.86 KB, patch)
2012-03-08 15:47 PST, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Review
preserve wrappers for webgl extension objects, v4 (7.58 KB, patch)
2012-03-08 20:49 PST, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Review
preserve wrappers for webgl extension objects, v5 (7.59 KB, patch)
2012-03-08 21:04 PST, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Review
preserve wrappers for webgl extension objects, v6 (7.67 KB, patch)
2012-03-08 21:27 PST, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Review
preserve wrappers for webgl extension objects, v7 (7.70 KB, patch)
2012-03-09 05:54 PST, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Review
preserve wrappers for webgl extension objects, v8 (9.09 KB, patch)
2012-03-09 06:19 PST, Benoit Jacob [:bjacob] (mostly away)
peterv: review-
Details | Diff | Review
drop patch removing the uniqueObjectTest from the WebGL conformance test suite (4.21 KB, patch)
2012-03-09 08:32 PST, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Review
preserve wrappers for webgl extension objects, v9 (10.91 KB, patch)
2012-03-13 16:00 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Review
preserve wrappers for webgl extension objects, v10 (19.76 KB, patch)
2012-03-19 05:05 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Review
preserve wrappers for webgl extension objects, v11 (19.05 KB, patch)
2012-03-19 05:55 PDT, Benoit Jacob [:bjacob] (mostly away)
peterv: review+
Details | Diff | Review
add tests creating reference cycles between WebGLContext and WebGLExtension (and subclass) (3.70 KB, patch)
2012-03-19 06:05 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Review
add tests creating reference cycles between WebGLContext and WebGLExtension (and subclass) (3.69 KB, patch)
2012-03-19 06:09 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Review

Description Benoit Jacob [:bjacob] (mostly away) 2012-02-16 14:54:01 PST
The WebGL getExtension function returns a unique WebGLExtension object for a given extension name. It's really unique so that calling getExtension twice with the same name, will return twice the same object.

While that is easy to implement at the C++ level, honoring this requirement at the JS level is a little trickier: the conformance tests do this:

    gl.getExtension("OES_texture_float").myProperty = 2;
    // ... tries to trigger the GC ...
    shouldBe('gl.getExtension("OES_texture_float").myProperty', '2');

The question is how can I use the JSAPI to make that work?

Blake gave a tip in bug 630672 comment 22 but a little more help would be very welcome! This is really important for us as it's the last nontrivial thing for WebGL conformance (WebGL 1.0.1 due 6 weeks from now).
Comment 1 Andrew McCreight [:mccr8] 2012-02-16 15:18:58 PST
How is the WebGLExtension object implemented?
Comment 2 :Ms2ger 2012-02-16 15:20:54 PST
nsIWebGLExtension, I guess. This probably "just" needs to implement nsWrapperCache?
Comment 3 Andrew McCreight [:mccr8] 2012-02-16 15:22:31 PST
Yes, that's what it sounds like.
Comment 4 Andrew McCreight [:mccr8] 2012-02-16 15:25:31 PST
Looking at the GetExtension function ( http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLContext.cpp#970 ) , there are three separate classes that will have to be made into wrapper caches: WebGLExtensionStandardDerivatives, WebGLExtensionLoseContext, WebGLExtension.
Comment 5 Andrew McCreight [:mccr8] 2012-02-16 15:30:54 PST
In addition to making them implement nsWrapperCache, you have to turn them into cycle collected classes, and tell the CC about their wrappers.  That should be a fairly boiler plate change to make.  I assume that's what Blake was referring to by "and some other small things to get right to avoid leaks", but maybe there's more.
Comment 6 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-02-17 04:52:05 PST
So, I just landed a bunch of patches with this bug number instead of bug 727680. Sorry about that...
Comment 7 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-02-17 06:16:27 PST
In addition to implementing nsWrapperCache and making sure that your CC stuff is sorted out, you'll also need to have a scriptable helper in nsDOMClassInfo that calls nsContentUtils::PreserveWrapper for the AddProperty hook.
Comment 8 Bobby Holley (PTO through June 13) 2012-02-17 09:15:43 PST
(In reply to Blake Kaplan (:mrbkap) from comment #7)
> In addition to implementing nsWrapperCache and making sure that your CC
> stuff is sorted out, you'll also need to have a scriptable helper in
> nsDOMClassInfo that calls nsContentUtils::PreserveWrapper for the
> AddProperty hook.

Won't they also need a PreCreate hook? Otherwise, the primary wrapper will live in whatever compartment first accesses it. Could this be a problem?
Comment 9 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-02-17 09:37:24 PST
Oops, yes. The PreCreate hook needs to set *parentObj to the windows with which the extension object is associated with.
Comment 10 Benoit Jacob [:bjacob] (mostly away) 2012-02-21 07:20:25 PST
Hey, thanks a lot for all the helpful replies. But this is pretty technical and it would help a lot to have some example code. Is there any existing code in particular that I should look at and follow as a model?
Comment 11 Bobby Holley (PTO through June 13) 2012-02-21 10:29:42 PST
(In reply to Benoit Jacob [:bjacob] from comment #10)
> Hey, thanks a lot for all the helpful replies. But this is pretty technical
> and it would help a lot to have some example code. Is there any existing
> code in particular that I should look at and follow as a model?

For PreCreate, check out nsNavigatorSH::PreCreate in nsDOMClassInfo.cpp.

For the call to PreserveWrapper, check out nsEventTargetSH::AddProperty in nsDOMClassInfo.cpp.

For the nsWrapperCache stuff, just mxr for the various classes that implement nsWrapperCache.

For the CC stuff, talk to mccr8. ;-)
Comment 12 Benoit Jacob [:bjacob] (mostly away) 2012-02-27 08:23:15 PST
For the PreserveWrapper stuff, can you confirm that this is OK:

NS_IMETHODIMP
WebGLExtensionTextureFilterAnisotropic::AddProperty(nsIXPConnectWrappedNative *wrapper, JSContext *cx,
                             JSObject *obj, jsid id, jsval *vp, bool *_retval)
{
  if (id == sAddEventListener_id) {
    return NS_OK;
  }

  WebGLExtensionTextureFilterAnisotropic::PreserveWrapper(GetNative(wrapper, obj));

  return NS_OK;
}

void
WebGLExtensionTextureFilterAnisotropic::PreserveWrapper(nsISupports *aNative)
{
  nsIWebGLExtensionTextureFilterAnisotropic *ext = static_cast<nsIWebGLExtensionTextureFilterAnisotropic*>(aNative);
  nsContentUtils::PreserveWrapper(aNative, ext);
}



***

For the PreCreate stuff, I'm stuck. The nsNavigatorSH::PreCreate implementation does things that seem specific to their case, that I can't adapt to my case in a straightforward way:

  nsCOMPtr<nsIDOMNavigator> safeNav(do_QueryInterface(nativeObj));
  Navigator *nav = static_cast<Navigator*>(safeNav.get());
  nsGlobalWindow *win = static_cast<nsGlobalWindow*>(nav->GetWindow());
  JSObject *global = win->GetGlobalJSObject();

What should I do in my case?
Comment 13 Benoit Jacob [:bjacob] (mostly away) 2012-02-27 08:33:03 PST
OK, can you explain a bit more what PreCreate does? Is it called before or after the WebGLExtension c++ object is created?
Comment 14 Peter Van der Beken [:peterv] 2012-02-27 08:35:46 PST
(In reply to Benoit Jacob [:bjacob] from comment #12)
> For the PreserveWrapper stuff, can you confirm that this is OK:
> 
> NS_IMETHODIMP
> WebGLExtensionTextureFilterAnisotropic::
> AddProperty(nsIXPConnectWrappedNative *wrapper, JSContext *cx,
>                              JSObject *obj, jsid id, jsval *vp, bool
> *_retval)

Why are you adding this to WebGLExtensionTextureFilterAnisotropic instead of to a scriptable helper in nsDOMClassInfo.cpp?

> What should I do in my case?

You need a way to get from a WebGLExtensionTextureFilterAnisotropic to the window that it's associated with.

(In reply to Benoit Jacob [:bjacob] from comment #13)
> OK, can you explain a bit more what PreCreate does? Is it called before or
> after the WebGLExtension c++ object is created?

It's called just before a JS wrapper is created. You can't create a JS wrapper if the C++ object doesn't exist ;-).
Comment 15 Bobby Holley (PTO through June 13) 2012-02-28 12:42:29 PST
From IRC:

1 - Implement nsWrapperCache in the native object you want preserved.

2 - Create a scriptable helper for your object in nsDOMClassinfo.cpp

3 - Use a PreCreate hook on the scriptable helper to force your object to be parented to the window it belongs to, so that there's a one-to-one correspondence between XPConnect wrappers and native objects.

4 - Use an AddProperty hook on the scriptable helper to call nsContentUtils::PreserveWrapper when somebody sticks an expando on the object.

5 - Have someone who understands the CC implication have a look. :-)
Comment 16 Benoit Jacob [:bjacob] (mostly away) 2012-03-06 15:47:39 PST
Created attachment 603497 [details] [diff] [review]
preserve wrappers for webgl extension objects

Not quite ready for review, but it works, and needs feedback from some of you:  please check that it does what it's supposed to.

Here is a testcase, too:
http://people.mozilla.org/~bjacob/webgl-extension-jsobject.html

You should see a dialog inviting you to GC (go to about:memory), then press OK, it should say "My value was preserved!". If this doesn't work, it will say "undefined".

My problem is that I can't test myself: for some mysterious reason, on my different machines, it says "My value was preserved!" even without the patch. But on other peoples' machines, without the patch, it does say "undefined".
Comment 17 Benoit Jacob [:bjacob] (mostly away) 2012-03-06 15:49:54 PST
https://tbpl.mozilla.org/?tree=Try&rev=20946e352a96
Comment 18 Andrew McCreight [:mccr8] 2012-03-06 15:54:11 PST
Comment on attachment 603497 [details] [diff] [review]
preserve wrappers for webgl extension objects

The CC macro gunk looks reasonable to me.
Comment 19 Bobby Holley (PTO through June 13) 2012-03-06 16:26:54 PST
Comment on attachment 603497 [details] [diff] [review]
preserve wrappers for webgl extension objects

>+WebGLExtensionSH::AddProperty(nsIXPConnectWrappedNative *wrapper, JSContext *cx,
>+                              JSObject *obj, jsid id, jsval *vp, bool *_retval)
>+{
>+  if (id == sAddEventListener_id) {
>+    return NS_OK;
>+  }

Presumably, you cribbed this from nsEventTargetSH::AddProperty. But it makes no sense here.

>+nsresult
>+WebGLExtensionSH::PreCreate(nsISupports *nativeObj, JSContext *cx,
>+                            JSObject *globalObj, JSObject **parentObj)
>+{
>+  // MONKEY ALERT
>+  // i have no idea why I'm doing this to *parentObj. Other PreCreate funcs seemed to be doing that.
>+  *parentObj = globalObj;

Well, this function is all about selecting a parentObj. So the intent is that it's a default, in case you return early without choosing anything else. I don't think it matters much, because XPConnect always calls PreCreate with (native, cx, parent, &parent). But it can't hurt.

Otherwise, the classinfo stuff looks reasonable.
Comment 20 Benoit Jacob [:bjacob] (mostly away) 2012-03-06 20:26:19 PST
Created attachment 603588 [details] [diff] [review]
preserve wrappers for webgl extension objects

Thanks for the feedback, asking for review now. Incorporated feedback from bholley and added a license header for the new file.
Comment 21 Andrew McCreight [:mccr8] 2012-03-06 20:31:29 PST
You should use MPL2 for new files, I believe.
Comment 22 Bobby Holley (PTO through June 13) 2012-03-06 20:52:56 PST
I'm a bit wary about the mysterious anomaly in comment 16. Can you investigate it some more?

What's the idea behind nsIWebGLExtensionWithWrapperCache? Why not implement nsWrapperCache in your concrete class (WebGLExtension)?

Also, I think that peterv, jst, or someone similar should probably do the final review here.
Comment 23 Benoit Jacob [:bjacob] (mostly away) 2012-03-07 11:26:21 PST
Created attachment 603799 [details] [diff] [review]
preserve wrappers for webgl extension objects, v2

Here you go. That intermediate nsIWebGLExtensionWithWrapperCache class is no more: it was just a remnant from me trying to follow what seemed like a path of least resistance when I had trouble including WebGLContext.h from nsDOMClassInfo.cpp. In the end, just #undef None (presumably defined by Xlib headers, presumably included by some header included there) is enough to make #include "WebGLContext.h" work there.
Comment 24 Benoit Jacob [:bjacob] (mostly away) 2012-03-07 12:26:12 PST
https://tbpl.mozilla.org/?tree=Try&rev=52fe22cd2e6c

Peter, just so you know: this review is relatively time-sensitive. The hard deadline is that this must be at least in Nightly when WebGL 1.0.1 gets published, around March 31st. Indeed, all browser vendors plan to announce 100% WebGL conformance simultaneously.

It would be better, though, to have this landed a bit of time ahead of that date, and even better to have this in Firefox 13 (we could request aurora approval if we can't land by next week, but it's easiest if we can land before the uplift).
Comment 25 Bobby Holley (PTO through June 13) 2012-03-07 13:10:33 PST
(also, it should be quick to review - I've already given it a once-over).
Comment 26 Peter Van der Beken [:peterv] 2012-03-08 02:28:49 PST
Comment on attachment 603799 [details] [diff] [review]
preserve wrappers for webgl extension objects, v2

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

::: content/canvas/src/WebGLContext.h
@@ +2535,5 @@
>  };
>  
> +// needed for WebGLExtensionSH::PreCreate
> +#define WEBGLEXTENSION_IID \
> +   {0x02768e61, 0xb78f, 0x42d4, {0x98, 0x19, 0xb5, 0xf9, 0x54, 0x52, 0x31, 0x64}}

I don't see this being used in WebGLExtensionSH::PreCreate.

::: dom/base/nsDOMClassInfo.cpp
@@ +10704,5 @@
> +    // Oops, this wasn't really a nsIWebGLExtensionTextureFilterAnisotropic object. This can happen if someone
> +    // tries to use our scriptable helper as a real object and tries to wrap
> +    // it, see bug 319296.
> +    return NS_OK;
> +  }

You don't need to do this anymore, bug 319296 trunk fix made sure this doesn't happen.

@@ +10710,5 @@
> +  WebGLExtension *webglext = static_cast<WebGLExtension*>(iwebglext.get());
> +
> +  WebGLContext *webgl = webglext->Context();
> +  nsIDOMHTMLCanvasElement *canvas;
> +  webgl->GetCanvas(&canvas);

This leaks canvas.

@@ +10712,5 @@
> +  WebGLContext *webgl = webglext->Context();
> +  nsIDOMHTMLCanvasElement *canvas;
> +  webgl->GetCanvas(&canvas);
> +  
> +  nsCOMPtr<nsIContent> content = do_QueryObject(canvas); 

Is there a reason to not parent this to the canvas element itself instead of the window?
Comment 27 Benoit Jacob [:bjacob] (mostly away) 2012-03-08 15:47:36 PST
Created attachment 604237 [details] [diff] [review]
preserve wrappers for webgl extension objects, v3

Thanks for the quick review! updated patch attached.

(In reply to Peter Van der Beken [:peterv] from comment #26)
> Comment on attachment 603799 [details] [diff] [review]
> preserve wrappers for webgl extension objects, v2
> 
> Review of attachment 603799 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContext.h
> @@ +2535,5 @@
> >  };
> >  
> > +// needed for WebGLExtensionSH::PreCreate
> > +#define WEBGLEXTENSION_IID \
> > +   {0x02768e61, 0xb78f, 0x42d4, {0x98, 0x19, 0xb5, 0xf9, 0x54, 0x52, 0x31, 0x64}}
> 
> I don't see this being used in WebGLExtensionSH::PreCreate.

Right, I don't know why I believed this. Removed.


> 
> ::: dom/base/nsDOMClassInfo.cpp
> @@ +10704,5 @@
> > +    // Oops, this wasn't really a nsIWebGLExtensionTextureFilterAnisotropic object. This can happen if someone
> > +    // tries to use our scriptable helper as a real object and tries to wrap
> > +    // it, see bug 319296.
> > +    return NS_OK;
> > +  }
> 
> You don't need to do this anymore, bug 319296 trunk fix made sure this
> doesn't happen.

Removed.

> 
> @@ +10710,5 @@
> > +  WebGLExtension *webglext = static_cast<WebGLExtension*>(iwebglext.get());
> > +
> > +  WebGLContext *webgl = webglext->Context();
> > +  nsIDOMHTMLCanvasElement *canvas;
> > +  webgl->GetCanvas(&canvas);
> 
> This leaks canvas.

Added a NS_RELEASE to compensate the NS_IF_ADDREF that GetCanvas does.

> 
> @@ +10712,5 @@
> > +  WebGLContext *webgl = webglext->Context();
> > +  nsIDOMHTMLCanvasElement *canvas;
> > +  webgl->GetCanvas(&canvas);
> > +  
> > +  nsCOMPtr<nsIContent> content = do_QueryObject(canvas); 
> 
> Is there a reason to not parent this to the canvas element itself instead of
> the window?

I didn't understand this comment. Did you mean to apply it to another part of the patch? I need a window just below this code to be able to call GetGlobalJSObject on it. Is there another way to acheve the same? I just need to know what to set *parentObj to.
Comment 28 Bobby Holley (PTO through June 13) 2012-03-08 15:57:03 PST
(In reply to Benoit Jacob [:bjacob] from comment #27)
> > @@ +10710,5 @@
> > > +  WebGLExtension *webglext = static_cast<WebGLExtension*>(iwebglext.get());
> > > +
> > > +  WebGLContext *webgl = webglext->Context();
> > > +  nsIDOMHTMLCanvasElement *canvas;
> > > +  webgl->GetCanvas(&canvas);
> > 
> > This leaks canvas.
> 
> Added a NS_RELEASE to compensate the NS_IF_ADDREF that GetCanvas does.
> 

That's error-prone. Just use an nsCOMPtr with getter_AddRefs

> > 
> > @@ +10712,5 @@
> > > +  WebGLContext *webgl = webglext->Context();
> > > +  nsIDOMHTMLCanvasElement *canvas;
> > > +  webgl->GetCanvas(&canvas);
> > > +  
> > > +  nsCOMPtr<nsIContent> content = do_QueryObject(canvas); 
> > 
> > Is there a reason to not parent this to the canvas element itself instead of
> > the window?
> 
> I didn't understand this comment. Did you mean to apply it to another part
> of the patch? I need a window just below this code to be able to call
> GetGlobalJSObject on it. Is there another way to acheve the same? I just
> need to know what to set *parentObj to.

So, your current code sets the parent to the JS wrapper of the window. Peter is saying that it would make more sense to parent it to the wrapped js object of the canvas. You can probably use WrapNativeParent to do this. Ping me on irc if you need help.
Comment 29 Benoit Jacob [:bjacob] (mostly away) 2012-03-08 20:49:34 PST
Created attachment 604297 [details] [diff] [review]
preserve wrappers for webgl extension objects, v4

Thanks Bobby! The function looks a lot simpler now. But I'm still in monkey mode, all I know is that this compiles.
Comment 30 Benoit Jacob [:bjacob] (mostly away) 2012-03-08 20:51:28 PST
https://tbpl.mozilla.org/?tree=Try&rev=bd68552713dc
Comment 31 Benoit Jacob [:bjacob] (mostly away) 2012-03-08 20:53:09 PST
*oops* I just realized that I'm not even using the canvas anymore... so this can't be right. OK, I need help please!
Comment 32 Benoit Jacob [:bjacob] (mostly away) 2012-03-08 21:04:03 PST
Created attachment 604299 [details] [diff] [review]
preserve wrappers for webgl extension objects, v5

Bobby got me out of here. Thanks again!

https://tbpl.mozilla.org/?tree=Try&rev=d639970383d4
Comment 33 Benoit Jacob [:bjacob] (mostly away) 2012-03-08 21:27:22 PST
Created attachment 604300 [details] [diff] [review]
preserve wrappers for webgl extension objects, v6

sorry, v5 didn't compile... thanks again to Bobby.
Comment 34 :Ms2ger 2012-03-09 02:23:52 PST
Comment on attachment 604300 [details] [diff] [review]
preserve wrappers for webgl extension objects, v6

>--- a/dom/base/nsDOMClassInfo.cpp
>+++ b/dom/base/nsDOMClassInfo.cpp
>+WebGLExtensionSH::PreCreate(nsISupports *nativeObj, JSContext *cx,
>+                            JSObject *globalObj, JSObject **parentObj)
>+{
>+  nsINode *node;
>+  nsresult rv = CallQueryInterface(domcanvas, &node);
>+  if (NS_FAILED(rv))
>+    return rv;

Looks like another leak here.
Comment 35 Peter Van der Beken [:peterv] 2012-03-09 04:01:27 PST
(In reply to Ms2ger from comment #34)
> Looks like another leak here.

Please use smart pointers (nsCOMPtr in this case) to avoid these errors.
Comment 36 :Ms2ger 2012-03-09 04:30:15 PST
Except for nsWrapperCache, which just exists to confuse you.
Comment 37 Benoit Jacob [:bjacob] (mostly away) 2012-03-09 05:54:17 PST
Created attachment 604387 [details] [diff] [review]
preserve wrappers for webgl extension objects, v7

Thanks for catching this!

This new patch assumes that CallQueryInterface addrefs only its second argument, not its first argument.

Also, I had to put this .get() on the first argument to get it to compile.
Comment 38 :Ms2ger 2012-03-09 06:02:35 PST
Comment on attachment 604387 [details] [diff] [review]
preserve wrappers for webgl extension objects, v7

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

::: dom/base/nsDOMClassInfo.cpp
@@ +10723,5 @@
> +  nsCOMPtr<nsIDOMHTMLCanvasElement> domcanvas;
> +  webgl->GetCanvas(getter_AddRefs(domcanvas));
> +
> +  nsCOMPtr<nsINode> node;
> +  nsresult rv = CallQueryInterface(domcanvas.get(), getter_AddRefs(node));

Actually, why don't you add

Element* GetCanvas() const
{
  return HTMLCanvasElement();
}

to WebGLContext?
Comment 39 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-03-09 06:07:38 PST
(In reply to Benoit Jacob [:bjacob] from comment #37)
> This new patch assumes that CallQueryInterface addrefs only its second
> argument, not its first argument.

This assumption is correct: In gecko, the general rule (as with all rules there are cases where it doesn't hold) is that in parameters (nsIInterface *foo) are strong references that do not need additional reference counting on the caller's end whereas out parameters (nsIInterface **foo) are returned as strong parameters that will need a release from the caller. 

> Also, I had to put this .get() on the first argument to get it to compile.

Yeah... So what you've written is correct, but it would be slightly clearer and more idiomatic to write:

nsCOMPtr<nsINode> node = do_QueryInterface(domcanvas);

That being said, Ms2ger's suggestion seems much cleaner to me.
Comment 40 Benoit Jacob [:bjacob] (mostly away) 2012-03-09 06:19:46 PST
Created attachment 604393 [details] [diff] [review]
preserve wrappers for webgl extension objects, v8

Here you go. I had to cast the canvas to nsINode because nsISupports is an ambiguous base of nsHTMLCanvasElement.
Comment 41 Benoit Jacob [:bjacob] (mostly away) 2012-03-09 08:32:29 PST
Created attachment 604420 [details] [diff] [review]
drop patch removing the uniqueObjectTest from the WebGL conformance test suite

no review, this just removes a custom patch we had against the WebGL conformance test suite, which removed some sub-tests from it. Not needed anymore, we should consistently pass them now.
Comment 42 Peter Van der Beken [:peterv] 2012-03-13 07:48:51 PDT
Comment on attachment 604393 [details] [diff] [review]
preserve wrappers for webgl extension objects, v8

WebGLExtensionSH isn't actually used anywhere, you need to s/nsDOMGenericSH/WebGLExtensionSH/ in the NS_DEFINE_CLASSINFO_DATA for all WebGLExtensions.
WebGLContext needs to traverse mEnabledExtensions, and you need to add a testcase that adds the WebGLContext as a property on an extension, pretty sure that'll leak with this patch as-is since we're missing the edge from context to extension.
Comment 43 Benoit Jacob [:bjacob] (mostly away) 2012-03-13 15:48:31 PDT
Thanks for catching this. At some point, it did strike me that it was strange that I didn't know where WebGLExtenionSH was being used at all, but somehow I forgot about that.

Will add the testcase, but keep in mind that due to the strange phenomenon mentioned in comment 16, which looks like it might be a problem with conservative GC, I can't test locally.
Comment 44 Benoit Jacob [:bjacob] (mostly away) 2012-03-13 16:00:52 PDT
Created attachment 605580 [details] [diff] [review]
preserve wrappers for webgl extension objects, v9

Here's the patch tweaking NS_DEFINE_CLASSINFO_DATA to use WebGLExtensionSH.

I didn't know whether I had to do it only for WebGLExtension, or for all sub-classes. This patch does it for all. Is that useful?

Regarding your other review comment about the cycle, I checked, it turns out that WebGLExtension does NOT hold a strong reference to the corresponding WebGLContext. Instead, WebGLExtension inherits WebGLContextBoundObject, which only has a plain pointer (WebGLContext *mContext) to the corresponding WebGLContext.
Comment 45 Bobby Holley (PTO through June 13) 2012-03-13 23:08:15 PDT
(In reply to Benoit Jacob [:bjacob] (On vacation until March 18) from comment #43) 
> Will add the testcase, but keep in mind that due to the strange phenomenon
> mentioned in comment 16, which looks like it might be a problem with
> conservative GC, I can't test locally.

How does the conservative GC play into this? Are you referring to the stack scanner?

I think this needs to be investigated more carefully. It should be possible to construct a testcase that reproduces the expected behavior (both before and after) reliably on all systems. If it's not doing that, then something might be afoot.
Comment 46 Peter Van der Beken [:peterv] 2012-03-14 00:37:45 PDT
(In reply to Benoit Jacob [:bjacob] (On vacation until March 18) from comment #44)
> I didn't know whether I had to do it only for WebGLExtension, or for all
> sub-classes. This patch does it for all. Is that useful?

Yes, you need to do it for all. Sorry, I forgot that you also need to replace DOM_DEFAULT_SCRIPTABLE_FLAGS with DOM_DEFAULT_SCRIPTABLE_FLAGS | nsIXPCScriptable::WANT_ADDPROPERTY. Could you please check in a debugger that the AddProperty hook is actually called? We really should get a testcase for this.

(In reply to Benoit Jacob [:bjacob] (On vacation until March 18) from comment #44)
> Regarding your other review comment about the cycle, I checked, it turns out
> that WebGLExtension does NOT hold a strong reference to the corresponding
> WebGLContext. Instead, WebGLExtension inherits WebGLContextBoundObject,
> which only has a plain pointer (WebGLContext *mContext) to the corresponding
> WebGLContext.

That's not the issue, the context holds a strong reference to the extesion (mEnabledExtensions) and you're making it possible to add a reference from the extension to the context in JS. The testcase I'm asking for should add that (context.foo = extension;). Pretty sure that'll leak if you don't traverse mEnabledExtensions from WebGLContext.
Comment 47 :Ms2ger 2012-03-14 03:00:10 PDT
I guess you meant |extension.foo = context|, though we might as well add them both.
Comment 48 Peter Van der Beken [:peterv] 2012-03-16 14:36:06 PDT
(In reply to Ms2ger from comment #47)
> I guess you meant |extension.foo = context|, though we might as well add
> them both.

Right.
Comment 49 Benoit Jacob [:bjacob] (mostly away) 2012-03-19 05:05:21 PDT
Created attachment 607120 [details] [diff] [review]
preserve wrappers for webgl extension objects, v10

Here is a new patch taking into account your comments, and doing some other changes (fixing the extension-specific goop to use _INHERITED macros, and moving it to extension-specific cpp files).

However, I set a breakpoint on every WebGLExtensionSH method, and the only method that's ever called is WebGLExtensionSH::PreCreate. In particular, WebGLExtensionSH::AddProperty is never called, when running a script that does add a property on an extension object. Help needed.
Comment 50 Peter Van der Beken [:peterv] 2012-03-19 05:22:59 PDT
You need to add WANT_ADDPROPERTY to the flags for the extensions, not the context.
Comment 51 Benoit Jacob [:bjacob] (mostly away) 2012-03-19 05:32:15 PDT
Ouch. Sorry about that. Testing.
Comment 52 Benoit Jacob [:bjacob] (mostly away) 2012-03-19 05:55:16 PDT
Created attachment 607135 [details] [diff] [review]
preserve wrappers for webgl extension objects, v11

That fixed it.

Another upcoming patch will add the tests you requested.
Comment 53 Benoit Jacob [:bjacob] (mostly away) 2012-03-19 06:05:48 PDT
Created attachment 607139 [details] [diff] [review]
add tests creating reference cycles between WebGLContext and WebGLExtension (and subclass)

This causes some WebGL extension tests to create cycles with the context,

    var ext = gl.getExtension("OES_standard_derivatives");
    ext.context = gl;

and with themselves,

    ext.ext = ext;

I ran this with XPCOM_MEM_LEAK_LOG=1 and no leak was found.
Comment 54 Benoit Jacob [:bjacob] (mostly away) 2012-03-19 06:06:35 PDT
Note that the patches on the webgl conformance suite will be landed together with the next syncing of our copy; meanwhile I'll attempt to upstream them.
Comment 55 Benoit Jacob [:bjacob] (mostly away) 2012-03-19 06:09:44 PDT
Created attachment 607140 [details] [diff] [review]
add tests creating reference cycles between WebGLContext and WebGLExtension (and subclass)
Comment 56 Benoit Jacob [:bjacob] (mostly away) 2012-03-19 07:19:25 PDT
Try (together with upgrade of WebGL conformance tests, and patches on conformance tests attached on this bug):
https://tbpl.mozilla.org/?tree=Try&rev=5260a1a817b0
Comment 57 Benoit Jacob [:bjacob] (mostly away) 2012-03-19 10:15:33 PDT
Updated list of failing tests (for webgl tests upgrade):
https://tbpl.mozilla.org/?tree=Try&rev=02d5912b7d62
Comment 58 Benoit Jacob [:bjacob] (mostly away) 2012-03-19 15:10:21 PDT
Green, but I requested 20 rebuilds of WinXP opt M1 to make sure that it's fixed. Indeed (before that test got disabled), this used to be an intermittent orange, primarily on WinXP opt, as the GC sometimes ran during these conformance tests and collected the extension object.
Comment 59 Jeff Gilbert [:jgilbert] 2012-03-19 21:12:29 PDT
Hah, I saw those builds pending. I thought TBPL had just gone crazy or something.
Comment 60 Benoit Jacob [:bjacob] (mostly away) 2012-03-20 01:13:22 PDT
All M1 runs are green! So with your review, we can land this and the webgl tests update.
Comment 61 Benoit Jacob [:bjacob] (mostly away) 2012-03-23 17:41:06 PDT
Today I verified that a build with this patch + Jeff's patch in bug 696569 passes 100% of the WebGL 1.0.1 conformance test suite, aside from driver bugs.

Please review! One week to go before WebGL 1.0.1 is announced.
Comment 62 Benoit Jacob [:bjacob] (mostly away) 2012-03-26 07:46:45 PDT
The final WebGL conference call before 1.0.1 is this Thursday. This patch is the last one for full 1.0.1 conformance test success. I would like very much to have it in Nightly ahead of the Thursday conference call. That would mean land it within 2 days of now. Please review!
Comment 63 Peter Van der Beken [:peterv] 2012-03-26 10:12:58 PDT
Comment on attachment 607135 [details] [diff] [review]
preserve wrappers for webgl extension objects, v11

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

::: content/canvas/src/WebGLContext.h
@@ +756,5 @@
>          WebGL_EXT_texture_filter_anisotropic,
>          WebGL_MOZ_WEBGL_lose_context,
>          WebGLExtensionID_Max
>      };
> +    nsTArray<nsRefPtr<WebGLExtension> > mEnabledExtensions;

Avoid allocating by making this an nsAutoTArray<nsRefPtr<WebGLExtension>, WebGLExtensionID_Max>.

::: dom/base/nsDOMClassInfo.cpp
@@ +10719,5 @@
> +{
> +  *parentObj = globalObj;
> +
> +  nsCOMPtr<nsIWebGLExtension> iwebglext(do_QueryInterface(nativeObj));
> +  WebGLExtension *webglext = static_cast<WebGLExtension*>(iwebglext.get());

You can just cast, like in PreserveWrapper. Drop the QI.
Comment 64 Benoit Jacob [:bjacob] (mostly away) 2012-03-26 12:24:19 PDT
The patch:
http://hg.mozilla.org/integration/mozilla-inbound/rev/46a8181a7f15
follow-up as I accidentally checked in the previous iteration:
http://hg.mozilla.org/integration/mozilla-inbound/rev/331ff4b8e009
WebGL 1.0.1 tests upgrade + drop local patch removing the relevant unit test
http://hg.mozilla.org/integration/mozilla-inbound/rev/f9585cefcf56
Use the proper way for privileged JS to request a GC:
http://hg.mozilla.org/integration/mozilla-inbound/rev/acb20ce6ec61
Comment 67 Benoit Jacob [:bjacob] (mostly away) 2012-05-14 18:21:00 PDT
Thanks!

I never realized that extension names should be case-insensitive.

The test doesn't give the JS engine much of a chance to GC before checking that the added properties are preserved. Some of the extensions' tests, like OES_texture_float's, try to get the browser to GC before they check that. It would be nice to harmonize that and remove any now-redundant test, but as far as Firefox is concerned there isn't a very good way to do that. Anyway, the 'least bad way' to induce GC is probably the gc() function that we have in js-test-pre.js.
Comment 68 Benoit Jacob [:bjacob] (mostly away) 2012-05-28 13:37:41 PDT
I've added a gc() call as explained in comment 67. Don't see anything else to be done here, removing webgl-test-needed.

We will pass this test once that patches in bug 759178 and bug 755618 have landed.

Note You need to log in before you can comment on or make changes to this bug.