485889 - Incorrect null checking/assignment? (with xpcshell test case)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Mark Banner (:standard8)]

Attachment: xpcshell test case

This is a regression caused by the tracemonkey branch merge here:

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2009-03-24+10%3A50%3A00&enddate=2009-03-24+10%3A51%3A00

This bug is keeping the Thunderbird trunk tinderboxes orange (bug 485585). We didn't pick it up earlier due to other trunk issues that were hiding the problem.

Attaching a reduced test case for the failure. This fails for mozilla-central, but passes on mozilla-1.9.1.

The main issue is here:

    do_check_neq(originator, null);
    while (originator.parent) {
      originator = originator.parent;
      // Adding this line can make it pass
      // do_check_neq(originator, null);
    }

So we know originator is not null going into the loop, but the test fails with:

TEST-UNEXPECTED-FAIL | /Users/moztest/comm/trunk/tb/mozilla/_tests/xpcshell/test_mailnewsglobaldb/unit/test_min_js_failure.js | test failed, see following log:
  >>>>>>>
*** test pending
TypeError: originator is null
*** FAIL ***
WARNING: nsExceptionService ignoring thread destruction after shutdown: file /Users/moztest/comm/trunk/src/mozilla/xpcom/base/nsExceptionService.cpp, line 194
nsStringStats
 => mAllocCount:           1029
 => mReallocCount:            0
 => mFreeCount:            1029
 => mShareCount:             53
 => mAdoptCount:             15
 => mAdoptFreeCount:         15

  <<<<<<<

Uncommenting the do_check_neq line or doing another change as commented in the test file makes the test pass, and hence indicates this is a javascript issue.

Comment 1

[Robert Sayre]

Attachment: js shell test case

robert-sayres-macbook-pro:bin sayrer$ ./js -f nullcheck.js
robert-sayres-macbook-pro:bin sayrer$ ./js -j -f nullcheck.js
nullcheck.js:42: TypeError: originator is null

Comment 2

[Jeff Walden [:Waldo]]

Attachment: JSVAL_TAG(v) == JSVAL_OBJECT ⇏ !JSVAL_IS_PRIMITIVE(v)

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 370099 [details] [diff] [review]
JSVAL_TAG(v) == JSVAL_OBJECT ⇏ !JSVAL_IS_PRIMITIVE(v)

>From: Jeff Walden <[e-mail]>
>
>Bug 485889 - Incorrect null checking/assignment? (with xpcshell test case).  NOT REVIEWED YET
>
>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp
>+++ b/js/src/jstracer.cpp
>@@ -5043,10 +5043,12 @@ TraceRecorder::ifop()
>     bool cond;
>     LIns* x;
> 
>-    /* Objects always evaluate to true since we specialize the Null type on trace. */
>-    if (JSVAL_TAG(v) == JSVAL_OBJECT) {
>+    if (!JSVAL_IS_PRIMITIVE(v)) {
>         cond = true;
>         x = lir->insImm(1);
>+    } else if (JSVAL_IS_NULL(v)) {

JSVAL_IS_PRIMITIVE(v) is (!JSVAL_IS_OBJECT(v) || JSVAL_IS_NULL(v)), so !JSVAL_IS_PRIMITIVE(v) is (JSVAL_IS_OBJECT(v) && !JSVAL_IS_NULL(v)), so maybe reorder this if-else-if to test JSVAL_IS_NULL before JSVAL_IS_OBJECT?

/be

Comment 4

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/949b20b7fd94 (with nit picked)

Comment 5

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/949b20b7fd94

Comment 6

[(mostly gone) XtC4UaLL [:xtc4uall]]

verified fixed on trunk per https://bugzilla.mozilla.org/show_bug.cgi?id=485637#c21

Comment 7

[Brendan Eich [:brendan]]

What landed tests !JSVAL_IS_PRIMITIVE in the else-if where leading if tests JSVAL_IS_NULL, which should be optimized to JSVAL_IS_OBJECT -- but maybe it's better to write !JSVAL_IS_PRIMITIVE and preserve order independence here.

/be

Comment 8

[Mark Banner (:standard8)]

Today's tracemonkey merge to 1.9.1 [1] has caused bug 485585 to show up on the Thunderbird 3/SeaMonkey 2 tinderboxes which was fixed by this bug. Note that the regression also caused bug 485637 - TM: Gmail contacts list is empty.

Therefore can we get this fix landed on 1.9.1 asap please? 

[1] http://hg.mozilla.org/releases/mozilla-1.9.1/pushloghtml?startdate=2009-04-14+11%3A46%3A00&enddate=2009-04-14+11%3A47%3A00

Comment 9

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5c5b006f9481

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with given xpshell testcase on trunk and 1.9.1 with the
following debug builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre)
Gecko/20090422 Minefield/3.6a1pre ID:20090422224452

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre)
Gecko/20090422 Shiretoko/3.5b4pre ID:20090422122043

Comment 11

[Bob Clary [:bc] (inactive)]

js1_8_1/trace/trace-test.js
http://hg.mozilla.org/tracemonkey/rev/61892f57b46a

Comment 12

[Bob Clary [:bc] (inactive)]

cvsroot/mozilla/js/tests/js1_8_1/trace/trace-test.js,v  <--  trace-test.js
new revision: 1.14; previous revision: 1.13

/cvsroot/mozilla/js/tests/shell.js,v  <--  shell.js