Closed Bug 351687 Opened 14 years ago Closed 13 years ago

[@ nsBlockFrame::GetAccessible] mContent null check comes after we use mContent

Categories

(Core :: Disability Access APIs, defect)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: timeless, Assigned: timeless)

References

(Blocks 1 open bug, )

Details

(Keywords: coverity)

Attachments

(1 file, 1 obsolete file)

809 bytes, patch
aaronlev
: review+
dbaron
: superreview+
beltzner
: approval1.9+
Details | Diff | Splinter Review
 
It looks like I checked mContent for null after it was already used. It should be checked before.
Blocks: keya11y
Severity: normal → critical
Summary: nsBlockFrame::GetAccessible(nsIAccessible **) has a hopefully useless null check of mContent → [@ nsBlockFrame::GetAccessible] crashes dereferencing mContent before null check
Attached patch reorder (obsolete) — Splinter Review
Assignee: aaronleventhal → timeless
Status: NEW → ASSIGNED
Attachment #238880 - Flags: review?(aaronleventhal)
Attachment #238880 - Flags: review?(aaronleventhal) → review+
Why wasn't this checked in?
Comment on attachment 238880 [details] [diff] [review]
reorder

because we require sr's and i don't track bugs so someone has to shepherd them through the process...
Attachment #238880 - Flags: superreview?(dbaron)
Has anyone actually seen this crash or is this just theoretical?  If the former, why was mContent null?  If the latter, do you have reason to believe that mContent is sometimes null?
Comment on attachment 238880 [details] [diff] [review]
reorder

I think it's the null-check itself that's bogus; show me a block frame creation path that can happen with a null content node and I'll consider it.
Attachment #238880 - Flags: superreview?(dbaron) → superreview-
Blocks: fox3access
Dbaron, I've never seen this crash. But, we should either have the proper null check, change it to an assertion or remove it entirely.
No longer blocks: fox3access
Summary: [@ nsBlockFrame::GetAccessible] crashes dereferencing mContent before null check → [@ nsBlockFrame::GetAccessible] mContent null check comes after we use mContent
What's the status on this? Aaron, do crash reports show any problems in this area? Or should this be marked WONTFIX if the NULL check is bogus anyway?
Marco, this is low priority. We should can the bogus check since it doesn't do anything.
Attached patch kill itSplinter Review
Attachment #238880 - Attachment is obsolete: true
Attachment #313952 - Flags: review?(aaronleventhal)
Attachment #313952 - Flags: review?(aaronleventhal) → review+
Attachment #313952 - Flags: superreview?(dbaron)
Comment on attachment 313952 [details] [diff] [review]
kill it

sr=dbaron
Attachment #313952 - Flags: superreview?(dbaron) → superreview+
Comment on attachment 313952 [details] [diff] [review]
kill it

a1.9=beltzner
Attachment #313952 - Flags: approval1.9+
Comment on attachment 313952 [details] [diff] [review]
kill it

mozilla/layout/generic/nsBlockFrame.cpp 	3.951
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.