57597 - Onload Event processing of Document.load fails with local xml file

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Description

[Bob Clary [:bc] (inactive)]

Scenario: Using Document.load to load an xml document xmlconf.xml file from
the NIST/OASIS XML Parser test suite asynchronously using addEventListener for
'onload' notification.

Using Mozilla/2000102108 on Win2k, Crashes when attempting to load xmlconf.xml
locally (on the same machine as the client) but does not crash when accessing
the file from OASIS.

Crashes at nsDOMEvent::GetTarget (nsDOMEvent.cpp, line 175) when dereferencing
a null pointer mPresContext.  See below for stack trace.

Steps to reproduce:

1.  download attached test html page.
2.  download attached xmlconf.xml file into same directory.
3.  attempt to run test page.

Expected behavior: nothing
Actual behavior: crash

4.  edit test html page to point to remote OASIS copy of xmlconf.xml
5.  attempt to run test page.

Expected behavior: nothing
Actual behavior: nothing

It sounds like some kind of race condition that only appears when the 'load'
event fires too quickly.

Stack Trace:
nsDOMEvent::GetTarget(nsDOMEvent * const 0x02fcb190, nsIDOMEventTarget * *
0x0012e598) line 175 + 16 bytes
GetEventProperty(JSContext * 0x02e3a188, JSObject * 0x02f300c8, long -3, long *
0x0012f00c) line 98 + 19 bytes
js_GetProperty(JSContext * 0x02e3a188, JSObject * 0x02f300c8, long 12559864,
long * 0x0012f00c) line 2096 + 125 bytes
js_Interpret(JSContext * 0x02e3a188, long * 0x0012f194) line 2437 + 1544 bytes
js_Invoke(JSContext * 0x02e3a188, unsigned int 1, unsigned int 2) line 807 + 13
bytes
js_InternalInvoke(JSContext * 0x02e3a188, JSObject * 0x02c71078, long 46600216,
unsigned int 0, unsigned int 1, long * 0x0012f300, long * 0x0012f2bc) line 879 +
20 bytes
JS_CallFunctionValue(JSContext * 0x02e3a188, JSObject * 0x02c71078, long
46600216, unsigned int 1, long * 0x0012f300, long * 0x0012f2bc) line 3193 + 31 bytes
nsJSContext::CallEventHandler(nsJSContext * const 0x02e46f88, void * 0x02c71078,
void * 0x02c71018, unsigned int 1, void * 0x0012f300, int * 0x0012f2fc, int 0)
line 907 + 33 bytes
nsJSDOMEventListener::HandleEvent(nsIDOMEvent * 0x02fcb194) line 88 + 52 bytes
nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x027479c8,
nsIDOMEvent * 0x02fcb194, nsIDOMEventTarget * 0x02e04be4, unsigned int 1,
unsigned int 7) line 788 + 19 bytes
nsEventListenerManager::HandleEvent(nsIPresContext * 0x00000000, nsEvent *
0x0012f76c, nsIDOMEvent * * 0x0012f73c, nsIDOMEventTarget * 0x02e04be4, unsigned
int 7, nsEventStatus * 0x0012f7b0) line 1365 + 39 bytes
nsDocument::HandleDOMEvent(nsDocument * const 0x02e04bb8, nsIPresContext *
0x00000000, nsEvent * 0x0012f76c, nsIDOMEvent * * 0x0012f73c, unsigned int 1,
nsEventStatus * 0x0012f7b0) line 3033
nsXMLDocument::EndLoad(nsXMLDocument * const 0x02e04bb8) line 623
nsXMLContentSink::DidBuildModel(nsXMLContentSink * const 0x02f008d8, int 1) line 273
CWellFormedDTD::DidBuildModel(CWellFormedDTD * const 0x02edcfc0, unsigned int 0,
int 1, nsIParser * 0x02742ae0, nsIContentSink * 0x02f008d8) line 293 + 20 bytes
nsParser::DidBuildModel(unsigned int 0) line 1435 + 60 bytes
nsParser::ResumeParse(int 1, int 1) line 1954
nsParser::OnStopRequest(nsParser * const 0x02742ae8, nsIChannel * 0x02d59990,
nsISupports * 0x00000000, unsigned int 0, const unsigned short * 0x100a66c8
gCommonEmptyBuffer) line 2395 + 19 bytes
nsHTTPFinalListener::OnStopRequest(nsHTTPFinalListener * const 0x02a37fd0,
nsIChannel * 0x02d59990, nsISupports * 0x00000000, unsigned int 0, const
unsigned short * 0x100a66c8 gCommonEmptyBuffer) line 1158 + 42 bytes
InterceptStreamListener::OnStopRequest(InterceptStreamListener * const
0x02dce268, nsIChannel * 0x02d59990, nsISupports * 0x00000000, unsigned int 0,
const unsigned short * 0x100a66c8 gCommonEmptyBuffer) line 1207
nsHTTPChannel::ResponseCompleted(nsIStreamListener * 0x02dce268, unsigned int 0,
const unsigned short * 0x100a66c8 gCommonEmptyBuffer) line 1920 + 42 bytes
nsHTTPServerListener::OnStopRequest(nsHTTPServerListener * const 0x00cc68f8,
nsIChannel * 0x00cd9c34, nsISupports * 0x02d59990, unsigned int 0, const
unsigned short * 0x100a66c8 gCommonEmptyBuffer) line 729
nsOnStopRequestEvent::HandleEvent(nsOnStopRequestEvent * const 0x02e36d10) line 302
nsStreamListenerEvent::HandlePLEvent(PLEvent * 0x03008630) line 97 + 12 bytes
PL_HandleEvent(PLEvent * 0x03008630) line 576 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00bb6660) line 509 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x00410056, unsigned int 49380, unsigned int 0,
long 12281440) line 1054 + 9 bytes
USER32! 77e148dc()
USER32! 77e14aa7()
USER32! 77e266fd()
nsAppShellService::Run(nsAppShellService * const 0x00c01ce0) line 408
main1(int 2, char * * 0x00496c60, nsISupports * 0x00000000) line 1021 + 32 bytes
main(int 2, char * * 0x00496c60) line 1261 + 37 bytes
mainCRTStartup() line 338 + 17 bytes

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: Test HTML file causing crash. Uses attached xmlconf.xml

