If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

GC: Handlify the JSAPI

RESOLVED FIXED in mozilla24

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: terrence, Assigned: evilpie)

Tracking

({dev-doc-needed})

Trunk
mozilla24
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(10 attachments)

(Reporter)

Description

4 years ago
Created attachment 764240 [details] [diff] [review]
TransplantObject*; v0

I've been doing these piecemeal while building other things and will be attaching pieces as I create them. So far they have been trivial.

Anyone that wants to pitch in and handlify a few functions would be more than welcome.
Attachment #764240 - Flags: review?(jcoppeard)
(Reporter)

Comment 1

4 years ago
Created attachment 764242 [details] [diff] [review]
ResolveStandardClass; v0
Attachment #764242 - Flags: review?(jcoppeard)
(Reporter)

Comment 2

4 years ago
Created attachment 764243 [details] [diff] [review]
EnumerateStandardClasses; v0
Attachment #764243 - Flags: review?(jcoppeard)
(Reporter)

Comment 3

4 years ago
Created attachment 764246 [details] [diff] [review]
REMOVAL EnumerateResolvedStandardClasses; v0

The browser does not use this and I don't think we want to expose such functionality anyway, even in the friend api.
Attachment #764246 - Flags: superreview?(luke)
Attachment #764246 - Flags: review?(jwalden+bmo)
Comment on attachment 764240 [details] [diff] [review]
TransplantObject*; v0

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

