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

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Blocks 1 bug)

unspecified
mozilla31
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments)

Assignee

Description

5 years ago
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?
Assignee

Updated

5 years ago
Flags: needinfo?(terrence)
> 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.
Flags: needinfo?(terrence)
Assignee

Updated

5 years ago
Assignee: nobody → bobbyholley
Depends on: CVE-2014-1524
Assignee

Comment 3

5 years ago
I was able to accomplish quite a lot here:

https://tbpl.mozilla.org/?tree=Try&rev=687cee582cb4
Summary: Stop storing XBL class objects as properties on the content global → Stop storing XBL class objects as properties on the content global and eliminate the dynamic JSClasses
That stack is a thing of beauty! Don't forget to kill JS_GetObjectId too, since you've removed the only user.
Assignee

Comment 5

5 years ago
(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.
Assignee

Updated

5 years ago
Attachment #8400775 - Flags: review?(bobbyholley) → review+
Assignee

Comment 11

5 years ago
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.
Attachment #8400825 - Flags: review?(bzbarsky)
Assignee

Comment 13

5 years ago
Attachment #8400827 - Flags: review?(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.
Attachment #8400821 - Flags: review?(terrence) → review+
Comment on attachment 8400827 [details] [diff] [review]
Part 8 - Remove JS_GetObjectId. v1

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

\o/ r=me
Attachment #8400827 - Flags: review?(terrence) → review+
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.
Assignee

Comment 18

5 years ago
(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.
Assignee

Updated

5 years ago
Blocks: 993772
Comment on attachment 8400822 [details] [diff] [review]
Part 3 - Remove unnecessary conditional and unindent. v1

r=me
Attachment #8400822 - Flags: review?(bzbarsky) → review+
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
Attachment #8400823 - Flags: review?(bzbarsky) → review+
Comment on attachment 8400824 [details] [diff] [review]
Part 5 - Stop using the full-blown class object setup for precompilation. v1

r=me
Attachment #8400824 - Flags: review?(bzbarsky) → review+
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
Attachment #8400825 - Flags: review?(bzbarsky) → review+
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.  :(
Attachment #8400826 - Flags: review?(bzbarsky) → review+
Assignee

Comment 25

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

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

Comment 26

5 years ago
(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
Assignee

Updated

5 years ago
Blocks: 990729
Assignee

Updated

5 years ago
Depends on: 996077
Duplicate of this bug: 969410
Blocks: 756086
You need to log in before you can comment on or make changes to this bug.