582523 - Javascript/xpconnect should give some warning about invalid boolean values as a result of incorrect PRBool usage.

Status: RESOLVED DUPLICATE of bug 658351

(Core :: JavaScript Engine, defect)

Description

[Mark Banner (:standard8)]

sayre mentioned to me that tracemonkey is doing another merge to trunk very
soon. I therefore checked it out with comm-central and latest tracemonkey
(revision d7c7ba27b84e), and built and ran the Thunderbird tests.

Some of the our xpcshell-tests are broken with the following error:

TEST-UNEXPECTED-FAIL | base_index_messages.js | true == true - See following stack:
JS frame :: /Users/moztest/comm/tracemonkey/src/mozilla/testing/xpcshell/head.js :: do_throw :: line 273
JS frame :: /Users/moztest/comm/tracemonkey/src/mozilla/testing/xpcshell/head.js :: do_check_eq :: line 303
JS frame :: base_index_messages.js :: test_attributes_explicit :: line 492
JS frame :: ../../mailnews/resources/asyncTestUtils.js :: _async_driver :: line 155
JS frame :: /Users/moztest/comm/tracemonkey/src/mozilla/testing/xpcshell/head.js :: anonymous :: line 240

It appears to be a regression as this is saying that true != true.

This is the failing line of code:

http://hg.mozilla.org/comm-central/annotate/1dea83e6409f/mailnews/db/gloda/test/unit/base_index_messages.js#l492

Comment 1

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

What was the previous TM / m-c revision that worked for c-c? It'll be cool to get that rev id so a regression range can be obtained. :)

Comment 2

[Mark Banner (:standard8)]

(In reply to comment #1)
> What was the previous TM / m-c revision that worked for c-c? It'll be cool to
> get that rev id so a regression range can be obtained. :)

Well, the obvious one would the last TM merge. I don't know when that was. Unfortunately I've not really got time to go looking for it today either.

Comment 3

[Mark Banner (:standard8)]

I've got hg bisect working on a regression range at the moment. So far I have it down to:

http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=943de0cf6f0a&tochange=77eb248fa854

but given an few more hours I should hopefully have the actual changeset/bug.

Comment 4

[Mark Banner (:standard8)]

(In reply to comment #3)
> http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=943de0cf6f0a&tochange=77eb248fa854

Ignore this one, I think I was wrongly interpreting what bisect was doing, should have a better idea later.

Comment 5

[Mark Banner (:standard8)]

Ok, this is a regression from bug 549143, this changeset:

http://hg.mozilla.org/tracemonkey/rev/9c869e64ee26

I have also worked out what one regression is:

Here's what I'm seeing.

In nsIMsgDBHdr:

  readonly attribute boolean isFlagged;

in the GetIsFlagged c++ function:

  NS_IMETHODIMP nsMsgHdr::GetIsFlagged(PRBool *isFlagged)
  {
    ...
    *isFlagged = m_flags & nsMsgMessageFlags::Marked;
    return NS_OK;
  }

in the xpcshell-test:

  do_check_true(gHeader.isFlagged);



So isFlagged is being set to a non PR_TRUE value, but also something that isn't PR_FALSE.

Javascript "sees" it as true as per the test result text:

TEST-UNEXPECTED-FAIL | /Users/moztest/comm/tracemonkey/tb/mozilla/_tests/xpcshell/test_imap/unit/test_imapFilterActions.js | true == true

but javascript is obviously now requiring gHeader.isFlagged == 1 rather than gHeader.isFlagged != 0.


If I change the cpp code to do this:

*isFlagged = !!(m_flags & nsMsgMessageFlags::Marked);

then the tests all pass.


As effectively mailnews is abusing PR_Bool, then I'm happy to tidy this up, as per other bugs that have been done on core.


However, I also think there should be some assertion, warning or fix done to javascript - because it isn't obvious if you've done this by mistake, especially when doing a dump of variable gives you "true"!

Comment 6

[Luke Wagner [:luke]]

(In reply to comment #5)
> However, I also think there should be some assertion, warning or fix done to
> javascript - because it isn't obvious if you've done this by mistake,
> especially when doing a dump of variable gives you "true"!

Agreed, will do.

Comment 7

[Luke Wagner [:luke]]

Also, assuming that the solution to these failures is just what is described in comment 5, it doesn't seem like this bug should block newjsvals.

Comment 8

[Brendan Eich [:brendan]]

This is an XPConnect bug, right?

/be

Comment 9

[Mark Banner (:standard8)]

(In reply to comment #7)
> Also, assuming that the solution to these failures is just what is described in
> comment 5, it doesn't seem like this bug should block newjsvals.

Well I set it blocking because that was the bug attributed to the changeset that caused the regression, and that marking the regression bug as blocking is something we do as standard. If there's a different bug that actually caused it and was contained in that one changeset, then by all means reassign the blocking.

Comment 10

[Luke Wagner [:luke]]

Oh, I'm not that familiar with PRBool vs. JSBool.  Is it legal for a PRBool to have non-canonical true/false values?

Comment 11

[Mike Shaver (:shaver emeritus)]

No.  We have a static analysis about that, even, I thought.  Dunno why it didn't catch this case.

Comment 12

[Mark Banner (:standard8)]

(In reply to comment #11)
> No.  We have a static analysis about that, even, I thought.  Dunno why it
> didn't catch this case.

If you mean static analysis on PRBool, then I believe that hasn't been set up for comm-central apps.

Now I know about it, I can look at getting it set up, but it would be nice to have some sort of warning from the system that we're doing the wrong thing (so that developers have a chance of finding out before they check in and then static analysis runs).

Comment 13

[Mark Banner (:standard8)]

Updating title to reflect what I think should be happening here - developers should get some sort of warning if nothing else.

Comment 15

[Jonathan Protzenko [:protz]]

Mike, we briefly made it fatal to pass malformed PRBools into the JS engine, in bug 658351. During the brief window of time where this was enabled, quite a few bugs came, but it made Firefox very crashy, so we had to turn that into a warning when xpcconvert.cpp hits a PRBools that's neither 0 or 1.

The warning now reads "Passing a malformed PRBool through XPConnect", and I think it shows only in debug builds. I believe this solves the issue, so I'm marking this as a duplicate.

Comment 16

[Jonathan Protzenko [:protz]]

Oh and I meant to ask "Mike, are you positive we have static analysis for this?". There at least six places we had to patch (bug 654126, bug 662126 and bug 661319). There's probably others lurking around, so if we've got a way to run a pass of static analysis for this, I'd gladly volunteer to take care of it.