IonMonkey: Assertion failure: !isOwn, at ion/IonBuilder.cpp:5509

RESOLVED FIXED in mozilla18

Status

()

--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: decoder, Assigned: efaust)

Tracking

(Blocks: 2 bugs, {assertion, regression, testcase})

Trunk
mozilla18
assertion, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [jsbugmon:update][ion:p1:fx18])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
The following testcase asserts on mozilla-central revision e4757379b99a (run with --ion-eager):


Object.defineProperty(Object.prototype, 'x', { 
    set: function() {} 
});
var obj = {};
for (var i = 0; i < 100 ; ++i) {
    obj.x = 1;
    delete obj.x;
}
(Reporter)

Updated

6 years ago
Blocks: 724444
Whiteboard: [jsbugmon:update]
Summary: Assertion failure: !isOwn, at ion/IonBuilder.cpp:5509 → IonMonkey: Assertion failure: !isOwn, at ion/IonBuilder.cpp:5509
(Assignee)

Comment 1

6 years ago
This is me. Looking into it.
Assignee: general → efaustbmo
Status: NEW → ASSIGNED
(Assignee)

Comment 2

6 years ago
deleting the property calls AddTypePropertyId(..., Unknown), which marks the typeset as own, which is wrong in the case of an inherited accessor.
(Assignee)

Comment 3

6 years ago
In general, I think I am most interested in this case:

Object.defineProperty(Object.prototype, 'x', { 
    set: function() {print("got here");} 
});
var obj = {}; 
Object.defineProperty(obj, 'x', { 
    set : function() {print("got here instead");},
    configurable : true
});
for (var i = 0; i < 100 ; ++i) {
    obj.x = 1;
    // After the first deletion, we should be able to optimize to the function
    // on Object.prototype again right?
    delete obj.x;
}

TI should be able to keep track of what's actually going on in the face of deletions in the presence of inherited accessors.
Eric, is this something we need to fix for Firefox 18?
(Assignee)

Comment 5

6 years ago
Yeah, it just doesn't work in the face of |delete|. What's worse is that if affects correctness in optimized builds. I will generate a patch early tomorrow morning and put it up for review. Who should even review it? You?
(Assignee)

Comment 6

6 years ago
Created attachment 663425 [details] [diff] [review]
Fix: Once and for all.

Fixed to cover both this and the Object.watch case.
Attachment #663425 - Flags: review?(kvijayan)
Whiteboard: [jsbugmon:update] → [jsbugmon:update][ion:p1:fx18]
Here's another testcase for you to land:

try {
  __defineGetter__("eval", function() {
	this["__proto__"]
  })
  delete this["__proto__"]
  this["__proto__"]
} catch (e) {}
eval

also asserts js shell on m-c changeset f0c89cebf913 with --ion-eager.
Blocks: 349611
Keywords: regression
OS: Linux → All
Hardware: x86 → All
Comment on attachment 663425 [details] [diff] [review]
Fix: Once and for all.

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

Looks good.

::: js/src/ion/IonBuilder.cpp
@@ +5454,2 @@
>  
>          // Check here to make sure that everyone has Type Objects which known

nit: I assume you meant ".. Type Objects with known properties ..", not "which known properties".
Attachment #663425 - Flags: review?(kvijayan) → review+
https://hg.mozilla.org/mozilla-central/rev/298ef4e74849
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(Reporter)

Comment 11

6 years ago
A testcase for this bug was automatically identified at js/src/jit-test/tests/ion/bug792166-1.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.