Status

Core Graveyard
XForms
--
critical
RESOLVED FIXED
12 years ago
a year ago

People

(Reporter: Allan Beaufour, Assigned: Allan Beaufour)

Tracking

({crash, fixed1.8.0.5, fixed1.8.1})

Trunk
crash, fixed1.8.0.5, fixed1.8.1

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

6.30 KB, patch
Doron Rosenberg (IBM)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

12 years ago
I get some crashes when using select1s on trunk. 99% sure a regression from bug 313118.
(Assignee)

Comment 1

12 years ago
Created attachment 218961 [details] [diff] [review]
Patch

I was a bit to eager to RebindToModel() on document/parent changes. We should detach from the current model alright, but there's no need to try to Bind() again if there's no parent or document.

I've also added a few sanity-checks to related parts.
Assignee: aaronr → allan
Status: NEW → ASSIGNED
Attachment #218961 - Flags: review?(Olli.Pettay)

Comment 2

12 years ago
Comment on attachment 218961 [details] [diff] [review]
Patch

>? rebind.patch
>Index: nsXFormsControlStub.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsControlStub.cpp,v
>retrieving revision 1.43
>diff -u -p -U8 -r1.43 nsXFormsControlStub.cpp
>--- nsXFormsControlStub.cpp	19 Apr 2006 08:22:28 -0000	1.43
>+++ nsXFormsControlStub.cpp	19 Apr 2006 09:31:37 -0000
>@@ -235,28 +235,36 @@ nsXFormsControlStubBase::GetUsesModelBin
> }
> 
> nsresult
> nsXFormsControlStubBase::MaybeAddToModel(nsIModelElementPrivate *aOldModel,
>                                          nsIXFormsControl       *aParent)
> {
>   // XXX: just doing pointer comparison would be nice....
>   PRBool sameModel = PR_FALSE;
>-  nsCOMPtr<nsIDOM3Node> n3Model(do_QueryInterface(mModel));
>-  nsCOMPtr<nsIDOMNode> nOldModel(do_QueryInterface(aOldModel));
>-  NS_ASSERTION(n3Model, "model element not supporting nsIDOM3Node?!");
>-  nsresult rv = n3Model->IsSameNode(nOldModel, &sameModel);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  nsresult rv;
>+
>+  if (mModel) {
>+    nsCOMPtr<nsIDOM3Node> n3Model(do_QueryInterface(mModel));
>+    nsCOMPtr<nsIDOMNode> nOldModel(do_QueryInterface(aOldModel));
>+    NS_ASSERTION(n3Model, "model element not supporting nsIDOM3Node?!");
>+    rv = n3Model->IsSameNode(nOldModel, &sameModel);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  } else {
>+    sameModel = aOldModel ? PR_TRUE : PR_FALSE;
>+  }

Why sameModel is PR_TRUE if aOldModel but not mModel ?
(Assignee)

Comment 3

12 years ago
(In reply to comment #2)
> (From update of attachment 218961 [details] [diff] [review] [edit])
> >+  if (mModel) {
> >+    nsCOMPtr<nsIDOM3Node> n3Model(do_QueryInterface(mModel));
> >+    nsCOMPtr<nsIDOMNode> nOldModel(do_QueryInterface(aOldModel));
> >+    NS_ASSERTION(n3Model, "model element not supporting nsIDOM3Node?!");
> >+    rv = n3Model->IsSameNode(nOldModel, &sameModel);
> >+    NS_ENSURE_SUCCESS(rv, rv);
> >+  } else {
> >+    sameModel = aOldModel ? PR_TRUE : PR_FALSE;
> >+  }
> 
> Why sameModel is PR_TRUE if aOldModel but not mModel ?

Because I am a clown, is that a good enough reason? :-) Yes, the other way around.

Comment 4

12 years ago
Comment on attachment 218961 [details] [diff] [review]
Patch

Well, if you're a clown and change that, r=me :)
Attachment #218961 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 5

12 years ago
Created attachment 218991 [details] [diff] [review]
declownified patch

That should take the circus out of it
Attachment #218961 - Attachment is obsolete: true
Attachment #218991 - Flags: review?(doronr)

Comment 6

12 years ago
Comment on attachment 218991 [details] [diff] [review]
declownified patch

>? attachment.cgi?id=218961
>? cirque.patch
>Index: nsXFormsControlStub.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsControlStub.cpp,v
>retrieving revision 1.43
>diff -u -p -U8 -r1.43 nsXFormsControlStub.cpp
>--- nsXFormsControlStub.cpp	19 Apr 2006 08:22:28 -0000	1.43
>+++ nsXFormsControlStub.cpp	19 Apr 2006 14:16:33 -0000
>@@ -235,28 +235,36 @@ nsXFormsControlStubBase::GetUsesModelBin
> }
> 
> nsresult
> nsXFormsControlStubBase::MaybeAddToModel(nsIModelElementPrivate *aOldModel,
>                                          nsIXFormsControl       *aParent)
> {
>   // XXX: just doing pointer comparison would be nice....
>   PRBool sameModel = PR_FALSE;
>-  nsCOMPtr<nsIDOM3Node> n3Model(do_QueryInterface(mModel));
>-  nsCOMPtr<nsIDOMNode> nOldModel(do_QueryInterface(aOldModel));
>-  NS_ASSERTION(n3Model, "model element not supporting nsIDOM3Node?!");
>-  nsresult rv = n3Model->IsSameNode(nOldModel, &sameModel);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  nsresult rv;
>+
>+  if (mModel) {
>+    nsCOMPtr<nsIDOM3Node> n3Model(do_QueryInterface(mModel));
>+    nsCOMPtr<nsIDOMNode> nOldModel(do_QueryInterface(aOldModel));
>+    NS_ASSERTION(n3Model, "model element not supporting nsIDOM3Node?!");
>+    rv = n3Model->IsSameNode(nOldModel, &sameModel);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  } else {
>+    sameModel = !aOldModel;
>+  }
>   if (!sameModel) {
>     if (aOldModel) {
>       rv = aOldModel->RemoveFormControl(this);
>       NS_ENSURE_SUCCESS(rv, rv);
>     }
>-    rv = mModel->AddFormControl(this, aParent);
>-    NS_ENSURE_SUCCESS(rv, rv);
>+    if (mModel) {
>+      rv = mModel->AddFormControl(this, aParent);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    }
>   }

only nit - would be nice to have a newline after the if {} else{} block and the next if{} :)
Attachment #218991 - Flags: review?(doronr) → review+
(Assignee)

Comment 7

12 years ago
Fixed on trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.1
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.0.5
(Assignee)

Updated

11 years ago
Whiteboard: xf-to-branch
Severity: normal → critical
Keywords: crash
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.