XForms bug 304373 - crash loading a poorly formed xforms with external instance data. Patch by aaronr, r=smaug,me

extensions/xforms/nsXFormsInstanceElement.cpp

@@ -70,6 +70,12 @@ nsXFormsInstanceElement::nsXFormsInstanceElement()
 NS_IMETHODIMP
 nsXFormsInstanceElement::OnDestroyed()
 {
+  if (mChannel) {
+    // better be a good citizen and tell the browser that we don't need this
+    // resource anymore.
+    mChannel->Cancel(NS_BINDING_ABORTED);
+    mChannel = nsnull;
+  }
   mListener = nsnull;
   mElement = nsnull;
   return NS_OK;
@@ -209,9 +215,23 @@ nsXFormsInstanceElement::OnChannelRedirect(nsIChannel *OldChannel,
 
 // nsIStreamListener
 
+// It is possible that mListener could be null here.  If the document hit a
+// parsing error after we've already started to load the external source, then
+// Mozilla will destroy all of the elements and the document in order to load
+// the parser error page.  This will cause mListener to become null but since
+// the channel will hold a nsCOMPtr to the nsXFormsInstanceElement preventing
+// it from being freed up, the channel will still be able to call the
+// 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) {
+    return NS_OK;
+  }
   NS_ASSERTION(mListener, "No stream listener for document!");
   return mListener->OnStartRequest(request, ctx);
 }
@@ -223,6 +243,9 @@ nsXFormsInstanceElement::OnDataAvailable(nsIRequest *aRequest,
                                          PRUint32 sourceOffset,
                                          PRUint32 count)
 {
+  if (!mElement) {
+    return NS_OK;
+  }
   NS_ASSERTION(mListener, "No stream listener for document!");
   return mListener->OnDataAvailable(aRequest, ctxt, inStr, sourceOffset, count);
 }
@@ -231,6 +254,12 @@ 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;
+  }
+
   NS_ASSERTION(mListener, "No stream listener for document!");
   mListener->OnStopRequest(request, ctx, status);
 
@@ -416,16 +445,15 @@ nsXFormsInstanceElement::LoadExternalInstance(const nsAString &aSrc)
           // Using the same load group as the main document and creating
           // the channel with LOAD_NORMAL flag delays the dispatching of
           // the 'load' event until all instance data documents have been loaded.
