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

RESOLVED FIXED

Status

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

People

(Reporter: Steve Speicher, Assigned: Steve Speicher)

Tracking

({fixed1.8.0.12, fixed1.8.1.4})

Trunk
x86
Windows XP
fixed1.8.0.12, fixed1.8.1.4

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

11 years ago
Created attachment 243243 [details]
schema used for test case
(Assignee)

Comment 2

11 years ago
Created attachment 243245 [details]
test case that causes crash
(Assignee)

Comment 3

11 years ago
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?
Attachment #243247 - Flags: review?(aaronr)

Comment 4

11 years ago
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.
Attachment #243247 - Flags: review?(aaronr) → review-

Comment 5

11 years ago
(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.

Updated

11 years ago
Blocks: 353738
(Assignee)

Comment 6

11 years ago
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.
Attachment #243247 - Attachment is obsolete: true
Attachment #243337 - Flags: review?(aaronr)

Comment 7

11 years ago
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
Attachment #243337 - Flags: review?(aaronr) → review+

Updated

11 years ago
Assignee: xforms → sspeiche
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 8

11 years ago
Created attachment 244203 [details] [diff] [review]
patch with Aaron's comments about comments addressed
Attachment #243337 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #243337 - Attachment is obsolete: false
Attachment #243337 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

11 years ago
Attachment #244203 - Attachment description: patch #3 → patch with Aaron's comments about comments addressed

Updated

11 years ago
Attachment #243337 - Flags: review?(Olli.Pettay)

Updated

11 years ago
Attachment #244203 - Flags: review+

Comment 9

11 years ago
Checked in
Whiteboard: xf-to-branch

Updated

11 years ago
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 10

10 years ago
checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16
Keywords: fixed1.8.0.12, fixed1.8.1.4
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.