Last Comment Bug 489623 - JSOP_SETPROP does not run resolve hook when adding properties
: JSOP_SETPROP does not run resolve hook when adding properties
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Kannan Vijayan [:djvj]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-04-22 11:14 PDT by Igor Bukanov
Modified: 2012-05-04 11:40 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Quick fix (976 bytes, patch)
2012-04-12 15:03 PDT, Kannan Vijayan [:djvj]
no flags Details | Diff | Splinter Review
Patch v2 (1.74 KB, patch)
2012-04-16 16:18 PDT, Kannan Vijayan [:djvj]
jwalden+bmo: review+
Details | Diff | Splinter Review

Description Igor Bukanov 2009-04-22 11:14:28 PDT
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.
Comment 1 Kannan Vijayan [:djvj] 2012-04-12 15:03:41 PDT
Created attachment 614579 [details] [diff] [review]
Quick fix

I think this fixes it, but not sure.  Can someone confirm?
Comment 2 Igor Bukanov 2012-04-13 01:16:52 PDT
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.
Comment 3 Jason Orendorff [:jorendorff] 2012-04-13 14:13:21 PDT
Comment on attachment 614579 [details] [diff] [review]
Quick fix

That looks right to me, but I know nothing about Ion, so how about dvander?
Comment 4 Jason Orendorff [:jorendorff] 2012-04-13 14:42:17 PDT
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.
Comment 5 Kannan Vijayan [:djvj] 2012-04-16 16:18:29 PDT
Created attachment 615528 [details] [diff] [review]
Patch v2
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2012-04-25 17:45:25 PDT
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.
Comment 7 Kannan Vijayan [:djvj] 2012-04-27 11:53:32 PDT
Changed, committed, and pushed.

https://hg.mozilla.org/projects/ionmonkey/rev/fcbb7e877c12
Comment 8 Kannan Vijayan [:djvj] 2012-04-27 12:39:16 PDT
Causes regression in v8-bench.  Segfaults on raytrace and splay.
Comment 9 Paul Wright 2012-04-30 11:22:32 PDT
This was "resubmitted after fix" as rev 1b954a5da88c, but AWFY seems to indicate the problem still exists.  Can you confirm?
Comment 10 Kannan Vijayan [:djvj] 2012-04-30 15:12:06 PDT
Confirmed.  It's backed out again.
Comment 11 Kannan Vijayan [:djvj] 2012-05-04 11:40:57 PDT
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

Note You need to log in before you can comment on or make changes to this bug.