-          nsCOMPtr<nsIChannel> docChannel;
-          NS_NewChannel(getter_AddRefs(docChannel), uri, nsnull, loadGroup,
+          NS_NewChannel(getter_AddRefs(mChannel), uri, nsnull, loadGroup,
                         nsnull, nsIRequest::LOAD_NORMAL);
 
-          if (docChannel) {
-            rv = newDoc->StartDocumentLoad(kLoadAsData, docChannel, loadGroup, nsnull,
+          if (mChannel) {
+            rv = newDoc->StartDocumentLoad(kLoadAsData, mChannel, loadGroup, nsnull,
                                            getter_AddRefs(mListener), PR_TRUE);
             if (NS_SUCCEEDED(rv)) {
-              docChannel->SetNotificationCallbacks(this);
-              rv = docChannel->AsyncOpen(this, nsnull);
+              mChannel->SetNotificationCallbacks(this);
+              rv = mChannel->AsyncOpen(this, nsnull);
             }
           }
         } else {

extensions/xforms/nsXFormsInstanceElement.h

@@ -93,6 +93,7 @@ private:
   nsIDOMElement              *mElement;
   nsCOMPtr<nsIStreamListener> mListener;
   PRBool                      mIgnoreAttributeChanges;
+  nsCOMPtr<nsIChannel>        mChannel;
 };
 
 NS_HIDDEN_(nsresult)

extensions/xforms/nsXFormsLabelElement.cpp

@@ -79,6 +79,7 @@ public:
   NS_IMETHOD Refresh();
 
   NS_IMETHOD OnCreated(nsIXTFBindableElementWrapper *aWrapper);
+  NS_IMETHOD OnDestroyed();
 
   // nsIXTFElement overrides
   NS_IMETHOD ChildInserted(nsIDOMNode *aChild, PRUint32 aIndex);
@@ -117,6 +118,18 @@ nsXFormsLabelElement::OnCreated(nsIXTFBindableElementWrapper *aWrapper)
   return NS_OK;
 }
 
+NS_IMETHODIMP
+nsXFormsLabelElement::OnDestroyed()
+{
+  if (mChannel) {
+    // better be a good citizen and tell the browser that we don't need this
+    // resource anymore.
+    mChannel->Cancel(NS_BINDING_ABORTED);
+    mChannel = nsnull;
+  }
+  return nsXFormsDelegateStub::OnDestroyed();
+}
+
 NS_IMETHODIMP
 nsXFormsLabelElement::ChildInserted(nsIDOMNode *aChild, PRUint32 aIndex)
 {
@@ -285,10 +298,26 @@ nsXFormsLabelElement::GetInterface(const nsIID &aIID, void **aResult)
 
 // nsIStreamListener
 
+// It is possible that the label element could well on its way to invalid by
+// the time that the below handlers are called.  If the document hit a
+// parsing error after we've already started to load the external source, then
+// Mozilla will destroy all of the elements and the document in order to load
+// the parser error page.  This will cause mElement to become null but since
+// the channel will hold a nsCOMPtr to the nsXFormsLabelElement preventing
+// it from being freed up, the channel will still be able to call the
+// 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
 nsXFormsLabelElement::OnStartRequest(nsIRequest *aRequest,
                                      nsISupports *aContext)
 {
+  if (!mElement) {
+    return NS_OK;
+  }
+
   // Only handle data from text files for now.  Cancel any other requests.
   nsCOMPtr<nsIChannel> channel(do_QueryInterface(aRequest));
   if (channel) {
@@ -309,6 +338,10 @@ nsXFormsLabelElement::OnDataAvailable(nsIRequest *aRequest,
                                       PRUint32 aOffset,
                                       PRUint32 aCount)
 {
+  if (!mElement) {
+    return NS_OK;
+  }
+
   nsresult rv;
   PRUint32 size, bytesRead;
   char buffer[256];
@@ -332,10 +365,13 @@ nsXFormsLabelElement::OnStopRequest(nsIRequest *aRequest,
 {
   // Done with load request, so null out channel member
   mChannel = nsnull;
+  if (!mElement) {
+    return NS_OK;
+  }
 
   if (NS_FAILED(aStatusCode)) {
     // If we received NS_BINDING_ABORTED, then we were cancelled by a later
-    // AttributeSet() call.  Don't do anything and return.
+    // AttributeSet() or AttributeRemoved call.  Don't do anything and return.
     if (aStatusCode == NS_BINDING_ABORTED)
       return NS_OK;
 

extensions/xforms/nsXFormsSubmissionElement.cpp

@@ -355,11 +355,27 @@ nsXFormsSubmissionElement::GetInterface(const nsIID & aIID, void **aResult)
 
 // nsIChannelEventSink
 
+// It is possible that the submission element could well on its way to invalid
+// by the time that the below handlers are called.  If the document was
+// destroyed after we've already started submitting data then this will cause
+// mElement to become null.  Since the channel will hold a nsCOMPtr
+// to the nsXFormsSubmissionElement as a callback to the channel, this prevents
+// it from being freed up.  The channel will still be able to call the
+// 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
 nsXFormsSubmissionElement::OnChannelRedirect(nsIChannel *aOldChannel,
                                              nsIChannel *aNewChannel,
                                              PRUint32    aFlags)
 {
+  if (!mElement) {
+    return NS_OK;
+  }
+
   NS_PRECONDITION(aNewChannel, "Redirect without a channel?");
   nsCOMPtr<nsIURI> newURI;
   nsresult rv = aNewChannel->GetURI(getter_AddRefs(newURI));
@@ -391,6 +407,10 @@ nsXFormsSubmissionElement::OnStopRequest(nsIRequest *request, nsISupports *ctx,
 {
   LOG(("xforms submission complete [status=%x]\n", status));
 
+  if (!mElement) {
+    return NS_OK;
+  }
+
   nsCOMPtr<nsIChannel> channel = do_QueryInterface(request);
   NS_ASSERTION(channel, "request should be a channel");