The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nbp, Assigned: nbp)

Tracking

Other Branch
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

5 years ago
Created attachment 619807 [details] [diff] [review]
Check if shape is writable before registering IC.
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
Attachment #619807 - Flags: review?(dvander)
(Assignee)

Comment 2

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

Comment 3

5 years ago
Created attachment 620135 [details] [diff] [review]
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.
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
(Assignee)

Comment 6

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