::: dom/base/nsGlobalWindow.cpp
@@ +2395,5 @@
>      } else {
>        JS::Rooted<JSObject*> global(cx,
>          xpc_UnmarkGrayObject(newInnerWindow->mJSObject));
> +      JS::Rooted<JSObject*> outerObject(cx,
> +        NewOuterWindowProxy(cx, global, thisChrome));

You'll want a dom peer as usual

::: js/src/jsapi.h
@@ +2012,4 @@
>  
>  extern JS_FRIEND_API(JSObject *)
>  js_TransplantObjectWithWrapper(JSContext *cx,
> +                               JS::HandleObject origobj,

I thought we just decided over in bug 884371 that we'd use JS::Handle<JSObject*> in jsapi.h?
Comment on attachment 764242 [details] [diff] [review]
ResolveStandardClass; v0

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

::: content/xbl/src/nsXBLDocumentInfo.cpp
@@ +162,5 @@
>    NS_RELEASE(nativeThis);
>  }
>  
>  static JSBool
> +nsXBLDocGlobalObject_resolve(JSContext *cx, JS::HandleObject obj, JS::HandleId id)

Conflicts with bug 884371, and doesn't seem necessary?
(Reporter)

Comment 6

4 years ago
(In reply to :Ms2ger from comment #5)
> ::: content/xbl/src/nsXBLDocumentInfo.cpp
> >  static JSBool
> > +nsXBLDocGlobalObject_resolve(JSContext *cx, JS::HandleObject obj, JS::HandleId id)
> 
> Conflicts with bug 884371, and doesn't seem necessary?

That bug seemed to only be about MutableHandleValue. Besides, they will be trivial to merge and I'd rather over-fix than miss some.

(In reply to :Ms2ger from comment #4)
> ::: dom/base/nsGlobalWindow.cpp
> @@ +2395,5 @@
> >      } else {
> >        JS::Rooted<JSObject*> global(cx,
> >          xpc_UnmarkGrayObject(newInnerWindow->mJSObject));
> > +      JS::Rooted<JSObject*> outerObject(cx,
> > +        NewOuterWindowProxy(cx, global, thisChrome));
> 
> You'll want a dom peer as usual

For something this trivial?
 
> ::: js/src/jsapi.h
> @@ +2012,4 @@
> >  
> >  extern JS_FRIEND_API(JSObject *)
> >  js_TransplantObjectWithWrapper(JSContext *cx,
> > +                               JS::HandleObject origobj,
> 
> I thought we just decided over in bug 884371 that we'd use
> JS::Handle<JSObject*> in jsapi.h?

Well, JSAPI is technically in JS. There is a fair argument to be made that the API should look how we expect users of the API to look, but we haven't really had that discussion yet.

Updated

4 years ago
Attachment #764246 - Flags: superreview?(luke) → superreview+
Comment on attachment 764246 [details] [diff] [review]
REMOVAL EnumerateResolvedStandardClasses; v0

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

\o/
Attachment #764246 - Flags: review?(jwalden+bmo) → review+

Updated

4 years ago
Attachment #764240 - Flags: review?(jcoppeard) → review+

Updated

4 years ago
Attachment #764242 - Flags: review?(jcoppeard) → review+
Comment on attachment 764243 [details] [diff] [review]
EnumerateStandardClasses; v0

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

All looks good.  We should decide on a style for jsapi.h though.
Attachment #764243 - Flags: review?(jcoppeard) → review+
(Reporter)

Comment 9

4 years ago
Try looks good:
https://tbpl.mozilla.org/?tree=Try&rev=bb17ccf250fd

Pushed:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c55d7332c83a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b07c97f12e34
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b1636ba097f9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/95952d257a56
https://hg.mozilla.org/mozilla-central/rev/c55d7332c83a
https://hg.mozilla.org/mozilla-central/rev/b07c97f12e34
https://hg.mozilla.org/mozilla-central/rev/b1636ba097f9
https://hg.mozilla.org/mozilla-central/rev/95952d257a56
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
(Assignee)

Updated

4 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 11

4 years ago
Created attachment 765256 [details] [diff] [review]
JS_GetPrototype v0

Not sure if this needs addional reviewers.
Attachment #765256 - Flags: review?(terrence)
This is going to see quite some more activity
Status: REOPENED → NEW
Whiteboard: [leave open][js:t]
(Assignee)

Comment 13

4 years ago
Created attachment 765299 [details] [diff] [review]
JS_SetPrototype v0

All our rooting work really shows.
(Assignee)

Updated

4 years ago
Attachment #765299 - Flags: review?(terrence)
(Reporter)

Comment 14

4 years ago
Comment on attachment 765256 [details] [diff] [review]
JS_GetPrototype v0

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

\o/ Thanks! r=me

::: js/src/jsapi.cpp
@@ +3329,5 @@
>      return obj->getPrivate();
>  }
>  
>  JS_PUBLIC_API(JSBool)
> +JS_GetPrototype(JSContext *cx, JS::Handle<JSObject*> obj, JS::MutableHandle<JSObject*> protop)

As below.

::: js/src/jsapi.h
@@ +3099,5 @@
>  JS_GetInstancePrivate(JSContext *cx, JSObject *obj, JSClass *clasp,
>                        jsval *argv);
>  
>  extern JS_PUBLIC_API(JSBool)
> +JS_GetPrototype(JSContext *cx, JS::Handle<JSObject*> obj, JS::MutableHandle<JSObject*> protop);

Lets stick with the typedefs for now. I'm not sure how this is going to shake out eventually, but lets at least remain consistent until we get a clear concensus.
Attachment #765256 - Flags: review?(terrence) → review+
(Reporter)

Comment 15

4 years ago
Comment on attachment 765299 [details] [diff] [review]
JS_SetPrototype v0

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

r=me

::: js/src/jsapi.h
@@ +3102,5 @@
>  extern JS_PUBLIC_API(JSBool)
>  JS_GetPrototype(JSContext *cx, JS::Handle<JSObject*> obj, JS::MutableHandle<JSObject*> protop);
>  
>  extern JS_PUBLIC_API(JSBool)
> +JS_SetPrototype(JSContext *cx, JS::Handle<JSObject*> obj, JS::Handle<JSObject*> proto);

As per the prior patch, lets stick with typedefs for now.
Attachment #765299 - Flags: review?(terrence) → review+
(Assignee)

Comment 16

4 years ago
Oh damn, I forgot about these patches.
(Reporter)

Comment 17

4 years ago
It seems we're going with external style in jsapi.h, so ignore my review comments.
(Assignee)

Comment 18

4 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/f67fd341dd2e
http://hg.mozilla.org/integration/mozilla-inbound/rev/a2f13bbf1457

I had to make some minor changes to the first patch.
https://hg.mozilla.org/mozilla-central/rev/f67fd341dd2e
https://hg.mozilla.org/mozilla-central/rev/a2f13bbf1457
(Reporter)

Updated

4 years ago
Blocks: 795030
No longer blocks: 773686
(Assignee)

Comment 20

4 years ago
Created attachment 788955 [details] [diff] [review]
Remove JS_ValueToECMAInt32

Removes JS_ValueToECMAInt32 and replaces it by ToInt32, which already used handles. I had a patch for the uint32 version, but I screwed something up and can't find the changes anymore.
Attachment #788955 - Flags: review?(terrence)
(Reporter)

Comment 21

4 years ago
Comment on attachment 788955 [details] [diff] [review]
Remove JS_ValueToECMAInt32

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

The handlification changes look fine to me.

I do, however, vaguely recall there being some extensive discussion about the naming of the method you are removing: a difference between machine int32 and JS's "Int32" that we wanted to distinguis in the API. Please ask around (guessing Waldo, but I don't remember exactly) or make a search to ensure that this renaming is actually okay.
Attachment #788955 - Flags: review?(terrence)
(Assignee)

Comment 22

4 years ago
This function was just a wrapper around the ToInt32. ToInt32 also appears in the ES spec. If we still expose some non spec-conform version we should give that one an ugly name.
Comment on attachment 788955 [details] [diff] [review]
Remove JS_ValueToECMAInt32

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

There used to be JS_ValueToInt32 and JS_ValueToECMAInt32 both.  The former did weird clamping stuff, I don't remember.  The latter implemented, exactly, the spec's ToInt32 method.  Since JS::ToInt32 does exactly the spec's method, this looks fine on all those points.  I'm fairly sure that's the distinction being mooted in comment 21.
(Reporter)

Comment 24

4 years ago
Comment on attachment 788955 [details] [diff] [review]
Remove JS_ValueToECMAInt32

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

Excellent! In that case, r=me.
Attachment #788955 - Flags: review+
Backed out because of build bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/911fcdcadf4d
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=3d9664b78ca7
(Assignee)

Comment 26

4 years ago
Made some small fix and landed again today:
http://hg.mozilla.org/integration/mozilla-inbound/rev/339041b86edc
Try run: https://tbpl.mozilla.org/?tree=Try&rev=1bb8f3e147e8
https://hg.mozilla.org/mozilla-central/rev/339041b86edc
(Assignee)

Comment 28

4 years ago
Created attachment 804206 [details] [diff] [review]
Remove JS_ValueToECMAUint32

Not try test yet, but compiles. Test some of the dumpHeap functionality.
Attachment #804206 - Flags: review?(terrence)
(Reporter)

Comment 29

4 years ago
Comment on attachment 804206 [details] [diff] [review]
Remove JS_ValueToECMAUint32

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

In future, please try to split stuff like this up to make reviewing less context-switchy. It is a righteous cleanup, however, so I won't complain too much. :-)

r=me, but I would like Andrew to take a look at the DumpHeap changes too: he is DumpHeap's owner after all.

::: ipc/testshell/XPCShellEnvironment.cpp
@@ +327,3 @@
>      if (!fileName) {
>          dumpFile = stdout;
>      } else {

ditto

::: js/src/shell/js.cpp
@@ +2205,5 @@
> +                          startThing.isUndefined() ? nullptr : startThing.toGCThing(),
> +                          startThing.isUndefined() ? JSTRACE_OBJECT : startThing.get().gcKind(),
> +                          thingToFind.isUndefined() ? nullptr : thingToFind.toGCThing(),
> +                          maxDepth,
> +                          thingToIgnore.isUndefined() ? nullptr : thingToIgnore.toGCThing());

Has the shell finally gotten the right C++0x flags to compile everywhere with nullptr? I thought there were still some platforms that require an include to get the shim?

::: js/xpconnect/shell/xpcshell.cpp
@@ +534,1 @@
>      } else {

FILE *dumpFile = stdout;
if (fileName) {
    dumpFile = fopen(...);
    ....
}
Attachment #804206 - Flags: review?(terrence)
Attachment #804206 - Flags: review?(continuation)
Attachment #804206 - Flags: review+
I can get to it in a day or two, or Bill can probably take a look.
I would suggest deleting these two implementations of DumpHeap. Would that be okay with you, Andrew?
Oh, that's DumpHeap, not DumpHeapComplete, I don't know much about that.  I think the idea is that you can use it from the gdb and get somewhat useful results from it.  But yeah, I don't know if anybody uses it from xpcshell or the shell like that, so it is probably okay to get rid of.
Attachment #804206 - Flags: review?(continuation)
Attachment #804206 - Flags: feedback+
(Assignee)

Comment 33

4 years ago
Created attachment 816704 [details] [diff] [review]
JS_WrapObject
Attachment #816704 - Flags: review?(terrence)
(Reporter)

Comment 34

4 years ago
Comment on attachment 816704 [details] [diff] [review]
JS_WrapObject

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

Awesomesauce, deep fried and dipped in more awesomesauce. r=me
Attachment #816704 - Flags: review?(terrence) → review+
(Assignee)

Comment 35

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5126b48adf91
Just for the record, this landed with an incorrect bug number (8844195).
https://hg.mozilla.org/mozilla-central/rev/5126b48adf91
(Assignee)

Comment 37

4 years ago
How in gods name did I end up with that number? :( Sorry for that.
(Assignee)

Comment 38

4 years ago
Created attachment 817851 [details] [diff] [review]
Remove JS_ValueToNumber

Not try approved, but I just think this okay.
Attachment #817851 - Flags: review?(terrence)
(Reporter)

Comment 39

4 years ago
Comment on attachment 817851 [details] [diff] [review]
Remove JS_ValueToNumber

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

Looks good to me with one dev-doc-needed. I'd definitely try-server this before landing though.

::: js/src/jsapi.h
@@ -1041,5 @@
>  extern JS_PUBLIC_API(JSString *)
>  JS_ValueToSource(JSContext *cx, jsval v);
>  
> -extern JS_PUBLIC_API(bool)
> -JS_ValueToNumber(JSContext *cx, jsval v, double *dp);

It looks like JS::ToNumber isn't documented in MDN. Could you add that (maybe just copy the JS_ValueToNumber article), deprecate JS_ValueToNumber and link it to JS::ToNumber.
Attachment #817851 - Flags: review?(terrence) → review+
(Assignee)

Comment 40

4 years ago
I added some amount of documentation now.
http://hg.mozilla.org/integration/mozilla-inbound/rev/313eee20c52f
https://hg.mozilla.org/mozilla-central/rev/313eee20c52f

We should probably close this bug.
Flags: in-testsuite-

Updated

4 years ago
Blocks: 773686
Keywords: dev-doc-needed
(Assignee)

Comment 42

4 years ago
Okay let's close this one.
Status: NEW → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Whiteboard: [leave open][js:t] → [js:t]

Updated

4 years ago
Assignee: general → evilpies
You need to log in before you can comment on or make changes to this bug.