Comment 2

[Bob Clary [:bc] (inactive)]

Attachment: xmlconf.xml

Comment 3

[Loco]

Pawning off on petersen and XML folks.

Comment 4

[Bob Clary [:bc] (inactive)]

Crashed Mozilla using debug build from 10/31/00 AM sources on win2k by clicking 
on the Test HTML file.  

Note that the Test HTML file is setup to be saved to a server along with the 
xmlconf.xml file and run from there. Saving the attachments and running the 
test still crashes Mozilla.  In addition, clicking on the Test HTML file 
attachment crashes Mozilla, even though it references the xmlconf.xml file that 
it can not find.  This is suprising since I would have thought the fix for bug 
40121 that apparently fixed bug 40982 would have fixed any empty document 
problems here as well.

Happy Halloween!

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I noticed there is something wrong in the GetTarget method. This does not fix
this crash, but this should also be fixed at some point. The problem is that the
current code defines targetContent pointer which might be used uninitialized if
GetEventStateManager fails.

Index: src/nsDOMEvent.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/events/src/nsDOMEvent.cpp,v
retrieving revision 1.96
diff -u -r1.96 nsDOMEvent.cpp
--- nsDOMEvent.cpp      2000/09/19 21:46:59     1.96
+++ nsDOMEvent.cpp      2000/11/01 22:11:23
@@ -169,12 +169,11 @@

        *aTarget = nsnull;

-  nsIEventStateManager *manager;
-  nsIContent *targetContent;
+  nsCOMPtr<nsIEventStateManager> manager;
+  nsCOMPtr<nsIContent> targetContent;

-  if (NS_OK == mPresContext->GetEventStateManager(&manager)) {
-    manager->GetEventTargetContent(mEvent, &targetContent);
-    NS_RELEASE(manager);
+  if (NS_OK == mPresContext->GetEventStateManager(getter_AddRefs(manager))) {
+    manager->GetEventTargetContent(mEvent, getter_AddRefs(targetContent));
   }

   if (targetContent) {
@@ -182,7 +181,6 @@
       *aTarget = mTarget;
       NS_ADDREF(mTarget);
     }
-    NS_RELEASE(targetContent);
   }
   else {
     //Always want a target.  Use document if nothing else.

Comment 6

[Jay Patel [:jay]]

This is a talkback topcrasher...adding topcrash keyword and
[@ nsDOMEvent::GetTarget] for tracking.

Comment 7

[Axel Hecht]

we shouldn't crash on this one, I will attach a patch that returns null, if
the mPresContext isn't set.
mPresContext seems to be null for nsXMLDocument::EndLoad() sets this in its
HandleDOMEvent().
Peter Van der Beken sent me a patch to change that, but I have no clue if that
works.
I try this for a document.implementation.createDocument("","",null), which 
doesn't have a prescontext anyway, so peter's code doesn't help. What's the 
right thing to do there?

Axel

Comment 8

[Axel Hecht]

Attachment: null check for mPresContext and early return NS_OK

Comment 9

[Nisheeth Ranjan]

Bringing in to Mozilla 0.8.1...

Comment 10

[Nisheeth Ranjan]

Attachment: Slightly modified patch for null check of mPresContext

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Hmm... I am not sure if a nullcheck is the correct thing here. Doesn't that mean
the event is incorrect (wrong event target) or it does not work at all. Maybe
there is another way to get the needed information to set the target?

Comment 12

[Nisheeth Ranjan]

Adding joki and saari to the cc list to get their input...

nsDOMEvent::GetTarget() makes the assumption that mPresContext is non null.  
But, for this document.load() test case, that assumption is false and we crash.  
What needs to change to account for this faulty assumption?  Is the 
simplistic null check of mPresContext enough?  You guys are familiar with the 
big picture here better than Heikki or me, so please comment.  Thanks!

Comment 13

[joki (gone)]

The net affect of this is that the event will still fire but the event target
(and most any other context info) will be wrong.  Better than crashing as a
start but not a complete fix.  I say we check this in, remove the crash keyword
and keep working on it.

r=joki

Comment 14

[Nisheeth Ranjan]

I've checked in the null check for the mPresContext that fixes the crash.  
Changing summary and removing crash keywords.  Agreed that we need to continue 
to work on this bug to fix load event propagation across documents that do not 
have presentation contexts attached to them..  But, we'll attack that in the 7.0 
timeframe.  Futuring.

Comment 15

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

(In reply to comment #14)
> Agreed that we need to continue 
> to work on this bug to fix load event propagation across documents that do not 
> have presentation contexts attached to them.

Bug 234455 and bug 335251 changed the way load events work and
bug 234455 fixed event target handling.