Seriously. This is 2014, and we still dump junk like "chrome://xbl-marquee/content/xbl-marquee.xml#marquee 85b" on the content global. The simplest thing would be to just define it on the XBL scope instead, but that just sweeps the dirtiness under the rug, and doesn't let us get rid of JS_GetObjectId. Terrence, how badly do we want to get rid of that? The more robust solution would be to use a WeakMap keyed on the grand-proto, mapping to a dictionary object keyed by binding names. Thoughts?

> The simplest thing would be to just define it on the XBL scope instead I'm really surprised we're not doing that already. Can we just hang a hashtable off a reserved slot on the XBL scope's global, with some tracing bits and whatnot?

JS_GetObjectId is total nuttiness in a moving-gc world and it will only get worse as we go forward. Right now we are doing a minor GC whenever we call this function to tenure anything that could move. This simply isn't going to work with compacting GC. Even if it would work, we really do not want to trigger a non-incremental shrinking GC every time we call JS_GetObjectId. It needs to go.

I was able to accomplish quite a lot here: https://tbpl.mozilla.org/?tree=Try&rev=687cee582cb4

That stack is a thing of beauty! Don't forget to kill JS_GetObjectId too, since you've removed the only user.

(In reply to Terrence Cole [:terrence] from comment #4) > Don't forget to kill JS_GetObjectId too, > since you've removed the only user. That is the last patch, which you may have missed because its checkin-comment also has the try: goop.

This was from bug 989183.

Note that we simultaneously rip out all of the crazy lifetime management for the dynamic JSClasses here (it would be nice to do that in a separate patch, but it's all kind of tied up together). With this patch, we simply have one dynamic JSClass per class object, which is deleted in the finalizer. In the next patch, we remove dynamic JSClasses entirely.

Comment on attachment 8400821 [details] [diff] [review] Part 2 - Expose JSAPI functions for creating and manipulating scripted WeakMaps. v1 Review of attachment 8400821 [details] [diff] [review]: ----------------------------------------------------------------- Neat! r=me ::: js/src/jsapi.h @@ +4341,4 @@ > uint32_t lineNumber, uint32_t columnNumber, JSErrorReport *report, > HandleString message, MutableHandleValue rval); > > } /* namespace JS */ Let's move this below the new definitions and put the new definitions in JS::. ::: js/src/jsweakmap.cpp @@ +300,2 @@ > if (!map) { > + map = cx->new_<ObjectValueMap>(cx, mapObj.get()); Urk! This is a pre-existing oom bug. Please add an oom check here. The reporting is already done by cx->new_, so just: if (!map) return false; @@ +327,1 @@ > WeakMapPostWriteBarrier(cx->runtime(), map, key.get()); Please keep the post-barrier adjacent to the put.

Comment on attachment 8400827 [details] [diff] [review] Part 8 - Remove JS_GetObjectId. v1 Review of attachment 8400827 [details] [diff] [review]: ----------------------------------------------------------------- \o/ r=me

Comment on attachment 8400823 [details] [diff] [review] Part 4 - Remove silly LRU cache of nsXBLJSClass instances. v1 Review of attachment 8400823 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/xbl/nsXBLService.h @@ +142,5 @@ > nsCString& Key() { return mKey; } > void SetKey(const nsCString& aKey) { mKey = aKey; } > > nsrefcnt Hold() { return ++mRefCnt; } > + nsrefcnt Drop() { --mRefCnt; if (!mRefCnt) delete this; return mRefCnt; } if (!mRefCnt) delete this; return mRefCnt; Does that seem fishy to anyone else?

> Does that seem fishy to anyone else? I think that's okay. That's more or less how Release() is usually implemented. It would be nice if this just used one of our regular non-ISupports ref counted implementations. All these hand rolled AddRefs and Releases scattered around the codebase are not great. It is also fairly bizarre to have Hold() and Drop() as synonyms for AddRef() and Release(). Anyways, if you file a followup, Bobby, to clean up the refcounting implementation here maybe I can look at it at some point.

(In reply to Andrew McCreight [:mccr8] from comment #17) > > Does that seem fishy to anyone else? > > I think that's okay. That's more or less how Release() is usually > implemented. Even with the uaf? > It would be nice if this just used one of our regular > non-ISupports ref counted implementations. All these hand rolled AddRefs > and Releases scattered around the codebase are not great. It is also fairly > bizarre to have Hold() and Drop() as synonyms for AddRef() and Release(). > Anyways, if you file a followup, Bobby, to clean up the refcounting > implementation here maybe I can look at it at some point. All of this code actually goes away by the final patch/

(In reply to Bobby Holley (:bholley) from comment #18) > Even with the uaf? Ah, good point, I guess usually the count gets copied out! > All of this code actually goes away by the final patch/ Fantastic! At some point I need to just go through and eliminate as much custom refcounting from the codebase as possible.

Comment on attachment 8400822 [details] [diff] [review] Part 3 - Remove unnecessary conditional and unindent. v1 r=me

Comment on attachment 8400823 [details] [diff] [review] Part 4 - Remove silly LRU cache of nsXBLJSClass instances. v1 Let's not have the use-after-free on mRefCnt, and r=me with that

Comment on attachment 8400824 [details] [diff] [review] Part 5 - Stop using the full-blown class object setup for precompilation. v1 r=me

Comment on attachment 8400825 [details] [diff] [review] Part 6 - Store class objects in a weak map off the XBL global. v1 >+ // hoist anonymous into the XBL scope, this creates the potential for tricky "anonymous content into"? s/RootedObject/Rooted<JSObject*>, please. r=me

Comment on attachment 8400826 [details] [diff] [review] Part 7 - Get rid of dynamic XBL JSClasses. v1 r=me, I think. My wetware emulation is not quite following all the nooks and crannies here. :(

Addressed review feedback. Doing one final all-platform try push: https://tbpl.mozilla.org/?tree=Try&rev=1829a100d6e4

(In reply to Bobby Holley (:bholley) from comment #25) > Addressed review feedback. Doing one final all-platform try push: > > https://tbpl.mozilla.org/?tree=Try&rev=1829a100d6e4 Hm, seems to trigger a weird windows-only SnowWhite crash in AudioNode. Let's see which changeset is responsible: https://tbpl.mozilla.org/?tree=Try&rev=47fe768ce1d8 https://tbpl.mozilla.org/?tree=Try&rev=1c733826bbcd https://tbpl.mozilla.org/?tree=Try&rev=e51330150267 https://tbpl.mozilla.org/?tree=Try&rev=c8f8880fbf4d

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/cde3907adff0 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ae4769d5b60e remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c1f95c7c1ef5 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d14eeca5a05a remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/bd83539593c6 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b7e93a0b9b47 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e212a0c2a3cb remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0fe126aa32df remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/bf804e26c54b

https://hg.mozilla.org/mozilla-central/rev/cde3907adff0 https://hg.mozilla.org/mozilla-central/rev/ae4769d5b60e https://hg.mozilla.org/mozilla-central/rev/c1f95c7c1ef5 https://hg.mozilla.org/mozilla-central/rev/d14eeca5a05a https://hg.mozilla.org/mozilla-central/rev/bd83539593c6 https://hg.mozilla.org/mozilla-central/rev/b7e93a0b9b47 https://hg.mozilla.org/mozilla-central/rev/e212a0c2a3cb https://hg.mozilla.org/mozilla-central/rev/0fe126aa32df https://hg.mozilla.org/mozilla-central/rev/bf804e26c54b