Closed Bug 750582 Opened 12 years ago Closed 12 years ago

IonMonkey: eager write on global non-writable properties. (jaeger/bug584647.js:2: Error: Assertion failed: got false, expected true)

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(1 file, 1 obsolete file)

This bug can be reproduced with

./js --ion-eager ./jit-test/tests/jaeger/bug584647.js
./jit-test/tests/jaeger/bug584647.js:2: Error: Assertion failed: got false, expected true
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
Attachment #619807 - Flags: review?(dvander)
Another similar issue which implies changes of set-property (JSOP_SETNAME) code is:

./js --ion-eager ./jit-test/tests/jaeger/bug735161.js => ./jit-test/tests/jaeger/bug735161.js:4: Error: Assertion failed: got true, expected false

As it look similar and modify similar code I am adding the changes to the current patch in addition to the suggestion to clean-up SetPropertyCache made by dvander on IRC.
* Check if the shape is writable.
* Clean-up SetPropertyCache and extract isSetPropertyInlinable and
  isPropertyInlinable, rename the other predicate to 
  isAddPropertyInlinable to clarify.
* Add VMFunction to handle the bottom of SetPropertyOperation and use
  it inside SetPropertyCache to fix wrong use of SETNAME on the global 
  object.
Attachment #619807 - Attachment is obsolete: true
Attachment #619807 - Flags: review?(dvander)
Attachment #620135 - Flags: review?(dvander)
Comment on attachment 620135 [details] [diff] [review]
Remove unexpected properties from the global object.

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

Thanks for doing this refactoring! This is much more readable than the existing code.

::: js/src/ion/IonCaches.cpp
@@ +449,5 @@
>      return true;
>  }
>  
>  static bool
> +isPropertyInlinable(JSObject *obj, IonCacheSetProperty &cache)

nit: IsPropertyInlineable

@@ +466,5 @@
> +    return true;
> +}
> +
> +static bool
> +isSetPropertyInlinable(JSContext *cx, JSObject *obj, JSAtom *atom, jsid *pId, const Shape **pShape)

nit: IsPropertySetInlineable

@@ +492,5 @@
> +}
> +
> +static bool
> +isAddPropertyInlinable(JSContext *cx, JSObject *obj, jsid id, uint32_t oldSlots,
> +                       const Shape **pShape)

nit: IsPropertyAddInlineable

@@ +527,5 @@
>      return true;
>  }
>  
>  bool
> +js::ion::SetPropertyCache(JSContext *cx, size_t cacheIndex, JSObject *obj, const Value& value,

We should probably make this gc-safe while we're here: change |JSObject *obj| to be |HandleObject obj| and |const Value &value| to be |HandleValue value|.

::: js/src/ion/VMFunctions.cpp
@@ +288,5 @@
>  }
>  
> +bool
> +SetProperty(JSContext *cx, JSObject *obj, JSAtom *atom, const Value &value,
> +                       bool strict, bool isSetName)

nit: bool should align with JSContext

@@ +292,5 @@
> +                       bool strict, bool isSetName)
> +{
> +    Value v = value;
> +    jsid id = ATOM_TO_JSID(atom);
> +    RootObject objRoot(cx, &obj);

|obj| isn't rooted, so we can't use RootObject (wow these names are confusing!)

Instead, change |JSObject *obj| to |HandleObject obj|.
Attachment #620135 - Flags: review?(dvander) → review+
Err -- sorry, we actually *can* use RootObject there, but I think we're supposed to propagate HandleObject instead
https://hg.mozilla.org/projects/ionmonkey/rev/68777651999a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: