Per bug 552237 comment 5: > New rule, now that I've been reminded why I didn't like this style: if you want > to have NS_ENSURE_SUCCESS checks on everything for extra debug info, that's > fine, but any pointer you are going to dereference must be explicitly checked, > because that can never lie. It would be great if you could make a patch for > your other recent changes that used this pattern, as I'm now concerned we may > have introduced latent or edge-cases crashers.

This probably needs to block 2.1a1; even though we haven't seen anything else, we've seen at least one crash, so we don't want an alpha (on 1.9.0 or 1.9.2) to uncover crashes where NS_ENSURE_SUCCESS has failed us.

Based on bug 552237 (which should have landed on 1.9.2 with null-checks, due to that being caught in review), I assumed everything landed after it was fine, and used bug 351351 (deMork) as the start-date for the lying macros. Then I went through checkins and looked for ones that could have used this pattern and looked at their diffs. All I saw was bug 547304. http://hg.mozilla.org/camino/pushloghtml?fromchange=a547467a492c&tochange=5b0a589f113f should be the pushlog link, but it doesn't work for some reason. Instead, I used http://hg.mozilla.org/camino/shortlog/3056 (bug 552237/FocusMgr) and just went backwards until I hit deMork.

Er, it looks like bug 547304 actually landed *with* the null-checks (per patch 2 there and the fact it landed essentially at the same time as FocusManager, which instituted the new rule), so, if I'm right, it's only deMork (bug 351351) that needs fixing.

This should do it.

Seeing the final patch, I guess I could have done this one after all, but it didn't look nearly as clear when I looked at the stuff on the original bug :-(

Comment on attachment 455371 [details] [diff] [review] fix sr=pink

Pushed http://hg.mozilla.org/camino/rev/33653b0b1c3d for Stuart (with a less inflammatory checkin comment than the summary ;) ). Just curious: why didn't the rest of the NS_ENSURE_SUCCESS checks in HistoryItem.mm's firstDate (and corresponding checks in HistoryDataSource.mm's loadLazily) need the checks?

We only need them when we are dereferencing the pointers; many times the value is quest QI'd, and that's safe to do on a NULL pointer. Thanks for landing!