304373 - CRASH: loading a poorly formed xforms xhtml with external instance data

Status: VERIFIED FIXED

(Core Graveyard :: XForms, defect)

Description

[aaronr]

We are crashing in nsXFormsInstanceElement::OnStartRequest because of a null
mListener.  Looks like what is happening is that we kick off the AsyncOpen to
open up the instance document and then when the parser finds a missing end tag
or something, then it will destroy the main document to put up the parsing
error, thus destroying the nsXFormsInstanceElement object and with it, its
mListener.

We need to figure out why the channel isn't smart enough to recognize that the
listener that we passed it is no longer good.  Is it our fault?

Comment 1

[aaronr]

Attachment: testcase

loading this testcase crashes.	It should show a parsing error because it is
missing the </body> tag.

Comment 2

[aaronr]

Attachment: possible fix

fixed the crash, plus some other places that might encounter issues if the
document is pulled out from under them like this.

Comment 3

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

Comment on attachment 192458 [details] [diff] [review]
possible fix


> nsXFormsInstanceElement::nsXFormsInstanceElement()
>   : mElement(nsnull)
>   , mIgnoreAttributeChanges(PR_FALSE)
>+  , mChannel(nsnull)

No need to set mChannel nsnull.



> 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 NS_OK;

return nsXFormsDelegateStub::OnDestroyed():

With those two, r=me.

Comment 4

[aaronr]

Attachment: patch with smaug's comments

Comment 5

[Doron Rosenberg (IBM)]

Comment on attachment 192756 [details] [diff] [review]
patch with smaug's comments

>+// 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)

NIT: why 2 newlines before the method?

Comment 6

[aaronr]

(In reply to comment #5)
> >+// 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)
> 
> NIT: why 2 newlines before the method?
> 

Just to set it off a little since the comment applies to the whole following
section of listener callbacks and not just the one immediately below it.  But
I'm not married to the idea.  If you think one newline is right, then that is
fine with me.

Comment 7

[Doron Rosenberg (IBM)]

Comment on attachment 192756 [details] [diff] [review]
patch with smaug's comments

no idea how the sr happend, removing

Comment 8

[Doron Rosenberg (IBM)]

checked into trunk, leaving open for branch

Comment 9

[Asa Dotzler [:asa]]

Comment on attachment 192756 [details] [diff] [review]
patch with smaug's comments

in order to land on the branch, this needs to be resolved then verified on the
trunk.

Comment 10

[Doron Rosenberg (IBM)]

that has been checked/verified already

Comment 11

[Asa Dotzler [:asa]]

Time is short for 1.8b4. If this isn't landed today, it's not going to make the
train.

Comment 12

[Doron Rosenberg (IBM)]

already landed.

Comment 13

[Tracy Walker [:tracy]]

verified on Firefox 1.4 -mozilla1.8 branch- Win, Lin and Mac : 2005-09-07