424376 - Stop exposing compile-time function objects

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

Attachment: v 0.5

+++ This bug was initially created as a clone of Bug #423874 +++

Currently JSObject instances that code creates when compiling scripts can be accessed by API users via calling JS_GetFunctionObject. Since such objects have null as scope and proto to prevent leaking compile-time scopes, this causes various issues including crashes when one try to call such objects.

One way to fix this is to make JSFunction equivalent to JSObject.

The attached patch implements this idea. Note that currently it fails a few tests in js shell, I will address that later today.

Comment 1

[Igor Bukanov]

Attachment: v1

The problem with making JSFunction == JSObject is the current code that does (JSFunction *) JS_GetPrivate(cx, obj) where obj is a function object. Thus for compatibility even with JSFunction == JSObject the private slot must still contain JSFunction.

But that mean that for a scripted function there is no room to store a pointer to JSScripedFunction as the extra 2 available slots are used by xpconnect even in the case of scripted functions. 

To address this the patch uses fat objects for all function objects to store a pointer to JSScripedFunction.

Another notable change is elimination of backpointer to the function object from JSScriptedFunction. This change makes it very explicit that the only purpose of the function object that is allocated at the compile time is to store the scripted function in the object table of the parent script. It should be straightforward to eliminate such objects completely, but this is something that is for bug 406356.

In all places that the patch touches I use sfun for JSScriptedFunction, nfun for JSNativeFunction and funobj for JSFunction.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 311070 [details] [diff] [review]
v1

No back pointer is great -- an ancient horrible hazard removed!

>@@ -2693,17 +2697,20 @@ JS_InitClass(JSContext *cx, JSObject *ob
>     if (key != JSProto_Null &&
>         !parent_proto &&
>         !js_GetClassPrototype(cx, obj, INT_TO_JSID(JSProto_Object),
>                               &parent_proto)) {
>         return NULL;
>     }
> 
>     /* Create a prototype object for this class. */
>-    proto = js_NewObject(cx, clasp, parent_proto, obj, 0);
>+    extraBytes = (clasp != &js_FunctionClass)
>+                 ? 0
>+                 : sizeof(JSFunction) - sizeof(JSObject);

This recurs elsewhere. Macroize?

Might want a general way for a JSClass (or JSExtendedClass) to add extra bytes to its JSObject allocation. But we would be low on clasp->flags bits (although we could reorganize and use fewer bits for certain fields today). We could require JSExtendedClass and add an extraBytes member to it, or an optional hook returning same. Not in this bug, but I wanted to get a quick reaction.

> #define NATIVE_FUN_MINARGS(nfun)                                              \
>     (((nfun)->flags & JSFUN_FAST_NATIVE) ? (nfun)->minargs : (nfun)->nargs)
> 
>+#define GET_NATIVE_FUN_CLASS(nfun)                                            \
>+    ((JSClass *)((nfun)->base.sfunOrClass & ~(jsuword) 1))

Prevailing naming convention seems FUN_<VERB>_<OBJECT> so NATIVE_FUN_GET_CLASS seems better, etc.

Looks great, thanks for the sfun/nfun canonical naming. r=me,

/be

Comment 3

[Igor Bukanov]

Attachment: v2

The new version addresses the nits and no longer adds unused JSScript.functionName, which is a leftover from some early version of the patch.

Asking for b5 approval as this patch is required to fix bug 418443, a b5 blocker.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

That bug is no longer a b5 blocker - is this still super-safe and absolutely necessary to take for b5?

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)
> That bug is no longer a b5 blocker - is this still super-safe and absolutely
> necessary to take for b5?

This bug blocks bug 418443 which is a 1.9 blocker. The patch is safe once past the testing gauntlet. I think we want to get this into b5, not wait for RC. Igor, what do you think?

/be

Comment 6

[Mike Schroepfer]

(In reply to comment #5)
> (In reply to comment #4)
> > That bug is no longer a b5 blocker - is this still super-safe and absolutely
> > necessary to take for b5?
> 
> This bug blocks bug 418443 which is a 1.9 blocker. The patch is safe once past
> the testing gauntlet. I think we want to get this into b5, not wait for RC.
> Igor, what do you think?
> 
> /be
> 


Have we run through the test gauntlet?  Any new tests for this.  I tend to agree that with a patch this large we should get it in b5, not rc1.

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> (In reply to comment #5)
> > Igor, what do you think?

The patch may cause compatibility problems with extensions that relies too much on the detailed behavior of SpiderMonkey internals. For this reason it is nice to have it in b5 for extra testing. 

> 
> Have we run through the test gauntlet?  Any new tests for this.

Well, I run it through shell tests. Since mochi tests fails for me without the patch right now (I am trying to figure out the reason for this), I can not claim that the patches passes mochi tests without regressions.

Comment 8

[Brendan Eich [:brendan]]

Anyone able to apply to a clean tree and help mochitest?

/be

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

I just did. When I ran all the tests at once, two failed. However, when I ran just the failing tests individually, they passed. Therefore, I give the patch a passing grade on mochitest.

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> I just did. When I ran all the tests at once, two failed. 

Which tests have failed?

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Unfortunately, I lost the tests that failed when I tried to run them individually.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 311135 [details] [diff] [review]
v2

a=beltzner based on bkap's testing summary. Please watch the tree and buglist very closely and be available to deal with any regressions or fallout.

Comment 13

[Igor Bukanov]

I checked in the patch from the comment 3 to the trunk:

Checking in js/src/js.c;
/cvsroot/mozilla/js/src/js.c,v  <--  js.c
new revision: 3.210; previous revision: 3.209
done
Checking in js/src/jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.436; previous revision: 3.435
done
Checking in js/src/jsdbgapi.c;
/cvsroot/mozilla/js/src/jsdbgapi.c,v  <--  jsdbgapi.c
new revision: 3.138; previous revision: 3.137
done
Checking in js/src/jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.311; previous revision: 3.310
done
Checking in js/src/jsemit.h;
/cvsroot/mozilla/js/src/jsemit.h,v  <--  jsemit.h
new revision: 3.93; previous revision: 3.92
done
Checking in js/src/jsexn.c;
/cvsroot/mozilla/js/src/jsexn.c,v  <--  jsexn.c
new revision: 3.109; previous revision: 3.108
done
Checking in js/src/jsfun.c;
/cvsroot/mozilla/js/src/jsfun.c,v  <--  jsfun.c
new revision: 3.271; previous revision: 3.270
done
Checking in js/src/jsfun.h;
/cvsroot/mozilla/js/src/jsfun.h,v  <--  jsfun.h
new revision: 3.60; previous revision: 3.59
done
Checking in js/src/jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.297; previous revision: 3.296
done
Checking in js/src/jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.483; previous revision: 3.482
done
Checking in js/src/jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.456; previous revision: 3.455
done
Checking in js/src/jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.307; previous revision: 3.306
done
Checking in js/src/jsparse.c;
/cvsroot/mozilla/js/src/jsparse.c,v  <--  jsparse.c
new revision: 3.338; previous revision: 3.337
done
Checking in js/src/jsparse.h;
/cvsroot/mozilla/js/src/jsparse.h,v  <--  jsparse.h
new revision: 3.61; previous revision: 3.60
done
Checking in js/src/jsscript.c;
/cvsroot/mozilla/js/src/jsscript.c,v  <--  jsscript.c
new revision: 3.176; previous revision: 3.175
done
Checking in js/src/jsscript.h;
/cvsroot/mozilla/js/src/jsscript.h,v  <--  jsscript.h
new revision: 3.42; previous revision: 3.41
done
Checking in js/src/jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.207; previous revision: 3.206
done
Checking in caps/src/nsScriptSecurityManager.cpp;
/cvsroot/mozilla/caps/src/nsScriptSecurityManager.cpp,v  <--  nsScriptSecurityManager.cpp
new revision: 1.355; previous revision: 1.354
done


Note that on the first check in I have missed caps/src/nsScriptSecurityManager.cpp from the set of files to check in. It caused the red.

Comment 14

[Alfred Peng]

Attachment: patch to fix the bustage for dtrace

Build error on Solaris with DTrace enabled:
"../../../mozilla/js/src/jsdtracef.c", line 63: undefined struct/union member: clasp
"../../../mozilla/js/src/jsdtracef.c", line 63: improper member use: clasp
"../../../mozilla/js/src/jsdtracef.c", line 63: non-unique member requires struct/union pointer: name
"../../../mozilla/js/src/jsdtracef.c", line 63: left operand of "->" must be pointer to struct/union

igor, any suggestion on the patch?

Comment 15

[Igor Bukanov]

(In reply to comment #14)
> Created an attachment (id=311340) [details]
> patch to fix the bustage for dtrace

This is a regression so it is a good idea to always file a separated bug for it and attach files there. The "Clone This Bug" link at the right bottom corner of bugzilla's screen is an easy way to do it. One just need to edit the title and change the cloned bug to block the one that introduced the regression, not to depend on it.

This is how I filed bug 424750. Could you add the patch there?

Comment 16

[Igor Bukanov]

I suggest to back out the patch and make the bug wontfix. The bug 425596 has shown that my assumption that it was possible to make JSFunction equivalent to JSObject without breking the public API does not hold. 

Comment 17

[Igor Bukanov]

I backed out the patch.

Comment 18

[Igor Bukanov]

Attachment: what was backed out

This is for references.

Comment 19

[Igor Bukanov]

This optimization became irrelevant in the view of bug 423874.