Last Comment Bug 357716 - Crash in model/schema loader when main page fails to load
: Crash in model/schema loader when main page fails to load
Status: RESOLVED FIXED
: fixed1.8.0.12, fixed1.8.1.4
Product: Core
Classification: Components
Component: XForms (show other bugs)
: Trunk
: x86 Windows XP
: -- critical (vote)
: ---
Assigned To: Steve Speicher
: Stephen Pride
Mentors:
Depends on:
Blocks: 353738
  Show dependency treegraph
 
Reported: 2006-10-23 13:13 PDT by Steve Speicher
Modified: 2007-04-23 15:46 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
schema used for test case (2.57 KB, application/xml)
2006-10-23 13:14 PDT, Steve Speicher
no flags Details
test case that causes crash (5.86 KB, application/xhtml+xml)
2006-10-23 13:17 PDT, Steve Speicher
no flags Details
patch (894 bytes, patch)
2006-10-23 13:23 PDT, Steve Speicher
aaronr: review-
Details | Diff | Review
patch #2 (1.23 KB, patch)
2006-10-24 05:09 PDT, Steve Speicher
aaronr: review+
Details | Diff | Review
patch with Aaron's comments about comments addressed (1.39 KB, patch)
2006-10-31 07:49 PST, Steve Speicher
bugs: review+
Details | Diff | Review

Description Steve Speicher 2006-10-23 13:13:36 PDT
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 2006-10-23 13:14:58 PDT
Created attachment 243243 [details]
schema used for test case
Comment 2 Steve Speicher 2006-10-23 13:17:54 PDT
Created attachment 243245 [details]
test case that causes crash
Comment 3 Steve Speicher 2006-10-23 13:23:46 PDT
Created attachment 243247 [details] [diff] [review]
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 2006-10-23 14:16:44 PDT
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 2006-10-23 14:42:03 PDT
(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 2006-10-24 05:09:41 PDT
Created attachment 243337 [details] [diff] [review]
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 2006-10-25 16:50:02 PDT
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 2006-10-31 07:49:04 PST
Created attachment 244203 [details] [diff] [review]
patch with Aaron's comments about comments addressed
Comment 9 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2006-10-31 08:04:31 PST
Checked in
Comment 10 aaronr 2007-04-23 15:46:47 PDT
checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16

Note You need to log in before you can comment on or make changes to this bug.