The infrastructure for self-hosted JavaScript implemented in bug 462300 doesn't support a number of common JavaScript constructs within self-hosted functions: function Func1() { var alpha = "[a-zA-Z]", digit = "[0-9]", alphanum = "(" + alpha + "|" + digit + ")", singleton = "(" + digit + "|[A-WY-Za-wy-z])", duplicateSingleton = "-" + singleton + "-(.*-)?\\1(?!" + alphanum + ")"; // create an instance of a built-in constructor var duplicateSingletonRE = new RegExp(duplicateSingleton, "i"); } → TypeError: undefined is not a constructor function Func2() { return {}; // create an object to return } → Assertion failure: JS_ObjectIsFunction(__null, this), at /Users/standards/Mozilla/self-hosted/js/src/jsfun.h:208 function Func3() { // create an array as a separate step so that I can add elements // before returning it result = []; return result; } → Assertion failure: *op == JSOP_NAME, at /Users/standards/Mozilla/self-hosted/js/src/frontend/BytecodeEmitter.cpp:1180

(In reply to Norbert Lindenberg from comment #0) > function Func3() { > // create an array as a separate step so that I can add elements > // before returning it > result = []; > return result; > } > > → Assertion failure: *op == JSOP_NAME, at > /Users/standards/Mozilla/self-hosted/js/src/frontend/BytecodeEmitter.cpp:1180 This is bug 784293. I'll have to investigate the others to see what's going on there.

Another one: function Func4(collator) { var desc = []; desc.value = true; Object.defineProperty(collator, "__initializedCollator", desc); } → TypeError: (intermediate value).defineProperty is not a function

This patch enables initialization of the self-hosting global with standard classes and their being automatically cloned into the current global's intrinsics holder upon first access. I hope this approach is workable as it makes setting up the self-hosting global a great deal easier than having to manually define all of the required native builtins.

This adds support for cloning object literals as part of cloning interpreted functions. Passes tests and is currently try-servering here: http://tbpl.mozilla.org/?tree=Try&rev=a9bfe71be16f

Comment on attachment 656406 [details] [diff] [review] part 2: Clone object literals within functions Review of attachment 656406 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsobj.cpp @@ +2852,5 @@ > return clone; > } > > +JSObject * > +js::CloneObjectLiteral(JSContext *cx, HandleObject enclosingScope, HandleObject srcObj) s/enclosingScope/parent, see below. @@ +2854,5 @@ > > +JSObject * > +js::CloneObjectLiteral(JSContext *cx, HandleObject enclosingScope, HandleObject srcObj) > +{ > + Rooted<TypeObject*> typeObj(cx, srcObj->type()); It looks like this will be using the same type as the original object, which will be from a different compartment if the CloneScript is cross-compartment. I think for this you'll need to lookup Object.prototype in the target global and pass its getNewType type. @@ +2855,5 @@ > +JSObject * > +js::CloneObjectLiteral(JSContext *cx, HandleObject enclosingScope, HandleObject srcObj) > +{ > + Rooted<TypeObject*> typeObj(cx, srcObj->type()); > + RootedShape shape(cx, srcObj->addressOfShape()->get()); srcObj->lastProperty() @@ +2856,5 @@ > +js::CloneObjectLiteral(JSContext *cx, HandleObject enclosingScope, HandleObject srcObj) > +{ > + Rooted<TypeObject*> typeObj(cx, srcObj->type()); > + RootedShape shape(cx, srcObj->addressOfShape()->get()); > + RootedObject obj(cx); obj is unused @@ +2857,5 @@ > +{ > + Rooted<TypeObject*> typeObj(cx, srcObj->type()); > + RootedShape shape(cx, srcObj->addressOfShape()->get()); > + RootedObject obj(cx); > + return NewReshapedObject(cx, typeObj, enclosingScope, srcObj->getAllocKind(), shape); Looking at NewReshapedObject, it is unfortunately not very robust and won't deal with getters/setters in object literals correctly, i.e. x = { get f() { print("What"); } } I think that what will happen is the cloned object will just have a normal data property f and will lose the getter/setter, which is probably fine for your purposes here but might deserve to get fixed in NewReshapedObject and should at least have a comment and tests. ::: js/src/jsscript.cpp @@ +2104,5 @@ > + enclosingScope = objects[FindBlockIndex(src, enclosingScope->asStaticBlock())]; > + else > + enclosingScope = fun; > + > + clone = CloneObjectLiteral(cx, enclosingScope, innerObj); The enclosingScope logic here is unnecessary; the object here will be of Object class and cannot appear on scope chains. The parent passed to CloneObjectLiteral should just be the new script's global object.

part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/1442613953ba

Sorry, I backed this out for mochitest-2 and xpcshell test breakage. See https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=1442613953ba https://hg.mozilla.org/integration/mozilla-inbound/rev/dca93af4b72a

(In reply to Steve Fink [:sfink] from comment #7) > Sorry, I backed this out for mochitest-2 and xpcshell test breakage. See > https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=1442613953ba > > https://hg.mozilla.org/integration/mozilla-inbound/rev/dca93af4b72a Oops, sorry for the breakage. Will investigate and try-server before repushing.

This version addresses all of bhackett's feedback in comment 5. > @@ +2857,5 @@ > > +{ > > + Rooted<TypeObject*> typeObj(cx, srcObj->type()); > > + RootedShape shape(cx, srcObj->addressOfShape()->get()); > > + RootedObject obj(cx); > > + return NewReshapedObject(cx, typeObj, enclosingScope, srcObj->getAllocKind(), shape); > > Looking at NewReshapedObject, it is unfortunately not very robust and won't > deal with getters/setters in object literals correctly, i.e. > > x = { get f() { print("What"); } } > > I think that what will happen is the cloned object will just have a normal > data property f and will lose the getter/setter, which is probably fine for > your purposes here but might deserve to get fixed in NewReshapedObject and > should at least have a comment and tests. Turns out this is not the case: getters/setters are cloned just fine. The following correctly prints out "foo0", "foo1", ..., "fooN" when executing `test().prop` repeatedly and updates `test.i` correctly when executing, say `test().prop = 10`: function test() { if (!test.i) test.i = 0; var obj = {get prop() {return 'foo' + test.i++}, set prop(val){test.i = val}}; return obj; }

Turns out that the indexedDB implementation creates a thread-local JSRuntime with a very small heap size. Initializing all standard classes caused that heap to be blown, which in turn caused the test failures. I have reworked things towards two goals: - resolving the standard classes lazily in the self-hosting global - moving all intrinsics initialization from the globalObject->intrinsicsHolder object to the runtime's selfHostingGlobal The latter isn't exactly related to this, but it touches on some of the same pieces of code and was pretty straight-forward to do. It has the advantage of lazily cloning intrinsic functions into the current global upon first usage.

Try run for 7edab9260d93 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=7edab9260d93 Results (out of 277 total builds): exception: 4 success: 247 warnings: 25 failure: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tschneidereit@gmail.com-7edab9260d93

Try run for 04a2543a9fcc is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=04a2543a9fcc Results (out of 8 total builds): success: 8 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tschneidereit@gmail.com-04a2543a9fcc

(In reply to Till Schneidereit [:till] from comment #10) > Turns out that the indexedDB implementation creates a thread-local JSRuntime > with a very small heap size. Initializing all standard classes caused that > heap to be blown, which in turn caused the test failures. > > I have reworked things towards two goals: > - resolving the standard classes lazily in the self-hosting global I don't think it's worth the trouble just for the indexedDB JSRuntime: there is at most 1, it's only created when indexedDB is used, and a normal browser already has hundreds of globals. Could you just bump the heap size in the indexedDB code (preferably in it's own patch)? (Back story in case you're curious: I added the indexedDB JSRuntime during JSRuntime single-thread-ification; indexedDB wants to decode a structured-cloned object on another thread, read a single property (the 'key'), then abandon the object.) > - moving all intrinsics initialization from the globalObject->intrinsicsHolder object to the runtime's selfHostingGlobal This sounds good; could it be in its own patch?

Comment on attachment 661193 [details] [diff] [review] part 2: Clone object literals within functions, v2 Review of attachment 661193 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsscript.cpp @@ +2098,5 @@ > > clone = CloneInterpretedFunction(cx, enclosingScope, innerFun); > + } else { > + RootedObject innerObj(cx, &obj); > + clone = CloneObjectLiteral(cx, cx->global(), innerObj); My first reaction when reading this was "wait, XDRScript assumes there are only block and function objects, why do we have to handle object literals here?!". Chrome JS (which gets XDR'd into the startup cache) definitely uses object literals, after all. The reason I found was this: - we only emit object literals for the JSOP_NEWOBJECT opcode - JSOP_NEWOBJECT is an optimization of JSOP_NEWINIT that creates a right-sized object up-front by providing an object to clone - we only emit JSOP_NEWOBJECT for compileAndGo code - we only XDR/Clone non-compileAndGo code, with the exception of self-hosted code So that is why self-hosting hit this bug. Could you capture this reasoning in a comment in the else branch so that the reader knows why we have this asymmetry between CloneScript and XDRScript?

Reverts back to non-lazy resolution of intrinsic functions, requires part 0 to work.

I added the requested comment but would like a quick re-review as I'm only somewhat certain that I correctly described the situation. Note that self-hosted code isn't *really* compileAndGo, but some weird thing in-between. I very much look forward to compileAndGo going away (or, rather, being true for all code).

This is just the refactoring extracted from attachment 661603 [details] [diff] [review].

Try-server run for the entire patch queue: http://tbpl.mozilla.org/?tree=Try&rev=b0e52a4a5627

(In reply to Till Schneidereit [:till] from comment #17) > I very much look forward to compileAndGo going away (or, rather, > being true for all code). Me too, but I think the best we can do is have a *dynamically inferred* per-script bit that says "I am always run with the same global object I was compiled with and without any other non-global non-scope objects". A mouthful, but I think that is what we need to optimize JSOP_NAME to super-fast global access.

Oops, I forgot, we already have the "same global" property with CPG, so all we need to do is dynamically check whether there are any non-scope non-global objects on the scope chain; we only need to check this at a few pinchpoints: CloneFunction, ExecuteScript. (The bug for this is bug 679939, if you are interested :)

Comment on attachment 670363 [details] [diff] [review] Part 1: Make standard builtins and the current global available to self-hosted code, v3 Review of attachment 670363 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsapi.cpp @@ +5002,5 @@ > } > > + /* > + * During creation of the self-hosting global, we ignore all > + * self-hosted functions. I had to think a little about this. I think the reason is: because when we initialize the self-hosted global, we are about to *define* the self-hosting functions. My other thought was: well this doesn't seem like the right place to put this check; it seems like we'd have two lists of JSFunctionSpecs: the normal list and the self-hosted list and only call JS_DefineFunctions on the normal list for the self-hosting global. Is that possible?

Comment on attachment 670364 [details] [diff] [review] part 2: Clone object literals within functions, v3 Review of attachment 670364 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsscript.cpp @@ +2144,5 @@ > + * Clone object literals emitted for the JSOP_NEWOBJECT opcode. This case should > + * only ever be hit when cloning objects from self-hosted code, because that's the > + * only kind of code that gets cloned and that we emit JSOP_NEWOBJECT for instead > + * of the less-optimized JSOP_NEWINIT. XDRScript doesn't handle this case as we > + * don't XDR self-hosted code. Looks good, but I would like one mention to JSOPTION_COMPILE_N_GO so that way, when we grep for it when we remove COMPILE_N_GO we'll find this little wart (and have to think about it... it would be a big pain to XDR object literals...).

Comment on attachment 670365 [details] [diff] [review] part 3: Move all intrinsics initialization from GlobalObject to JSContext. Review of attachment 670365 [details] [diff] [review]: ----------------------------------------------------------------- So, this puts these intrinsics only on the SHG, that sounds good. Now, when we clone a self-hosted function into a global when its used, and that function uses one of these intrinsics, how does the self-hosted function find it? Is there a resolve hook somewhere?

This comment should hopefully cover all important aspects.

(In reply to Luke Wagner [:luke] from comment #24) > Comment on attachment 670365 [details] [diff] [review] > part 3: Move all intrinsics initialization from GlobalObject to JSContext. > > Review of attachment 670365 [details] [diff] [review]: > ----------------------------------------------------------------- > > So, this puts these intrinsics only on the SHG, that sounds good. Now, when > we clone a self-hosted function into a global when its used, and that > function uses one of these intrinsics, how does the self-hosted function > find it? Is there a resolve hook somewhere? For self-hosted code, we emit JSOP_GETINTRINSIC instead of JSOP_NAME or JSOP_GETGNAME. That way, all name lookups use JSRuntime::getSelfHostedFunction (which should be renamed in a fix for bug 784293).

(In reply to Luke Wagner [:luke] from comment #22) > Comment on attachment 670363 [details] [diff] [review] > Part 1: Make standard builtins and the current global available to > self-hosted code, v3 > > Review of attachment 670363 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jsapi.cpp > @@ +5002,5 @@ > > } > > > > + /* > > + * During creation of the self-hosting global, we ignore all > > + * self-hosted functions. > > I had to think a little about this. I think the reason is: because when we > initialize the self-hosted global, we are about to *define* the self-hosting > functions. My other thought was: well this doesn't seem like the right > place to put this check; it seems like we'd have two lists of > JSFunctionSpecs: the normal list and the self-hosted list and only call > JS_DefineFunctions on the normal list for the self-hosting global. Is that > possible? So you're saying we should split off self-hosted methods into their own lists for each builtin and don't install that list in the self-hosting global? While that is certainly doable, it would complicate the handling of self-hosted methods quite a bit and increase the number of places you have to care about whether a builtin is native or self-hosted. If you think the increase in cleanliness is worth it, then sure: let's do that. Maybe we should change the check to an assert then, to catch wrongly-installed functions as early as possible?

(In reply to Till Schneidereit [:till] from comment #27) No, I think you're right :)

(In reply to Till Schneidereit [:till] from comment #26) Of course, sorry to make you re-explain; it all got paged out :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc34d5773895 https://hg.mozilla.org/integration/mozilla-inbound/rev/c9e4a0971222 https://hg.mozilla.org/integration/mozilla-inbound/rev/a821bac3baa9 https://hg.mozilla.org/integration/mozilla-inbound/rev/f305661c1217

The only thing left to do here is adding support for 'undefined', AFAICT. For now, that has to be worked around using 'void 0'.

https://hg.mozilla.org/mozilla-central/rev/dc34d5773895 https://hg.mozilla.org/mozilla-central/rev/c9e4a0971222 https://hg.mozilla.org/mozilla-central/rev/a821bac3baa9 https://hg.mozilla.org/mozilla-central/rev/f305661c1217

(In reply to Till Schneidereit [:till] from comment #31) > The only thing left to do here is adding support for 'undefined', AFAICT. > For now, that has to be worked around using 'void 0'. 'undefined' has its own bug 785294. And yes, I've been successfully using 'void 0' for a while.

(In reply to Till Schneidereit [:till] from comment #30) > https://hg.mozilla.org/integration/mozilla-inbound/rev/dc34d5773895 > https://hg.mozilla.org/integration/mozilla-inbound/rev/c9e4a0971222 > https://hg.mozilla.org/integration/mozilla-inbound/rev/a821bac3baa9 > https://hg.mozilla.org/integration/mozilla-inbound/rev/f305661c1217 After pulling these changes (i.e., qparent = f305661c1217) and updating my sources to get rid of the GetBuiltIn workaround, I'm getting the following error message from SpiderMonkey: $ js self-hosted:115:0 ReferenceError: no intrinsic function ToObject: self-hosted:115:0 return %ToObject(v); self-hosted:115:0 ............^ The referenced %ToObject is the first call to an %intrinsic in my self-hosted file. I see that the intrinsics have moved to jscntxt.cpp and are now attached to the self-hosted global rather than the intrinsics holder - how are they found in the new location?

(In reply to Norbert Lindenberg from comment #34) > After pulling these changes (i.e., qparent = f305661c1217) and updating my > sources to get rid of the GetBuiltIn workaround, I'm getting the following > error message from SpiderMonkey: > > $ js > self-hosted:115:0 ReferenceError: no intrinsic function ToObject: > self-hosted:115:0 return %ToObject(v); > self-hosted:115:0 ............^ > > The referenced %ToObject is the first call to an %intrinsic in my > self-hosted file. > > I see that the intrinsics have moved to jscntxt.cpp and are now attached to > the self-hosted global rather than the intrinsics holder - how are they > found in the new location? Urgh, that looks like I lost a change during rebasing or something. This patch changes GlobalObject::hasIntrinsic to look for the intrinsics in the global object itself, instead of its intrinsics holder. With the new init code, that's where they get installed to. The intrinsics holder is used for holding cloned functions (both intrinsic and self-hosted) in all other globals.

(In reply to Till Schneidereit [:till] from comment #35) > Created attachment 671213 [details] [diff] [review] > Part 3b: fix GlobalObject::hasIntrinsic With this patch, intrinsics are found again. (In reply to Till Schneidereit [:till] from comment #30) > https://hg.mozilla.org/integration/mozilla-inbound/rev/a821bac3baa9 Object literals now work - I've been able to convert several hundred lines of code initializing arrays with non-indexed properties back to object literals.

The fix to make standard built-ins available to self-hosted functions doesn't seem to work - I still see failures similar (although not identical) to the ones in Func1 in the bug description and Func4 in comment 2. I'm attaching a test case to make it easier to verify the fix. To check, apply the patch and call Array.testBuiltIns(). The tests pass if I use my usual workaround, i.e., insert lines of the kind var Object = %GetBuiltIn("Object"); Without the workaround, they still fail.

Comment on attachment 671213 [details] [diff] [review] Part 3b: fix GlobalObject::hasIntrinsic https://hg.mozilla.org/integration/mozilla-inbound/rev/ccfda593ba94

After looking into the test cases from comment 37 some more, I realized that they all depend on the ability to deeply clone objects: The builtin classes are objects with prototypes and methods (and ctors), after all. Since, as discussed on IRC, deep cloning of such complex objects isn't likely to happen soon, the best workaround will probably be to add support for storing references to the builtins during initial interpretation of the self-hosted code and then using those. I'm optimistic that adding this support will be doable in the short term.

All problems listed here are fixed or sufficient workarounds have been developed.