The default bug view has changed. See this FAQ.

Preserve added JS properties on the JS object returned by webgl.getExtension

RESOLVED FIXED in mozilla14

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

Trunk
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: webgl-conformance)

Attachments

(3 attachments, 12 obsolete attachments)

4.21 KB, patch
Details | Diff | Splinter Review
19.05 KB, patch
peterv
: review+
Details | Diff | Splinter Review
3.69 KB, patch
Details | Diff | Splinter Review
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).
How is the WebGLExtension object implemented?
nsIWebGLExtension, I guess. This probably "just" needs to implement nsWrapperCache?
Yes, that's what it sounds like.
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.
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.
So, I just landed a bunch of patches with this bug number instead of bug 727680. Sorry about that...
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.
(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?
Oops, yes. The PreCreate hook needs to set *parentObj to the windows with which the extension object is associated with.
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?
(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. ;-)
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?
OK, can you explain a bit more what PreCreate does? Is it called before or after the WebGLExtension c++ object is created?
(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 ;-).
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. :-)
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".
Attachment #603497 - Flags: feedback?(peterv)
Attachment #603497 - Flags: feedback?(khuey)
Attachment #603497 - Flags: feedback?(continuation)
Attachment #603497 - Flags: feedback?(bobbyholley+bmo)
https://tbpl.mozilla.org/?tree=Try&rev=20946e352a96
Comment on attachment 603497 [details] [diff] [review]
preserve wrappers for webgl extension objects

The CC macro gunk looks reasonable to me.
Attachment #603497 - Flags: feedback?(continuation) → feedback+
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.
Attachment #603497 - Flags: feedback?(bobbyholley+bmo) → feedback+
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.
Attachment #603497 - Attachment is obsolete: true
Attachment #603588 - Flags: review?(bobbyholley+bmo)
Attachment #603497 - Flags: feedback?(peterv)
Attachment #603497 - Flags: feedback?(khuey)
You should use MPL2 for new files, I believe.
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.
Attachment #603588 - Flags: review?(bobbyholley+bmo)
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.
Attachment #603799 - Flags: review?(peterv)
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).
(also, it should be quick to review - I've already given it a once-over).
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?
Attachment #603799 - Flags: review?(peterv) → review-
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.
Attachment #603588 - Attachment is obsolete: true
Attachment #603799 - Attachment is obsolete: true
Attachment #604237 - Flags: review?(peterv)
(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.
Attachment #604237 - Flags: review?(peterv)
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.
Attachment #604237 - Attachment is obsolete: true
Attachment #604297 - Flags: review?(peterv)
https://tbpl.mozilla.org/?tree=Try&rev=bd68552713dc
*oops* I just realized that I'm not even using the canvas anymore... so this can't be right. OK, I need help please!
(Assignee)

Updated

5 years ago
Attachment #604297 - Attachment is patch: false
(Assignee)

Updated

5 years ago
Attachment #604297 - Attachment is patch: true
Attachment #604297 - Flags: review?(peterv)
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
Attachment #604297 - Attachment is obsolete: true
Attachment #604299 - Flags: review?(peterv)
Created attachment 604300 [details] [diff] [review]
preserve wrappers for webgl extension objects, v6

sorry, v5 didn't compile... thanks again to Bobby.
Attachment #604299 - Attachment is obsolete: true
Attachment #604300 - Flags: review?(peterv)
Attachment #604299 - Flags: review?(peterv)
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.
(In reply to Ms2ger from comment #34)
> Looks like another leak here.

Please use smart pointers (nsCOMPtr in this case) to avoid these errors.
Except for nsWrapperCache, which just exists to confuse you.
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.
Attachment #604300 - Attachment is obsolete: true
Attachment #604387 - Flags: review?(peterv)
Attachment #604300 - Flags: review?(peterv)
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?
(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.
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.
Attachment #604393 - Flags: review?(peterv)
(Assignee)

Updated

5 years ago
Attachment #604387 - Attachment is obsolete: true
Attachment #604387 - Flags: review?(peterv)
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 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.
Attachment #604393 - Flags: review?(peterv) → review-
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.
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.
Attachment #604393 - Attachment is obsolete: true
Attachment #605580 - Flags: review?(peterv)
(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.
(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.
I guess you meant |extension.foo = context|, though we might as well add them both.
Assignee: nobody → bjacob
(In reply to Ms2ger from comment #47)
> I guess you meant |extension.foo = context|, though we might as well add
> them both.

Right.
Target Milestone: --- → mozilla13
Version: unspecified → Trunk
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.
Attachment #605580 - Attachment is obsolete: true
Attachment #607120 - Flags: review?(peterv)
Attachment #605580 - Flags: review?(peterv)
You need to add WANT_ADDPROPERTY to the flags for the extensions, not the context.
Ouch. Sorry about that. Testing.
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.
Attachment #607120 - Attachment is obsolete: true
Attachment #607135 - Flags: review?(peterv)
Attachment #607120 - Flags: review?(peterv)
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.
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.
Created attachment 607140 [details] [diff] [review]
add tests creating reference cycles between WebGLContext and WebGLExtension (and subclass)
Attachment #607139 - Attachment is obsolete: true
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
Updated list of failing tests (for webgl tests upgrade):
https://tbpl.mozilla.org/?tree=Try&rev=02d5912b7d62
(Assignee)

Updated

5 years ago
Blocks: 728357
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.
Hah, I saw those builds pending. I thought TBPL had just gone crazy or something.
All M1 runs are green! So with your review, we can land this and the webgl tests update.
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.
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 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.
Attachment #607135 - Flags: review?(peterv) → review+
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
Target Milestone: mozilla13 → mozilla14
https://hg.mozilla.org/mozilla-central/rev/46a8181a7f15
https://hg.mozilla.org/mozilla-central/rev/331ff4b8e009
https://hg.mozilla.org/mozilla-central/rev/f9585cefcf56
https://hg.mozilla.org/mozilla-central/rev/acb20ce6ec61
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
No longer blocks: 728357, 680721
Whiteboard: webgl-conformance webgl-test-needed

Comment 66

5 years ago
Here's a test to test this
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/extensions/get-extension.html
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.
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.
Whiteboard: webgl-conformance webgl-test-needed → webgl-conformance
You need to log in before you can comment on or make changes to this bug.