Last Comment Bug 658351 - Add extra assertions to jsval.h to check for malformed JSBools
: Add extra assertions to jsval.h to check for malformed JSBools
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Jonathan Protzenko [:protz]
:
Mentors:
: 582523 (view as bug list)
Depends on: 583699 659666 662086 662125 662126
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-19 12:11 PDT by Jonathan Protzenko [:protz]
Modified: 2011-06-08 12:55 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add extra assertions to catch these errors early (933 bytes, patch)
2011-05-19 12:11 PDT, Jonathan Protzenko [:protz]
no flags Details | Diff | Splinter Review
Newer patch (861 bytes, patch)
2011-05-23 01:54 PDT, Jonathan Protzenko [:protz]
mrbkap: review+
Details | Diff | Splinter Review
At a conversion from rogue PRBools at the xpcconnect border (1.54 KB, patch)
2011-06-06 12:26 PDT, Jonathan Protzenko [:protz]
bzbarsky: review+
Details | Diff | Splinter Review

Description Jonathan Protzenko [:protz] 2011-05-19 12:11:34 PDT
Created attachment 533744 [details] [diff] [review]
Add extra assertions to catch these errors early

So we've had that kind of bugs in the past. The thing is bug 654126 is due to nsMsgDBView.cpp (Thunderbird code) doing

 *_retval = (whatever & 0x40000000);

with _retval being a PRBool*. This expression sometimes evaluates to 0x40000000 which leads to things similar to bug 346586. When this PRBool is converted to a JSBool, its value is incorrect. I'm not sure what happened next (:mrbkap knows better), but from what I understood, we then try to convert this value to a string. The code thinks that this is a boxed value, and tries to read at the address and segfaults.

