587809 - JM64: Mochitest assert in json_worker.js

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

entering trace at http://mochi.test:8888/tests/dom/src/threads/test/json_worker.js:306@9, execs: 2 code: 0x11be3a5dc
leaving trace at http://mochi.test:8888/tests/dom/src/threads/test/json_worker.js:310@28, op=getprop, lr=0x10633d9e0, exitType=BRANCH, sp=3, calldepth=0, cycles=0
args0=function<0x1350a68b8:onmessage> args1=object<0x1350a7558:DedicatedWorkerGlobalScope> args2=object<0x134cf0d38:XPCWrappedNative_NoHelper> arguments0=null scopeChain0=object<0x1350a7558:DedicatedWorkerGlobalScope> var0=int<5> var1=undefined stack0=function<0x134cf0d80:postMessage> stack1=Assertion failure: v.isNull(), at /Users/dvander/mozilla/jaegermonkey/js/src/jstracer.cpp:2898

Comment 1

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

Looks like this is most if not all of the trace-test failures, and is related to eager-this. To reproduce:

./Debug64/js -m -j trace-test/tests/basic/bug535760.js

The interpreter stores a NULL and the tracer stores the global object. This was only caught when Luke added assertions in NativeToValue(). It likely affects x86 as well but it might be harder to see with the split value encoding.

Comment 2

[Robert Sayre]

cdleary, you have a fix for this somewhere? maybe in jorendorff's queue?

Comment 4

[David Mandelin [:dmandelin]]

Attachment: Patch (was tracer changes for eager this patch in other bug)

This patch is cdleary's work; I just rebased it to JM tip. This is probably the worst outstanding JM bug blocking Sep 1 landing right now, so expedited review would be much appreciated. Jason, let us know if you are too busy and we need to find another reviewer.

Comment 5

[David Mandelin [:dmandelin]]

I "pre-landed" this on JM as:

  http://hg.mozilla.org/projects/jaegermonkey/rev/305b00ec07e7

just so we can get Tinderbox coverage on it, both as an extra test for the patch, and to find out earlier what new bugs this fix might expose (from tests that asserted early before).

Comment 7

[Jason Orendorff [:jorendorff]]

I am too busy. I'll try and get to it anyway, maybe tomorrow.

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 467263 [details] [diff] [review]
Patch (was tracer changes for eager this patch in other bug)

I originally wrote this patch (for bug 556277). So I only have a few nits.

In TR::getThis:
>+        JS_ASSERT(!fp->getThisValue().isPrimitive());

Instead, assert that fp->getThisValue().isObject(), which is the same
condition. And then...

>+        JS_ASSERT(fp->getThisValue().toObjectOrNull() == obj);

...use the stricter .toObject() here...

>+        this_ins = INS_CONSTOBJ(fp->getThisValue().toObjectOrNull());

...and here.

>+    Value& thisv = fp->argv[-1];

const Value& please.

>+    if (!obj)
>+        RETURN_ERROR("getThisObject failed");
>+    JS_ASSERT(fp->argv[-1] == ObjectOrNullValue(obj));

ObjectValue, please -- obj can't be null given the previous lines.

Great patch! ;-)

Comment 9

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/tracemonkey/rev/1b354f3d6e5c

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/1b354f3d6e5c