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 :-).
Could we use a fatal assertion (ie. MOZ_ASSERT or NS_ABORT_IF_FALSE) instead, just to really drive the point home?
I'm very much in favor of a hard fail there, but I'll let real JS hackers express their opinion!
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.
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 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.
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 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.
Thanks Blake. Shall I land this on tracemonkey or mozilla-central?
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.
Backed out: http://hg.mozilla.org/mozilla-central/rev/38bf7cf03a69 http://hg.mozilla.org/mozilla-central/rev/aad7af3a3360
I've filed bug 659707 which may have helped to catch the compiler errors earlier.
Re-landed with bug 659666 fixed http://hg.mozilla.org/mozilla-central/rev/14b1ffc1bf53
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?
6 years ago
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 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.
(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.
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.
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.
(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.
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!