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

RESOLVED FIXED

Status

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

People

(Reporter: smaug, Assigned: aaronr)

Tracking

({crash, fixed1.8})

Trunk
crash, fixed1.8

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 3 obsolete attachments)

4.92 KB, text/xml
Details
1.55 KB, application/xhtml+xml
Details
1.68 KB, application/xhtml+xml
Details
5.25 KB, patch
Doron Rosenberg (IBM)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

12 years ago
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
(Assignee)

Comment 1

12 years ago
Created attachment 196698 [details]
external instance data for testcase
(Assignee)

Comment 2

12 years ago
Created attachment 196699 [details]
testcase

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

Updated

12 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Handle changes to the src attribute of the instance element. → [CRASH] Handle changes to the src attribute of the instance element.
(Assignee)

Comment 3

12 years ago
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.
Status: NEW → ASSIGNED
(Assignee)

Comment 4

12 years ago
Created attachment 196721 [details]
testcase2

testcase to test removal of the 'src' attribute
(Assignee)

Comment 5

12 years ago
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.
(Assignee)

Comment 6

12 years ago
Created attachment 196850 [details]
testcase2

forgot to put xmlns="" on the instance data.  Won't match the cloned inline
instance without it.
Attachment #196721 - Attachment is obsolete: true
(Assignee)

Comment 7

12 years ago
Created attachment 196947 [details] [diff] [review]
latest patch

fixed some comments.  Ready for review.
Attachment #196730 - Attachment is obsolete: true
Attachment #196947 - Flags: review?(smaug)
(Assignee)

Updated

12 years ago
Attachment #196947 - Flags: superreview?(doronr)
(Reporter)

Updated

12 years ago
Attachment #196947 - Flags: review?(smaug) → review+
(Assignee)

Comment 8

12 years ago
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.
Attachment #196947 - Attachment is obsolete: true
Attachment #197068 - Flags: review?(doronr)
(Assignee)

Updated

12 years ago
Attachment #196947 - Flags: superreview?(doronr)

Comment 9

12 years ago
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()
Attachment #197068 - Flags: superreview+

Updated

12 years ago
Attachment #197068 - Flags: superreview+
Attachment #197068 - Flags: review?(doronr)
Attachment #197068 - Flags: review+
checked into trunk.  I assume we should check this into branch?
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
checked into branch
Keywords: fixed1.8
Whiteboard: xf-to-branch

Updated

12 years ago
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.