Closed Bug 334007 Opened 18 years ago Closed 18 years ago

Schema Validation uses getter_AddRefs wrong, can cause crash

Categories

(Core Graveyard :: XForms, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: doronr, Assigned: doronr)

References

Details

(Keywords: crash, fixed1.8.0.4, fixed1.8.1)

Attachments

(1 file, 1 obsolete file)

this fixes the crash in the first testcase in bug 333113
Attached patch patch (obsolete) — Splinter Review
Stupid me, was doing foo->bar(getter_AddRefs(foo)) in two places inside the same method.

Also fix 2 warnings.
Attachment #218436 - Flags: review?(aaronr)
Attachment #218436 - Flags: review?(allan)
Comment on attachment 218436 [details] [diff] [review]
patch

>? extensions/schema-validation/src/temp
>Index: extensions/schema-validation/src/nsSchemaValidator.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/schema-validation/src/nsSchemaValidator.cpp,v
>retrieving revision 1.24
>diff -u -r1.24 nsSchemaValidator.cpp
>--- extensions/schema-validation/src/nsSchemaValidator.cpp	10 Apr 2006 16:51:32 -0000	1.24
>+++ extensions/schema-validation/src/nsSchemaValidator.cpp	14 Apr 2006 16:31:19 -0000
>@@ -3828,7 +3828,7 @@
>   rv = aSchemaParticle->GetMaxOccurs(&maxOccurs);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  nsCOMPtr<nsIDOMNode>leftOvers;
>+  nsCOMPtr<nsIDOMNode> leftOvers, tmpNode;
> 
>   switch(particleType) {
>     case nsISchemaParticle::PARTICLE_TYPE_ELEMENT: {
>@@ -3845,7 +3845,8 @@
> 
>         // if not an element node, skip
>         if (nodeType != nsIDOMNode::ELEMENT_NODE) {
>-          leftOvers->GetNextSibling(getter_AddRefs(leftOvers));
>+          leftOvers->GetNextSibling(getter_AddRefs(tmpNode));
>+          leftOvers = tmpNode;
>           continue;
>         }
> 

nit: I'd suggest nulling out tmpNode after you assign it into leftOvers since you never check return codes.  That way a junk tmpNode can't make it any farther.  But not that big of a deal.

>@@ -3860,8 +3861,10 @@
>           NS_ENSURE_SUCCESS(rv, rv);
> 
>           // set rest to the next element if node is valid
>-          if (isValid)
>-            leftOvers->GetNextSibling(getter_AddRefs(leftOvers));
>+          if (isValid) {
>+            leftOvers->GetNextSibling(getter_AddRefs(tmpNode));
>+            leftOvers = tmpNode;
>+          }
> 

same here.

r=me
Attachment #218436 - Flags: review?(aaronr) → review+
allan, if you r+, could you check this into trunk/branches since you are first up? thanks!
Blocks: 333113
Comment on attachment 218436 [details] [diff] [review]
patch

with Aaron's comments fixed, r=me too
Attachment #218436 - Flags: review?(allan) → review+
Fixed on trunk.
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: PC → All
Attachment #218436 - Attachment is obsolete: true
Checked in on 1_8_0 too. So hopefully, this will fix the crash in the release XPI?
Keywords: fixed1.8.0.3
Whiteboard: xf-to-branch
(In reply to comment #7)
> Checked in on 1_8_0 too. So hopefully, this will fix the crash in the release
> XPI?
> 

Hopefully it will be included in today's nightly xpis
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Whiteboard: xf-to-branch
Keywords: crash
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: