Stop exposing compile-time function objects

RESOLVED WONTFIX

Status

()

--
enhancement
RESOLVED WONTFIX
11 years ago
11 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

11 years ago
Created attachment 310994 [details] [diff] [review]
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.
(Assignee)

Comment 1

11 years ago
Created attachment 311070 [details] [diff] [review]
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.
Attachment #310994 - Attachment is obsolete: true
Attachment #311070 - Flags: review?(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
Attachment #311070 - Flags: review?(brendan) → review+
(Assignee)

Comment 3

11 years ago
Created attachment 311135 [details] [diff] [review]
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.
Attachment #311070 - Attachment is obsolete: true
Attachment #311135 - Flags: review+
Attachment #311135 - Flags: approval1.9b5?
That bug is no longer a b5 blocker - is this still super-safe and absolutely necessary to take for b5?
(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

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

Comment 7

11 years ago
(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.
Anyone able to apply to a clean tree and help mochitest?

/be
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.
(Assignee)

Comment 10

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

Which tests have failed?
Unfortunately, I lost the tests that failed when I tried to run them individually.
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.
Attachment #311135 - Flags: approval1.9b5? → approval1.9b5+
(Assignee)

Comment 13

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

Updated

11 years ago
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 14

11 years ago
Created attachment 311340 [details] [diff] [review]
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?
Attachment #311340 - Flags: review?(igor)
(Assignee)

Updated

11 years ago
Blocks: 424750
(Assignee)

Updated

11 years ago
No longer blocks: 424750
Depends on: 424750

Updated

11 years ago
Attachment #311340 - Attachment is obsolete: true
Attachment #311340 - Flags: review?(igor)
(Assignee)

Comment 15

11 years ago
(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?

Updated

11 years ago
Flags: in-testsuite-
Flags: in-litmus-
(Assignee)

Updated

11 years ago
Depends on: 425596
(Assignee)

Comment 16

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

Comment 17

11 years ago
I backed out the patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 18

11 years ago
Created attachment 312384 [details] [diff] [review]
what was backed out

This is for references.
(Assignee)

Updated

11 years ago
Attachment #311135 - Attachment is obsolete: true
Attachment #311135 - Flags: review+
Attachment #311135 - Flags: approval1.9b5+
(Assignee)

Updated

11 years ago
No longer depends on: 423874
(Assignee)

Updated

11 years ago
No longer blocks: 418443
(Assignee)

Comment 19

11 years ago
This optimization became irrelevant in the view of bug 423874.
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.