1032956 - Self-hosted functions in {Object,Function}.{,prototype.}* are broken and fail on an assert

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nathan Braswell [:nathan]]

Attachment: reorderPartialFix.patch - The reordering patch - not a final solution

Trying to self-host a function in {Object,Function}.{,prototype.}* cause the engine to fail the assertion JS_ASSERT(!getSlot(INTRINSICS).isUndefined());
in js/src/vm/GlobalObject.h

This happens because the INTRINSICS slot isn't set up until the post-initialization hook, which happens after we try to set up the self-hosted functions. (This is in GlobalObject::resolveConstructor)

One partial solution is to reorder this so that the post-initialization happens earlier, but there's something wrong with doing initialization after a post-initilization hook. Anyway, this doesn't fix it for Function, just Object.

Attached is a patch that does the reordering (just to test) and a patch origionally made by Waldo (in bug 937855) that includes a simple Object self hosted function for testing. I also added a commented out line in jsfun.cpp that does the same thing.

Comment 1

[Nathan Braswell [:nathan]]

Attachment: testObjectFunctionSelfHost.patch - Adds a trivial self-hosted function to Object (and commented out for Function) for testing

Comment 2

[Till Schneidereit [:till]]

I think it'd be better to delay installing the self-hosted functions for Function and Object. In bug 825199 comment 2 I described how that could work. Essentially, you just create a second set of JSFunctionSpec[] lists and apply it at the end of FinishObjectClassInit, after the intrinsicsHolder has been set.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 8448882 [details] [diff] [review]
reorderPartialFix.patch - The reordering patch - not a final solution

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

Hmm, yeah.  I agree with till that doing this as a post-processing step in FinishObjectClassInit is better than disrupting the entire init path.  Looks like you can do that just after the JSObject::defineProperty(cx, intrinsicsHolder, cx->names().global bit in jsobj.cpp.

Comment 4

[Nathan Braswell [:nathan]]

Attachment: selfHostObjectForReview.patch

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8450441 [details] [diff] [review]
selfHostObjectForReview.patch

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

::: js/src/builtin/Object.h
@@ +15,5 @@
>  
>  extern const JSFunctionSpec object_methods[];
>  extern const JSPropertySpec object_properties[];
>  extern const JSFunctionSpec object_static_methods[];
> +extern const JSFunctionSpec object_self_hosted_methods[];

object_static_selfhosted_methods, please.  "static" most important part, very mild preference for selfhosted as being more readable as one word in under_score_naming_style.

::: js/src/jsobj.cpp
@@ +169,5 @@
>      }
>  
> +    /* Define self-hosted functions after setting the intrinsics holder
> +     * (which is needed to define self-hosted functions)
> +     */

/*
 * Define self-hosted...
 * (which...
 */

is the right style for multi-line /**/-style comments.  Alternatively, you could do

// Define self-hosted...
// (which...

if you don't want to burn two more lines.  Doesn't matter either way to me.

Comment 6

[Nathan Braswell [:nathan]]

Attachment: newJSFunctionSpecArray_v2.patch

Fixed stuff up from review.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8451807 [details] [diff] [review]
newJSFunctionSpecArray_v2.patch

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

I'll push this shortly.

::: js/src/builtin/Object.cpp
@@ +274,5 @@
>              comma = true;
>              if (gsop[j]) {
>                  if (!buf.append(gsop[j]) || !buf.append(' '))
>                      return nullptr;
> +            }

As far as I can tell, this hunk removes a line, then inserts the exact same line again.  Dunno what's up with that.  Fixed locally to remove the trailing space that's currently there on trunk, which is I guess what you (or your editor) wanted to do.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/923774f79869