Closed Bug 489623 Opened 13 years ago Closed 10 years ago

JSOP_SETPROP does not run resolve hook when adding properties


(Core :: JavaScript Engine, defect)

Not set





(Reporter: igor, Assigned: djvj)



(1 file, 1 obsolete file)

When checking in the debugger I have observed that JSOP_SETPROP does not run the resolve hook for the cached property additions. Consider the following script fragment: 

  obj.x = something

If x is not in obj but obj has a resolve hook that adds the property x (perhaps with custom flags) even when JSRESOLVE_ASSIGNING, then the cache treats such property addition as predictive one and caches it using the shape prior the addition. Thus, when the above fragment is executed for another object with that cached shape, JSOP_SETPROP just adds a new property ignoring the resolve hooks. 

This was probably not observed before since most resolved hooks do nothing on assignments.
Assignee: igor → general
Assignee: general → kvijayan
Attached patch Quick fix (obsolete) — Splinter Review
I think this fixes it, but not sure.  Can someone confirm?
Attachment #614579 - Flags: review?(igor)
Comment on attachment 614579 [details] [diff] [review]
Quick fix

I am not right person to review this. In particular, I have no idea about performance implication of this.
Attachment #614579 - Flags: review?(igor) → review?(jorendorff)
Comment on attachment 614579 [details] [diff] [review]
Quick fix

That looks right to me, but I know nothing about Ion, so how about dvander?
Attachment #614579 - Flags: review?(jorendorff) → review?(dvander)
Random nits:

- Once a property is found on any prototype, the loop can stop. A resolve hook on a prototype further along the chain won't matter.

- The thing to look for is probably JSPROP_SHARED, not a non-default setter (Waldo can say if this is correct).

- Inside the loop, the check for resolve hooks is only necessary when nativeLookup returns NULL. If there's already a property there, the resolve hook wouldn't be called anyway.
Attached patch Patch v2Splinter Review
Attachment #614579 - Attachment is obsolete: true
Attachment #615528 - Flags: review?(jwalden+bmo)
Attachment #614579 - Flags: review?(dvander)
Comment on attachment 615528 [details] [diff] [review]
Patch v2

Review of attachment 615528 [details] [diff] [review]:

Just formatting tweaks mostly, this looks fine otherwise, I believe.

::: js/src/ion/IonCaches.cpp
@@ +470,5 @@
>          // if prototype defines this property in a non-plain way, don't optimize
>          const Shape *protoShape = proto->nativeLookup(cx, propId);
> +        if (protoShape) {
> +            // If the prototype defines the property directly, and has a default setter

"defines" would read as referring to property definition, but that's not what matters here.  I tend to find the rest of the wording a bit confusing as well.  How about this:

If the prototype has a property with this name, we may be able to inline-add if the property has a default setter.  If there's no default setter, then setting would invoke unknown behavior (or at least behavior we're choosing not to model here), so we can't optimize this way.

@@ +477,5 @@
> +            // Otherwise, assume that the property add cannot be optimized.
> +            if (protoShape->hasDefaultSetter())
> +                return true;
> +            else
> +                return false;

This should just be |return protoShape->hasDefaultSetter();|.

@@ +478,5 @@
> +            if (protoShape->hasDefaultSetter())
> +                return true;
> +            else
> +                return false;
> +        } else {

In SpiderMonkey, when an if-block ends in a return, we don't wrap subsequent statements in an else-block, because the stuff in the else would execute even without the |else{}| surrounding that code.  This also has the benefit of keeping overall indentation lower.  So this should be:

if (...) {
  return protoShape->hasDefaultSetter();

// If prototype...

@@ +481,5 @@
> +                return false;
> +        } else {
> +            // If prototype has a non-default resolve hook, don't inline.
> +            // This is only checked when nativeLookup() above returns NULL
> +            // because any resolve hooks are masked by native property definitions.

This comment is perhaps over-informative.  I think it's evident by the very definition of the resolve hook and nativeLookup().  So I'd do it this way:

// Otherwise, if there's no such property, watch out for a resolve hook that would need to be invoked and thus would prevent inlining property addition.
Attachment #615528 - Flags: review?(jwalden+bmo) → review+
Changed, committed, and pushed.
Closed: 10 years ago
Resolution: --- → FIXED
Causes regression in v8-bench.  Segfaults on raytrace and splay.
Resolution: FIXED → ---
This was "resubmitted after fix" as rev 1b954a5da88c, but AWFY seems to indicate the problem still exists.  Can you confirm?
Confirmed.  It's backed out again.
I checked in a slightly more conservative version of this patch that doesn't try to shortcut the prototype search path.

Passes jstests, and doesn't crash awfy v8bench.

Third time's the charm:
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.