308986 - [CRASH] Handle changes to the src attribute of the instance element.

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

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

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]

Attachment: external instance data for testcase

Comment 2

[aaronr]

Attachment: testcase

testcase crashes on load.  Should show a select1 with all 50 U.S. states in it.

Comment 3

[aaronr]

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]

Attachment: testcase2

testcase to test removal of the 'src' attribute

Comment 5

[aaronr]

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

Attachment: testcase2

forgot to put xmlns="" on the instance data.  Won't match the cloned inline
instance without it.

Comment 7

[aaronr]

Attachment: latest patch

fixed some comments.  Ready for review.

Comment 8

[aaronr]

Attachment: 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)]

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)]

checked into trunk.  I assume we should check this into branch?

Comment 11

[Doron Rosenberg (IBM)]

checked into branch