in RasterImage::WriteToDecoder this two checks happen: NS_ABORT_IF_FALSE(mDecoder, "Trying to write to null decoder!"); if (!mDecoder) The second one is redundant since the execution would have aborted in the first one and it wouldn't have made it to the second. Or worse it would crash in a previous line before the second check where mDecoder->Write() is called.
Created attachment 736928 [details] [diff] [review] patch to remove unneeded null check
Unfortunately, NS_ABORT_IF_FALSE is deceptively-named and only aborts in a debug build. Therefore the null check is not redundant, but as you point out it's ineffective in its current placement. Therefore, I suggest moving it somewhere where it will do some good.
Josh, I moved the check to happen before mDecoder is used. Since you say NS_ABORT_IF_FALSE only aborts in debug builds. Doesn't that mean that this if (!mDecoder) checks needs to happen in more instances. I see a bunch of situations where NS_ABORT_IF_FALSE is the only check and mDecoder is assumed to be initialized.
Created attachment 737534 [details] [diff] [review] moved the check Also, does the patch comment need to be updated to reflect the change? Location of check being wrong instead of duplicate.
Comment on attachment 737534 [details] [diff] [review] moved the check Yes, changing the commit message to reflect the change would be the right thing to do. I'll let Joe comment on the other places that just use the abort and no other check.
Created attachment 737574 [details] [diff] [review] moved check and rewrote commit message
Josh, if more checks need to be moved. Should they all be part of the same patch or independent ones?
Honestly, I'd be OK with removing the null check altogether. As long as we dereference the pointer, we'll crash, and having null checks is just going to give us poorly-tested, probably buggy error handling.
Cool. I was considering the option of just removing it, based on my new understanding of the NS_ABORT_IF_FALSE. I will remove it based on that it is better for testing/debugging.
Created attachment 737594 [details] [diff] [review] null check removed