Last Comment Bug 334612 - Crash when using select1
: Crash when using select1
Status: RESOLVED FIXED
: crash, fixed1.8.0.5, fixed1.8.1
Product: Core
Classification: Components
Component: XForms (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Allan Beaufour
: Stephen Pride
Mentors:
http://www.w3.org/TR/xforms/
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-04-19 02:30 PDT by Allan Beaufour
Modified: 2006-10-25 06:49 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (6.29 KB, patch)
2006-04-19 02:33 PDT, Allan Beaufour
bugs: review+
Details | Diff | Review
declownified patch (6.30 KB, patch)
2006-04-19 07:18 PDT, Allan Beaufour
doronr: review+
Details | Diff | Review

Description Allan Beaufour 2006-04-19 02:30:27 PDT
I get some crashes when using select1s on trunk. 99% sure a regression from bug 313118.
Comment 1 Allan Beaufour 2006-04-19 02:33:51 PDT
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.
Comment 2 Olli Pettay [:smaug] 2006-04-19 05:17:28 PDT
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 2006-04-19 07:03:50 PDT
(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] 2006-04-19 07:10:16 PDT
Comment on attachment 218961 [details] [diff] [review]
Patch

Well, if you're a clown and change that, r=me :)
Comment 5 Allan Beaufour 2006-04-19 07:18:39 PDT
Created attachment 218991 [details] [diff] [review]
declownified patch

That should take the circus out of it
Comment 6 Doron Rosenberg (IBM) 2006-04-19 07:29:47 PDT
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 2006-04-19 07:52:57 PDT
Fixed on trunk.

Note You need to log in before you can comment on or make changes to this bug.