Last Comment Bug 308986 - [CRASH] Handle changes to the src attribute of the instance element.
: [CRASH] Handle changes to the src attribute of the instance element.
Status: RESOLVED FIXED
: crash, fixed1.8
Product: Core
Classification: Components
Component: XForms (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: aaronr
: Stephen Pride
Mentors:
http://groups.google.fi/group/netscap...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-09-17 15:06 PDT by Olli Pettay [:smaug] (high review load, please consider other reviewers)
Modified: 2005-10-07 00:45 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
external instance data for testcase (4.92 KB, text/xml)
2005-09-19 14:13 PDT, aaronr
no flags Details
testcase (1.55 KB, application/xhtml+xml)
2005-09-19 14:16 PDT, aaronr
no flags Details
testcase2 (1.67 KB, application/xhtml+xml)
2005-09-19 16:46 PDT, aaronr
no flags Details
draft fix (6.45 KB, patch)
2005-09-19 18:47 PDT, aaronr
no flags Details | Diff | Review
testcase2 (1.68 KB, application/xhtml+xml)
2005-09-20 16:26 PDT, aaronr
no flags Details
latest patch (6.85 KB, patch)
2005-09-21 14:43 PDT, aaronr
bugs: review+
Details | Diff | Review
fixes comment by smaug (5.25 KB, patch)
2005-09-22 12:29 PDT, aaronr
doronr: review+
Details | Diff | Review

Description Olli Pettay [:smaug] (high review load, please consider other reviewers) 2005-09-17 15:06:04 PDT
I don't think we handle properly the situation where src attribute of the
instance element is changed while the loading of the instance document
is still ongoing.

Haven't yet confirmed the crash, but at least we could Cancel the channel
before creating a new one.
See nsXFormsInstanceElement::LoadExternalInstance.

http://groups.google.fi/group/netscape.public.mozilla.xml/browse_thread/thread/5b82303528d6c070/53c5f954761d931b?lnk=st&q=%22firefox+1.6a1+and+xforms.xpi+from+same+nightly+not+compatible%22&rnum=1&hl=fi#53c5f954761d931b
Comment 1 aaronr 2005-09-19 14:13:12 PDT
Created attachment 196698 [details]
external instance data for testcase
Comment 2 aaronr 2005-09-19 14:16:26 PDT
Created attachment 196699 [details]
testcase

testcase crashes on load.  Should show a select1 with all 50 U.S. states in it.
Comment 3 aaronr 2005-09-19 16:26:43 PDT
I'll fix this bug.  

Canceling the pending load and starting the new one fixes the crash, but we are
also going to get out of sync with the mPendingInstanceCount in the model.  Also
looks like we'll have the same problem with mPendingInstanceCount if we remove
the src attribute with script, too.  We'll call CloneInlineInstance to set up
the instance document with any inline instance that may be there, but won't let
the model know that we're good to go.  I'll put up a testcase for this scenario.
Comment 4 aaronr 2005-09-19 16:46:49 PDT
Created attachment 196721 [details]
testcase2

testcase to test removal of the 'src' attribute
Comment 5 aaronr 2005-09-19 18:47:14 PDT
Created attachment 196730 [details] [diff] [review]
draft fix

fixes the first testcase.  Not the second, but maybe this is related to the
itemsets not refreshing bug.  Will look more at it tomorrow.
Comment 6 aaronr 2005-09-20 16:26:19 PDT
Created attachment 196850 [details]
testcase2

forgot to put xmlns="" on the instance data.  Won't match the cloned inline
instance without it.
Comment 7 aaronr 2005-09-21 14:43:20 PDT
Created attachment 196947 [details] [diff] [review]
latest patch

fixed some comments.  Ready for review.
Comment 8 aaronr 2005-09-22 12:29:29 PDT
Created attachment 197068 [details] [diff] [review]
fixes comment by smaug

per IRC conversation with smaug, I moved mChannel test from ::AttributeSet to
::LoadExternalInstance which also removes the need for a restart parameter to
LoadExternalInstance.
Comment 9 Doron Rosenberg (IBM) 2005-09-22 19:09:55 PDT
Comment on attachment 197068 [details] [diff] [review]
fixes comment by smaug

>Index: nsXFormsInstanceElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsInstanceElement.cpp,v
>retrieving revision 1.17
>diff -u -8 -p -r1.17 nsXFormsInstanceElement.cpp
>--- nsXFormsInstanceElement.cpp	16 Sep 2005 10:53:36 -0000	1.17
>+++ nsXFormsInstanceElement.cpp	22 Sep 2005 19:25:23 -0000
>@@ -97,19 +97,42 @@ nsXFormsInstanceElement::AttributeSet(ns
> 
> NS_IMETHODIMP
> nsXFormsInstanceElement::AttributeRemoved(nsIAtom *aName)
> {
>   if (mIgnoreAttributeChanges)
>     return NS_OK;
> 
>   if (aName == nsXFormsAtoms::src) {
>+    PRBool restart = PR_FALSE;
>+    if (mChannel) {
>+      // looks like we are trying to remove the src attribute while we are
>+      // already trying to load the external instance document.  We'll stop
>+      // the current load effort.
>+      restart = PR_TRUE;
>+      mChannel->Cancel(NS_BINDING_ABORTED);
>+      mChannel = nsnull;
>+      mListener = nsnull;
>+    }
>     // We no longer have an external instance to use.  Reset our instance
>     // document to whatever inline content we have.
>-    return CloneInlineInstance();
>+    nsresult rv = CloneInlineInstance();
>+
>+    // if we had already started to load an external instance document, then
>+    // as part of that we would have told the model to wait for that external
>+    // document to load before it finishes the model construction.  Since we
>+    // aren't loading from an external document any longer, tell the model that
>+    // there is need to wait for us anymore.
>+    if (restart) {
>+      nsCOMPtr<nsIModelElementPrivate> model = GetModel();
>+      if (model) {
>+        model->InstanceLoadFinished(PR_TRUE);
>+      }
>+    }
>+    return rv;
>   }
> 
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> nsXFormsInstanceElement::BeginAddingChildren()
> {
>@@ -224,47 +247,45 @@ nsXFormsInstanceElement::OnChannelRedire
> // nsIStreamListener functions that we implement here.  And calling
> // mChannel->Cancel() is no guarantee that these other notifications won't come
> // through if the timing is wrong.  So we need to check for mElement below
> // before we handle any of the stream notifications.
> 
> NS_IMETHODIMP
> nsXFormsInstanceElement::OnStartRequest(nsIRequest *request, nsISupports *ctx)
> {
>-  if (!mElement) {
>+  if (!mElement || !mListener) {
>     return NS_OK;
>   }
>-  NS_ASSERTION(mListener, "No stream listener for document!");
>   return mListener->OnStartRequest(request, ctx);
> }
> 
> NS_IMETHODIMP
> nsXFormsInstanceElement::OnDataAvailable(nsIRequest *aRequest,
>                                          nsISupports *ctxt,
>                                          nsIInputStream *inStr,
>                                          PRUint32 sourceOffset,
>                                          PRUint32 count)
> {
>-  if (!mElement) {
>+  if (!mElement || !mListener) {
>     return NS_OK;
>   }
>-  NS_ASSERTION(mListener, "No stream listener for document!");
>   return mListener->OnDataAvailable(aRequest, ctxt, inStr, sourceOffset, count);
> }
> 
> NS_IMETHODIMP
> nsXFormsInstanceElement::OnStopRequest(nsIRequest *request, nsISupports *ctx,
>                                        nsresult status)
> {
>-  mChannel = nsnull;
>   if (status == NS_BINDING_ABORTED) {
>     // looks like our element has already been destroyed.  No use continuing on.
>     return NS_OK;
>   }
> 
>+  mChannel = nsnull;
>   NS_ASSERTION(mListener, "No stream listener for document!");
>   mListener->OnStopRequest(request, ctx, status);
> 
>   PRBool succeeded = NS_SUCCEEDED(status);
>   if (!succeeded)
>     mDocument = nsnull;
> 
>   if (mDocument) {
>@@ -421,16 +442,28 @@ nsXFormsInstanceElement::CloneInlineInst
>   return rv;
> }
> 
> void
> nsXFormsInstanceElement::LoadExternalInstance(const nsAString &aSrc)
> {
>   nsresult rv = NS_ERROR_FAILURE;
> 
>+  PRBool restart = PR_FALSE;
>+  if (mChannel) {
>+    // probably hit this condition because someone changed the value of our
>+    // src attribute while we are already trying to load the previously
>+    // specified document.  We'll stop the current load effort and kick off the
>+    // new attempt.
>+    restart = PR_TRUE;
>+    mChannel->Cancel(NS_BINDING_ABORTED);
>+    mChannel = nsnull;
>+    mListener = nsnull;
>+  }
>+
>   // Check whether we are an instance document ourselves
>   nsCOMPtr<nsIDOMDocument> domDoc;
>   mElement->GetOwnerDocument(getter_AddRefs(domDoc));
>   nsCOMPtr<nsIDocument> doc(do_QueryInterface(domDoc));
>   if (doc) {
>     if (doc->GetProperty(nsXFormsAtoms::isInstanceDocument)) {
>       /// Property exists, which means we are an instance document trying
>       /// to load an external instance document. We do not allow that.
>@@ -474,17 +507,23 @@ nsXFormsInstanceElement::LoadExternalIns
>           }
>         }
>       }
>     }
>   }
> 
>   nsCOMPtr<nsIModelElementPrivate> model = GetModel();
>   if (model) {
>-    model->InstanceLoadStarted();
>+    // if this isn't the first time that this instance element has tried
>+    // to load an external document, then we don't need to tell the model
>+    // to wait again.  It would screw up the counter it uses.  But if this
>+    // isn't a restart, then by golly, the model better wait for us!
>+    if (!restart) {
>+      model->InstanceLoadStarted();
>+    }
>     if (NS_FAILED(rv)) {
>       model->InstanceLoadFinished(PR_FALSE);
>     }
>   }
> }
> 
> nsresult
> nsXFormsInstanceElement::CreateInstanceDocument()
Comment 10 Doron Rosenberg (IBM) 2005-09-26 07:22:19 PDT
checked into trunk.  I assume we should check this into branch?
Comment 11 Doron Rosenberg (IBM) 2005-09-26 10:42:11 PDT
checked into branch

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