The default bug view has changed. See this FAQ.

JSOP_SETPROP does not run resolve hook when adding properties

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: Igor Bukanov, Assigned: djvj)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

8 years ago
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.
(Reporter)

Updated

5 years ago
Assignee: igor → general
(Assignee)

Updated

5 years ago
Assignee: general → kvijayan
(Assignee)

Comment 1

5 years ago
Created attachment 614579 [details] [diff] [review]
Quick fix

I think this fixes it, but not sure.  Can someone confirm?
Attachment #614579 - Flags: review?(igor)
(Reporter)

Comment 2

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

Comment 5

5 years ago
Created attachment 615528 [details] [diff] [review]
Patch v2
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+
(Assignee)

Comment 7

5 years ago
Changed, committed, and pushed.

https://hg.mozilla.org/projects/ionmonkey/rev/fcbb7e877c12
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 8

5 years ago
Causes regression in v8-bench.  Segfaults on raytrace and splay.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 9

5 years ago
This was "resubmitted after fix" as rev 1b954a5da88c, but AWFY seems to indicate the problem still exists.  Can you confirm?
(Assignee)

Comment 10

5 years ago
Confirmed.  It's backed out again.
(Assignee)

Comment 11

5 years ago
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:

https://hg.mozilla.org/projects/ionmonkey/rev/9d1cb3806f20
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.