Closed Bug 338174 Opened 14 years ago Closed 13 years ago

Null dereference in nsBlockFrame::HandleOverflowPlaceholdersOnPulledLine

Categories

(Core :: Layout, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED INVALID

People

(Reporter: kherron+mozilla, Assigned: ehsan)

References

(Blocks 1 open bug, )

Details

(Keywords: coverity)

Attachments

(1 obsolete file)

This is coverity CID 180. Please see the sample URL. Either the |aLine->mFirstChild| null check at line 4877 is unnecessary, or else a null value can be dereferenced at lines 4887 and 4891 (within the call to |HandleOverflowPlaceholdersForPulledFrame|).
Status: NEW → ASSIGNED
Taking over.
Assignee: nobody → ehsan.akhgari
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Attached patch Add the null check (obsolete) — Splinter Review
Add the null check to the for loop in <http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsBlockFrame.cpp&rev=3.842&mark=4178#4178>.
Attachment #273014 - Flags: superreview?
Attachment #273014 - Flags: review?
Attachment #273014 - Flags: superreview?(dbaron)
Attachment #273014 - Flags: superreview?
Attachment #273014 - Flags: review?(dbaron)
Attachment #273014 - Flags: review?
Comment on attachment 273014 [details] [diff] [review]
Add the null check

Sorry, I should have bounced this review request to roc when it came in.

But I think the patch is wrong:  aLine->GetChildCount() means it has that many children, so a null-check shouldn't be needed.

I think we probably also have the invariant that lines shouldn't be empty, but there might be times in the middle of reflow when that's not true.  So I suspect the initial null-check is bogus.  But even if it's not, the initial null-check could presumably also be an aLine->GetChildCount() > 0 check, so I don't think there's an inherent inconsistency here.
Attachment #273014 - Flags: superreview?(dbaron)
Attachment #273014 - Flags: superreview-
Attachment #273014 - Flags: review?(roc)
Attachment #273014 - Flags: review?(dbaron)
Attachment #273014 - Flags: review-
I agree with (In reply to comment #3)
> But I think the patch is wrong:  aLine->GetChildCount() means it has that many
> children, so a null-check shouldn't be needed.

I agree.
Comment on attachment 273014 [details] [diff] [review]
Add the null check

Marking the patch obsolete based on comment 3 and comment 4.
Attachment #273014 - Attachment is obsolete: true
It seems that the presumed null dereference never happens, so I'm marking this as INVALID.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → INVALID
Needs verification...
Keywords: verifyme
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.