990290 - Stop storing XBL class objects as properties on the content global and eliminate the dynamic JSClasses

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Bobby Holley (:bholley)]

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?

Comment 1

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 2

[Terrence Cole [:terrence]]

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.

Comment 3

[Bobby Holley (:bholley)]

I was able to accomplish quite a lot here:

https://tbpl.mozilla.org/?tree=Try&rev=687cee582cb4

Comment 4

[Terrence Cole [:terrence]]

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

Comment 5

[Bobby Holley (:bholley)]

(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.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 1 - Stop using ye olde JS_InitClass for XBL class objects. v3 r=bz

This was from bug 989183.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 2 - Expose JSAPI functions for creating and manipulating scripted WeakMaps. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 3 - Remove unnecessary conditional and unindent. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 4 - Remove silly LRU cache of nsXBLJSClass instances. v1

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 5 - Stop using the full-blown class object setup for precompilation. v1

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 6 - Store class objects in a weak map off the XBL global. v1

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 12

[Bobby Holley (:bholley)]

Attachment: Part 7 - Get rid of dynamic XBL JSClasses. v1

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 8 - Remove JS_GetObjectId. v1

Comment 14

[Terrence Cole [:terrence]]

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 15

[Terrence Cole [:terrence]]

Comment on attachment 8400827 [details] [diff] [review]
Part 8 - Remove JS_GetObjectId. v1

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

\o/ r=me

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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?

Comment 17

[Andrew McCreight [:mccr8]]

> 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.

Comment 18

[Bobby Holley (:bholley)]

(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/

Comment 19

[Andrew McCreight [:mccr8]]

(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 20

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 21

[Boris Zbarsky [:bzbarsky]]

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 22

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 23

[Boris Zbarsky [:bzbarsky]]

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 24

[Boris Zbarsky [:bzbarsky]]

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.  :(

Comment 25

[Bobby Holley (:bholley)]

Addressed review feedback. Doing one final all-platform try push:

https://tbpl.mozilla.org/?tree=Try&rev=1829a100d6e4

Comment 26

[Bobby Holley (:bholley)]

(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

Comment 27

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=349c105667b7

Comment 28

[Bobby Holley (:bholley)]

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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