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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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;
}

Comment 1

[Eric Faust [:efaust]]

This is me. Looking into it.

Comment 2

[Eric Faust [:efaust]]

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

Comment 3

[Eric Faust [:efaust]]

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.

Comment 4

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

Eric, is this something we need to fix for Firefox 18?

Comment 5

[Eric Faust [:efaust]]

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?

Comment 6

[Eric Faust [:efaust]]

Attachment: Fix: Once and for all.

Fixed to cover both this and the Object.watch case.

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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.

Comment 8

[Kannan Vijayan [:djvj]]

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

Comment 9

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/298ef4e74849

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/298ef4e74849

Comment 11

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/ion/bug792166-1.js.