Closed Bug 474501 Opened 11 years ago Closed 11 years ago

JSON literals shouldn't have prototype setters run during compilation


(Core :: JavaScript Engine, defect, P1)






(Reporter: bent.mozilla, Assigned: Waldo)



(Keywords: dev-doc-complete, fixed1.9.1, Whiteboard: fixed-in-tracemonkey)


(1 file, 4 obsolete files)

See bug 375250 comment 19. JSON literals shouldn't have prototype setters run during compilation. This bug blocks bug 375250 and has been deemed a P1 priority for beta 3.

Assigning to waldo per offline discussion.
Flags: blocking1.9.1?
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P1
No longer blocks: 375250
Jeff, as soon as you have an estimate on what it's going to take to resolve
this, can you update?  Just trying to get a realistic estimate on beta 3 wrap
I think this can be done by the end of the week without much trouble; biggest thing to figure out is exactly where to munge existing APIs to handle property-setting in object literals, because right now we go through several levels of nesting to finally get to the one place where properties are set, and I'm not where where the best place to insert a split is.
Attached patch Et voilà (obsolete) — Splinter Review
SPROP_SET stuff is left over from an earlier version of the patch; it's not relevant any more, but it is righteous and harmless.  I do, however, question whether it's really a worthwhile method to inline -- wouldn't be surprised if compilers ignored that.

Is there a reason we don't have JSPROP_DEFAULT_ATTRS yet?  The bare JSPROP_ENUMERATE is cringe-worthy.
Attachment #359844 - Flags: review?(brendan)
Comment on attachment 359844 [details] [diff] [review]
Et voilà

>+                if (!OBJ_DEFINE_PROPERTY(cx, obj, id, rval, NULL, NULL, 0,


>+                                         NULL)) {

Feel free to avoid wrapping and bracing, given new tw=99 standard.

>+            if (!OBJ_DEFINE_PROPERTY(cx, obj, id, rval, NULL, NULL, 0, NULL))

Ditto (this is JSOP_ARRAYPUSH).

>-                return !!SPROP_SET(cx, sprop, obj, pobj, vp);
>+                return SPROP_SET(cx, sprop, obj, pobj, vp);

Taras take note -- we don't need !! here.

> #define SPROP_GET(cx,sprop,obj,obj2,vp)                                       \
>     (((sprop)->attrs & JSPROP_GETTER)                                         \
>      ? js_InternalGetOrSet(cx, obj, (sprop)->id,                              \
>                            OBJECT_TO_JSVAL((sprop)->getter), JSACC_READ,      \
>                            0, 0, vp)                                          \
>      : (sprop)->getter(cx, OBJ_THIS_OBJECT(cx,obj), SPROP_USERID(sprop), vp))

Please do SPROP_GET as a static inline helper, since you are doing SET.

>- * NB: SPROP_SET must not be called if (SPROP_HAS_STUB_SETTER(sprop) &&
>- * !(sprop->attrs & JSPROP_GETTER)).
>- */
>-#define SPROP_SET(cx,sprop,obj,obj2,vp)                                       \
>-    (((sprop)->attrs & JSPROP_SETTER)                                         \
>-     ? js_InternalGetOrSet(cx, obj, (sprop)->id,                              \
>-                           OBJECT_TO_JSVAL((sprop)->setter), JSACC_WRITE,     \
>-                           1, vp, vp)                                         \
>-     : ((sprop)->attrs & JSPROP_GETTER)                                       \
>-     ? (JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,                    \
>-                             JSMSG_GETTER_ONLY, NULL), JS_FALSE)              \
>-     : (sprop)->setter(cx, OBJ_THIS_OBJECT(cx,obj), SPROP_USERID(sprop), vp))
>+static JS_INLINE JSBool
>+SPROP_SET(JSContext* cx, JSScopeProperty* sprop, JSObject* obj, JSObject* obj2,
>+          jsval* vp)
>+                !(sprop->attrs & JSPROP_GETTER)));

Comment still applies but I guess the assertion speaks for itself.

Nice, I bless the ride-along change to inline SPROP_SET (and GET) and pray they don't backfire on some lame platform.

r=me with above addressed.

Attachment #359844 - Flags: review?(tglek)
Attachment #359844 - Flags: review?(brendan)
Attachment #359844 - Flags: review+
Bleh, had to back out because including __proto__ in an object literal will then set the wrong property attributes on it.  Still thinking about how to make that work correctly...
Whiteboard: fixed-in-tracemonkey
Sorry, should have seen this coming. Settable __proto__ is something to remove, or at least confine. The object initialiser occurrence of __proto__:... is one confined form (no cycle can be created; useful for one-off prototype "beget" use-cases).

Best fix is to notice in the emitter that the property id is protoAtom and emit JSOP_DUP;JSOP_SETPROP;JSOP_POP instead of JSOP_INITPROP.

