334612 - Crash when using select1

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Allan Beaufour]

I get some crashes when using select1s on trunk. 99% sure a regression from bug 313118.

Comment 1

[Allan Beaufour]

Attachment: 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.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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 ?

Comment 3

[Allan Beaufour]

(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

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 218961 [details] [diff] [review]
Patch

Well, if you're a clown and change that, r=me :)

Comment 5

[Allan Beaufour]

Attachment: declownified patch

That should take the circus out of it

Comment 6

[Doron Rosenberg (IBM)]

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{} :)

Comment 7

[Allan Beaufour]

Fixed on trunk.