357716 - Crash in model/schema loader when main page fails to load

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Steve Speicher]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061011 Minefield/3.0a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061011 Minefield/3.0a1

Here's the steps that appear to be causing the problem:
1) a thread is launch to load the model's associated schema
2) main parsing of document fails, ill-formed content
3) schema loading finishes an invokes nsXFormsModelElement::OnLoad()
  which calls FinishConstruction(), which accesses mElement which is
  null and halt

One quick/permanent fix is to test mElement in FinishConstruction()

It does not always occur.  I usually have more luck making it occur when xsd is located on slower server so it takes it's thread longer.  Will attempt to post reasonable test case.

This happens in FF1.5.0.7/0.7 and trunk.

Reproducible: Sometimes

Comment 1

[Steve Speicher]

Attachment: schema used for test case

Comment 2

[Steve Speicher]

Attachment: test case that causes crash

Comment 3

[Steve Speicher]

Attachment: patch

Simple approach to guard against such a case.  Would it be better to ensure IsComplete() returns false, also or in addition to?

Comment 4

[aaronr]

Comment on attachment 243247 [details] [diff] [review]
patch

I don't mind if you leave the NS_ENSURE_STATE(mElement) here.  That might us catch other errors in the future.  But you still have the error that is causing this problem (calling FinishConstruction before it should be called).  I'd think that you should have a test in nsXFormsModelElement::OnLoad to see if model is ready to go through the 'finish' processing.

Comment 5

[aaronr]

(In reply to comment #4)
> (From update of attachment 243247 [details] [diff] [review] [edit])
> I don't mind if you leave the NS_ENSURE_STATE(mElement) here.  That might us
> catch other errors in the future.  But you still have the error that is causing
> this problem (calling FinishConstruction before it should be called).  I'd
> think that you should have a test in nsXFormsModelElement::OnLoad to see if
> model is ready to go through the 'finish' processing.
> 

Or we change the concept of IsComplete to be more than just testing to see if external resources are loaded.

Comment 6

[Steve Speicher]

Attachment: patch #2

Doing some more testing and investigation, not sure there is much more to do than adding an additional test in OnLoad().  Trying to load large external instances and having same ill formed main document, no crashes.

IsComplete() is used a few times to determine if schemas have completed loading (not that the main document has loaded successfully).  Other calls to FinishConstruction() are deep within other calls that make extensive use of mElement before calling it.

Comment 7

[aaronr]

Comment on attachment 243337 [details] [diff] [review]
patch #2

>Index: nsXFormsModelElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsModelElement.cpp,v
>retrieving revision 1.135
>diff -u -8 -p -r1.135 nsXFormsModelElement.cpp
>--- nsXFormsModelElement.cpp	8 Oct 2006 14:15:01 -0000	1.135
>+++ nsXFormsModelElement.cpp	24 Oct 2006 10:22:56 -0000
>@@ -1375,17 +1375,17 @@ nsXFormsModelElement::Refresh()
> 
> // nsISchemaLoadListener
> 
> NS_IMETHODIMP
> nsXFormsModelElement::OnLoad(nsISchema* aSchema)
> {
>   mSchemaCount++;
> 
>-  if (IsComplete()) {
>+  if (IsComplete() && mElement) {

grab comment from below

>     nsresult rv = FinishConstruction();
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>     MaybeNotifyCompletion();
>   }
> 
>   return NS_OK;
> }
>@@ -2072,16 +2072,20 @@ nsXFormsModelElement::BackupOrRestoreIns
>   }
> 
> }
> 
> 
> nsresult
> nsXFormsModelElement::FinishConstruction()
> {
>+  // If there is no model element, then schema loading finished after
>+  // main page failed to load.
>+  NS_ENSURE_STATE(mElement);
>+

I'd say that after this patch goes in, then this comment doesn't really apply here anymore, does it?  You are preventing schema loading from being a problem here :)  I'd say move this comment to ::OnLoad where you are testing for mElement.  Keep the NS_ENSURE_STATE here.  Maybe use a comment like, "ensure that FinishConstruction isn't called due to some callback or event handler after the model has started going through its destruction phase"

with those, r=me

Comment 8

[Steve Speicher]

Attachment: patch with Aaron's comments about comments addressed

Comment 9

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

Checked in

Comment 10

[aaronr]

checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16