485585 - test_query_messages.js fails on trunk builds

Status: VERIFIED FIXED

(MailNews Core :: Database, defect)

Description

[Mark Banner (:standard8)]

Attachment: Failure Log

Now that we have trunk builds running TUnit again, we've picked up a regression: test_query_messages.js is failing (all platforms). Not sure what the regression range is yet, or why its failing on trunk and not branch.

Log extract attached, brief info:

-- EXCEPTION START --
+ message (string) 'originator is null'
+ fileName (string) '/buildbot/linux-comm-central-check/build/objdir-tb/mozilla/_tests/xpcshell/test_mailnewsglobaldb/unit/test_query_messages.js'
+ lineNumber (number) 56
+ stack (string) 868 chars
+ name (string) 'TypeError'
*

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

I think we are witnessing another javascript bug.  Here is the code what crashes:

let originator = aSynthMessage; // aSynthMessage is never null!
while (originator.parent) { // this is the line that crashes.
  originator = originator.parent;
}

http://mxr.mozilla.org/comm-central/source/mailnews/db/gloda/test/unit/test_query_messages.js#56

Note that if aSynthMessage was null, the code would have crashed earlier in the function, before the function was ever called, etc.  Given the structure of the while loop there should be no way for originator to get null.

I'm thinking we should do one or more of the following things:

1) Turn off jitting for gloda unit tests or the whole tree.

2) Make it so unit test failures are logged and cause the tinderbox state to still be failed, but let the build run to completion.  Most of our unit tests are orthogonal, so it's not like letting the other unit tests run is going to be a waste.

2b) Make it so we can mark a unit test as persnicketty so that it gets ignored by the tinderbox.  For example, that intermittent gloda unit test failure originating from the fakeserver I have been remiss about resolving.

Comment 2

[Mark Banner (:standard8)]

(In reply to comment #1)
> I'm thinking we should do one or more of the following things:
> 
> 1) Turn off jitting for gloda unit tests or the whole tree.

I think this would be a bad step. If we're exposing JIT failures in our unit tests then we should be getting them to fix it. Hiding failures could mean that we're hiding real problems specific to the way we're using JS (and hence JIT) and then we'd get the problem in our release builds when we could have caught it much earlier.

The unfortunately thing in this case is that we have a week or so of checkins which could have caused the issue, normally we'd be down to a few changesets.

> 2) Make it so unit test failures are logged and cause the tinderbox state to
> still be failed, but let the build run to completion.  Most of our unit tests
> are orthogonal, so it's not like letting the other unit tests run is going to
> be a waste.

There's an m-c bug on this. I don't know why it hasn't been progressed.

> 2b) Make it so we can mark a unit test as persnicketty so that it gets ignored
> by the tinderbox.  For example, that intermittent gloda unit test failure
> originating from the fakeserver I have been remiss about resolving.

Again an option, but only temporarily. We should still be trying to track where these failures have started and get proper bugs/testcases filed.

There does look to have been some recent regressions filed. I'll try and do an c-c plus m-c build and see if we can get a specific range.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #2)
> > 1) Turn off jitting for gloda unit tests or the whole tree.
> 
> I think this would be a bad step. If we're exposing JIT failures in our unit
> tests then we should be getting them to fix it. Hiding failures could mean that
> we're hiding real problems specific to the way we're using JS (and hence JIT)
> and then we'd get the problem in our release builds when we could have caught
> it much earlier.

Anything spidermonkey runs, the JIT should be able to run and the results should be identical.

My concern is that the time lag between us getting bit by a JIT failure and the fix getting into the trunk or 1.9.1 is too long.  What if that intermittent gloda test trace assertion failure on 1.9.1 wasn't intermittent?

Moreover, I don't believe that us getting burnt by the JIT bugs is helping anyone.  That trace assertion failure sounds like it was already a known issue.  I doubt the JS team is watching our tinderboxes for JIT-related regressions.  When we get bit by the bugs, the only way they are going to know is if we do the legwork to determine that it is a JIT bug and attempt to alert them to it.  This seems like a lot of pain and legwork on our part for something that they will already know about with high probability.

Maybe we should pref the JIT off for Thunderbird entirely.  Worst case scenario is that we don't ship Thunderbird with the JIT on.  I honestly doubt that would make a big difference except for those individuals using Thunderbird as a web browser.  AFAIK, the JIT is still sufficiently feature-limited so that it ends up bailing on most of the gloda code.  And I'm not sure we have any other code that stays in pure JS space long enough that the JIT would make any meaningful performance impact.  (If the bayesian heavy-lifting was in JS space, then it would be another story.)

Comment 4

[Mark Banner (:standard8)]

Regression range:

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

Basically the recent tracemonkey merge as I think we suspected.

(In reply to comment #3)
> Maybe we should pref the JIT off for Thunderbird entirely.

I've just tried setting javascript.options.jit.content to false in all.js (then rebuilding etc). Neither this bug or bug 483857 are fixed by that. So unless there is some other way to turn off jit then its not an option here.

Comment 5

[Serge Gautherie (:sgautherie)]

(In reply to comment #1)
> 2) [...] let the build run to completion.

This is bug 451474.

> 2b) Make it so we can mark a unit test as persnicketty so that it gets ignored
> by the tinderbox.

This feature, like what reftest has, probably needs bug 485736 as a prerequisite...

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

I am unable to reproduce this locally using the same (trunk) mozilla-central rev that the tinderbox is currently exploding on, and this is without touching the preferences from default at all.  (m-c:262d44d6e425)  

I have done this with both the tunit mozconfig settings (taken from a full log that failed), plus a config with --enable-debugger-info-modules=yes and --disable-optimize.

Maybe the build boxes are using a buggy gcc or something?  I am on 4.3.3.

Whatever is happening, it's annoying.

Comment 7

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090328 SeaMonkey/2.0b1pre] (experimental/_m-c_, home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/262d44d6e425
 +http://hg.mozilla.org/comm-central/rev/054adad6539d)

I do reproduce (with --enable-optimize and no --enable-debugger-info-modules=yes).

Comment 8

[Mark Banner (:standard8)]

(In reply to comment #6)
> Maybe the build boxes are using a buggy gcc or something?  I am on 4.3.3.

FWIW I'm on 4.0.1 which I think is the standard mac install with xcode.

I've now done a reduced test case and filed it under bug 485889.

Comment 9

[Mark Banner (:standard8)]

Bug 485889 has now been checked in on trunk and all tinderboxes are green :-)

Therefore fixed by bug 485889.

Comment 10

[Serge Gautherie (:sgautherie)]

V.Fixed, per tinderboxes.