Closed Bug 211357 Opened 21 years ago Closed 21 years ago

[FIX]Crash - accessing TR.rowIndex

Categories

(Core :: DOM: Core & HTML, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.5alpha

People

(Reporter: bc, Assigned: bzbarsky)

References

Details

(Keywords: crash, fixed1.4.1, testcase)

Attachments

(2 files)

Create a TR element and access TR.rowIndex before attaching to a TABLE to crash.
Attached file testcase
Attached patch patchSplinter Review
The null-checking here got badly broken at some point....
Comment on attachment 126883 [details] [diff] [review]
patch

jst, is peterv generally around?  I'm feeling bad about all these review
requests...
Attachment #126883 - Flags: superreview?(jst)
Attachment #126883 - Flags: review?(jst)
.
Assignee: dom_bugs → bzbarsky
OS: Windows 2000 → All
Priority: -- → P2
Hardware: PC → All
Summary: Crash - accessing TR.rowIndex → [FIX]Crash - accessing TR.rowIndex
Target Milestone: --- → mozilla1.5alpha
Comment on attachment 126883 [details] [diff] [review]
patch

   nsresult rv = GetParentNode(getter_AddRefs(sectionNode));
-  NS_ENSURE_SUCCESS(rv, rv);
+  NS_ENSURE_TRUE(sectionNode, rv);

   nsCOMPtr<nsIDOMNode> tableNode;
   rv = sectionNode->GetParentNode(getter_AddRefs(tableNode));
-  if (NS_SUCCEEDED(rv) && sectionNode) {
-    rv = CallQueryInterface(tableNode, aTable);
-  }
-
-  return rv;
+  NS_ENSURE_TRUE(tableNode, rv);
+  
+  return CallQueryInterface(tableNode, aTable);

Shouldn't those be just "if (!...) return rv;"? We don't want to print out a
warning just because a tr element has no parent when accessing .rowIndex do we?

r+sr=jst
Attachment #126883 - Flags: superreview?(jst)
Attachment #126883 - Flags: superreview+
Attachment #126883 - Flags: review?(jst)
Attachment #126883 - Flags: review+
Indeed.  Changed those to normal ifs and checked in.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
This isn't that big a deal, but should we get it on the 1.4 branch as well?
Might be a good idea, yes... simple fix and all.
Flags: blocking1.4.x?
Attachment #126883 - Flags: approval1.4.x?
Comment on attachment 126883 [details] [diff] [review]
patch

a=asa (on behalf of drivers) for checkin to the 1.4 branch. Time is short. Who
can land this for us?
Attachment #126883 - Flags: approval1.4.x? → approval1.4.x+
Fixed on 1.4 branch.
Flags: blocking1.4.x?
Keywords: fixed1.4
Keywords: fixed1.4fixed1.4.1
Blocks: 224532
Component: DOM: HTML → DOM: Core & HTML
QA Contact: desale → general
You need to log in before you can comment on or make changes to this bug.