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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

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

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Check if shape is writable before registering IC.

Comment 2

[Nicolas B. Pierron [:nbp]]

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.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: Remove unexpected properties from the global object.

* 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.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

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|.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Err -- sorry, we actually *can* use RootObject there, but I think we're supposed to propagate HandleObject instead

Comment 6

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/projects/ionmonkey/rev/68777651999a