Attached is a patch that shows what mrbkap had in mind, feel free to build upon it :-).
Comment 1 Josh Matthews [:jdm] 2011-05-19 12:56:58 PDT
Could we use a fatal assertion (ie. MOZ_ASSERT or NS_ABORT_IF_FALSE) instead, just to really drive the point home?
Comment 2 Jonathan Protzenko [:protz] 2011-05-19 12:59:18 PDT
I'm very much in favor of a hard fail there, but I'll let real JS hackers express their opinion!
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2011-05-19 13:08:50 PDT
I think we need this assertion underneath BOOLEAN_TO_JSVAL somewhere (which level wasn't immediately apparent at a quick glance).  If that's in place it catches *all* instances of this problem, not just this particular one.  If we had that, this specific assertion wouldn't (I think) be necessary, although it wouldn't hurt to keep it around for belt-and-suspenders protection.

Fatal assertions are always good in the opinion of JS engine hackers (although in very very rare cases they might be backed up by opt-build failure-handling), pace other Mozilla developers.
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2011-05-19 13:18:32 PDT
On a slightly closer look, BOOLEAN_TO_JSVAL_IMPL(JSBool b) looks like the right place for the assertion.  JS_ASSERT is properly fatal, so no need to worry about selecting the right assertion to use there.
Comment 5 Blake Kaplan (:mrbkap) 2011-05-20 07:03:13 PDT
Comment on attachment 533744 [details] [diff] [review]
Add extra assertions to catch these errors early

I think Waldo's right on this one. The reason that I started to add the assertion there was mainly to reduce my rebuild time; pushing this assertion into the JS engine will catch more cases.
Comment 6 Jonathan Protzenko [:protz] 2011-05-23 01:54:37 PDT
Created attachment 534382 [details] [diff] [review]
Newer patch

Here's an updated patch more in line with what mrbkap had in mind. I took the liberty of adding comments, although comments are scarce in this file. I hope this is not too much of a n00b behavior :-).

I've been running Thunderbird with this patch, and it seems to behave just fine. Hopefully there's not too many places in the code where we fail to construct proper PRBools.
Comment 7 Blake Kaplan (:mrbkap) 2011-05-23 05:55:58 PDT
Comment on attachment 534382 [details] [diff] [review]
Newer patch

Review of attachment 534382 [details] [diff] [review]:
-----------------------------------------------------------------

I would write the comment as:

This assertion exists to catch JSAPI users who accidentally create bogo-booleans thanks to the fact that JSBool is, in reality, an integer. These users must convert to JS_TRUE or JS_FALSE before creating packing the boolean into a jsval.
Comment 8 Jonathan Protzenko [:protz] 2011-05-23 14:29:01 PDT
Thanks Blake. Shall I land this on tracemonkey or mozilla-central?
Comment 9 Blake Kaplan (:mrbkap) 2011-05-24 02:04:12 PDT
Whichever is easier for you. They get merged to each other often enough.

I also hope you didn't take my comment word for word. It needs my "creating packing" thinko fixed one way or the other.
Comment 10 Jonathan Protzenko [:protz] 2011-05-25 08:12:02 PDT
http://hg.mozilla.org/mozilla-central/rev/4e79d99681f0
Comment 11 Jonathan Protzenko [:protz] 2011-05-25 08:49:07 PDT
and http://hg.mozilla.org/mozilla-central/rev/108fdd171946
Comment 13 Jeff Muizelaar [:jrmuizel] 2011-05-25 11:36:08 PDT
I've filed bug 659707 which may have helped to catch the compiler errors earlier.
Comment 14 Jonathan Protzenko [:protz] 2011-05-31 11:21:17 PDT
Re-landed with bug 659666 fixed

http://hg.mozilla.org/mozilla-central/rev/14b1ffc1bf53
Comment 15 Boris Zbarsky [:bz] 2011-06-06 10:46:51 PDT
So just to be clear, we introduced a known fatal assert on a codepath we _know_ can pass in data that will trigger it without fixing that codepath?  What the hell?  How did this get review?
Comment 16 Jonathan Protzenko [:protz] 2011-06-06 12:26:52 PDT
Created attachment 537607 [details] [diff] [review]
At a conversion from rogue PRBools at the xpcconnect border

Being too strict has caused to much fallout, so the reasonable thing to do here (thanks bz) is to go on with our lives and convert bad PRBools into good ones before feeding them into the JS engine. This goes with a warning, though, and we're trying to fix as much as we can in the bugs this bug depends on so that the warning remains meaningful.
Comment 17 Boris Zbarsky [:bz] 2011-06-06 12:31:54 PDT
Comment on attachment 537607 [details] [diff] [review]
At a conversion from rogue PRBools at the xpcconnect border

>+            if (b != 1 && b != 0)
>+                NS_WARNING("Passing a malformed PRBool through XPConnect");

NS_WARN_IF_FALSE, please.

r=me with that change.
Comment 18 Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-06 14:10:55 PDT
(In reply to comment #15)
> So just to be clear, we introduced a known fatal assert on a codepath we
> _know_ can pass in data that will trigger it without fixing that codepath?

What do you mean?  I thought aside from the bug 659566 fallout things had been fixed.

I don't actually think a warning in xpconnect like this is going to get meaningful attention amongst the rest of our warning spew, for what it's worth.
Comment 19 Boris Zbarsky [:bz] 2011-06-06 14:31:36 PDT
Did you mean bug 659666?

Who knows whether "things have been fixed".  No one bothered to check whether they are!  All we know is that the precise limited set of codepaths exercised by our tests passes.

> I don't actually think a warning in xpconnect like this is going to get
> meaningful attention

That's probably true, but this code can't assert valid input.  It's just not guaranteed that.
Comment 20 Jonathan Protzenko [:protz] 2011-06-07 07:32:53 PDT
I'll wait for the discussion to be settled before landing this, I don't want any more bashing in #developers :-). When you both agree, please let me know and I'll commit (or not) both this and bug 662126 in one push.
Comment 21 Mounir Lamouri (:mounir) 2011-06-07 07:41:46 PDT
(In reply to comment #18)
> (In reply to comment #15)
> > So just to be clear, we introduced a known fatal assert on a codepath we
> > _know_ can pass in data that will trigger it without fixing that codepath?
> 
> What do you mean?  I thought aside from the bug 659566 fallout things had
> been fixed.

FWIW, I felt into that assert yesterday while trying to run Firefox with a profile already in use.
Comment 22 Boris Zbarsky [:bz] 2011-06-07 08:53:33 PDT
I don't know what there is to agree about other than whether the warning should be there at all.  The !! bit needs to be there.  Please just commit the patch so people who're trying to dogfood the browser can actually do so!
Comment 23 Jonathan Protzenko [:protz] 2011-06-07 11:08:51 PDT
http://hg.mozilla.org/mozilla-central/rev/dd3de318717f
Comment 24 Jonathan Protzenko [:protz] 2011-06-08 12:55:39 PDT
*** Bug 582523 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.