(In reply to comment #7)
> Sorry, should have seen this coming. Settable __proto__ is something to remove,

I mean from the language! Just not yet. Another JS2 or Mozilla 2 breaking change.


js> function() { var x = {x setter: 5}; };
function () {
    ({.x = 5);
    var x = {};

I may need to add countAtom to the specials list before this is all done, but I'm not familiar enough with it, its purpose, or its function to say for sure.

This fails a bunch of decompiler tests but otherwise, I think, may be okay.
Attachment #359844 - Attachment is obsolete: true
Attachment #359844 - Flags: review?(tglek)
__parent__ is read-only, no need to special-case it. Same with __count__.

Why change anything todo with setters or getters?

Attached patch Archival copy of latest version (obsolete) — Splinter Review
I had a feeling slowifying setters and getters would be a simplification, but that turned out to be wrong.  Here's what I have now, but given this requires decompiler hacking, it seems the easy way forward for now is just to split in JSOP_INITPROP, so doing that momentarily...
Attachment #360245 - Attachment is obsolete: true
Attached patch Just split in JSOP_INITPROP (obsolete) — Splinter Review
I don't believe we need to do anything special for tracing __proto__, because the entry passed to record_SetPropMiss should route us through js_AddProperty and do the right thing.  My minimal tests of creating an object literal in a loop seemed to verify this, at least.
Attachment #360413 - Flags: review?(brendan)
Comment on attachment 360413 [details] [diff] [review]
Just split in JSOP_INITPROP

>@@ -6224,31 +6229,31 @@ js_Interpret(JSContext *cx)
>              * an object initialiser, not an array initialiser).
>              */
>             if (!js_CheckRedeclaration(cx, obj, id, JSPROP_INITIALIZER, NULL,
>                                        NULL)) {
>                 goto error;
>             }
>             /*
>-             * If rval is a hole, do not call OBJ_SET_PROPERTY. In this case,
>+             * If rval is a hole, do not call OBJ_DEFINE_PROPERTY. In this case,
>              * obj must be an array, so if the current op is the last element
>              * initialiser, set the array length to one greater than id.
>              */
>             if (rval == JSVAL_HOLE) {
>                 JS_ASSERT(OBJ_IS_ARRAY(cx, obj));
>                 JS_ASSERT(JSID_IS_INT(id));
>                 JS_ASSERT((jsuint) JSID_TO_INT(id) < ARRAY_INIT_LIMIT);
>                 if ((JSOp) regs.pc[JSOP_INITELEM_LENGTH] == JSOP_ENDINIT &&
>                     !js_SetLengthProperty(cx, obj,
>                                           (jsuint) (JSID_TO_INT(id) + 1))) {
>                     goto error;
>                 }
>             } else {
>-                if (!OBJ_SET_PROPERTY(cx, obj, id, &rval))
>+                if (!OBJ_DEFINE_PROPERTY(cx, obj, id, rval, NULL, NULL, JSPROP_ENUMERATE, NULL))
>                     goto error;

Nice minimal patch -- but to make it look better, pull that js_SetLengthProperty param up and lose the braces. Could also hit js_CheckRedeclaration call. I'm in favor, but the conservative alternative is to wrap OBJ_DEFINE_PROPERTY before 80 and brace its goto error. Up to you.

>+#ifdef __cplusplus /* LiveConnect, you suck. */

Better to avoid insults and just cite a bug in a FIXME: -- jst knows the bug number I'm sure. Oh, found it: bug 442399.

r=me with above addressed.

Attachment #360413 - Flags: review?(brendan) → review+
Backed out.

This patch seriously changes our trace stats in DBG shell:

recorder: started(3661), aborted(2715), completed(1473), different header(20), trees trashed(4), slot promoted(0), unstable loop variable(157), breaks(2), returns(14), unstableInnerCalls(71)
monitor: triggered(3247325), exits(3247092), type mismatch(0), global mismatch(24)

Without the patch:

recorder: started(1332), aborted(381), completed(1422), different header(20), trees trashed(4), slot promoted(0), unstable loop variable(142), breaks(2), returns(14), unstableInnerCalls(71)
monitor: triggered(367813), exits(367575), type mismatch(0), global mismatch(24)

This needs some investigation.
Attachment #360346 - Attachment is obsolete: true
Attachment #360413 - Attachment is obsolete: true
Attachment #360678 - Flags: review?(brendan)
Comment on attachment 360678 [details] [diff] [review]
Wrong shape => cache miss every time, cf. js_SetPropertyHelper

>+    uint32_t shape = OBJ_SHAPE(obj);

Stick with uint32 -- we don't need incremental style mixing where it doesn't pay (contrast with bool).

I should have caught this, since js_SetPropertyHelper has the same need. r=me with _t elimination.

Attachment #360678 - Flags: review?(brendan) → review+
In TM with that change (finally):
Whiteboard: fixed-in-tracemonkey
Closed: 11 years ago
Resolution: --- → FIXED
tagged doc needed; waldo says to wait until he blogs about it on webtech.
As far as I can tell, this never got blogged about.  I'm still waiting before I write this up.
Yikes, forgot to CC you on bug 485645 -- done that now.
This is now mentioned in all the appropriate places that I could find. While I was at it, I also did some cleanup work on:
Duplicate of this bug: 470041
You need to log in before you can comment on or make changes to this bug.