728188 - IonMonkey: Assertion failure: obj, at ../../jsval.h:534

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

The following testcase crashes with --ion-eager -n:
--
delete String.prototype.indexOf;

function enterFunc (funcName) {
    funcName.indexOf();
}
enterFunc(new Array("foo"));
enterFunc(new String("Foo"));
--
The problem is that the GetPropertyCacheT for funcName.indexOf has known "object" return type, but when enterFunc is called again the return type becomes "undefined" and the script is invalidated by the Monitor call. However, the bailout mechanism still thinks the return type must be "object" so we try to construct an invalid Value with "object" type tag and "undefined" payload.

Noticed this when investigating bug 728187.

Comment 1

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

Yuck. The problem is that GetPropertyCacheT can actually return a Value, we've just made the unbox operation implicit. I think the easiest way to fix this is to get rid of GetPropertyCacheT, and always have explicit unboxing. Then we can safely bail out in between the getprop and the unbox.

Brian, does that sound okay?

Comment 2

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

Attachment: potential fix

Not sure if this is the right thing yet, but fixes the bug.

Comment 3

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

Attachment: alternate fix

This patch adds more code and is grosser than I hoped, since I had to work around various asserts. I'm not against this facility but it would be nice if we didn't need it.

Brian, how much do you think preserving GetPropertyCacheT will matter? Could we get a microbenchmark that has a lot of register pressure?

Comment 4

[Brian Hackett [Laid off!]]

Something like this should have more register pressure (assuming m/n/p are hoisted into registers):

function foo(x, m, n, p) {
  var res = 0;
  for (var i = 0; i < m; i++)
    res += n + p + x.f + x.g;
  return res;
}
for (var i = 0; i < 10000; i++) {
  x = {};
  x.f = 1;
  x.g = 1;
  foo(x, 10000, 1, 1);
}

Comment 5

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

before --ion -n: 458ms
          -m -n: 349ms
after  --ion -n: 490ms 

About a 6% difference, works for me!

Comment 10

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

http://hg.mozilla.org/projects/ionmonkey/rev/418840ca313e

Comment 11

[Christian Holler (:decoder)]

Test added:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7880f7434daa

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7880f7434daa