If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

mDecoder null check done twice

RESOLVED FIXED in mozilla23

Status

()

Core
ImageLib
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: luisbg, Assigned: luisbg)

Tracking

Trunk
mozilla23
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 736928 [details] [diff] [review]
patch to remove unneeded null check
Attachment #736928 - Flags: review?(josh)
(Assignee)

Updated

5 years ago
Assignee: nobody → luis

Comment 2

5 years ago
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.

Updated

5 years ago
Attachment #736928 - Flags: review?(josh) → review-
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
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.
Attachment #736928 - Attachment is obsolete: true
Attachment #737534 - Flags: review?(josh)

Comment 5

5 years ago
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.
Attachment #737534 - Flags: review?(josh) → review?(joe)
(Assignee)

Comment 6

5 years ago
Created attachment 737574 [details] [diff] [review]
moved check and rewrote commit message
Attachment #737534 - Attachment is obsolete: true
Attachment #737534 - Flags: review?(joe)
(Assignee)

Updated

5 years ago
Attachment #737574 - Flags: review?(josh)
(Assignee)

Comment 7

5 years ago
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.
(Assignee)

Comment 9

5 years ago
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.
(Assignee)

Comment 10

5 years ago
Created attachment 737594 [details] [diff] [review]
null check removed
Attachment #737574 - Attachment is obsolete: true
Attachment #737574 - Flags: review?(josh)
(Assignee)

Updated

5 years ago
Attachment #737594 - Flags: review?(josh)
Attachment #737594 - Flags: review?(joe)
Attachment #737594 - Flags: review?(josh)
Attachment #737594 - Flags: review?(joe)
Attachment #737594 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #737594 - Flags: checkin?
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5d6dca5794f
Attachment #737594 - Flags: checkin?
https://hg.mozilla.org/mozilla-central/rev/b5d6dca5794f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.