Closed Bug 57636 Opened 24 years ago Closed 24 years ago

Onload doesn't fire in javascript-generated documents

Categories

(Core :: DOM: Core & HTML, defect, P1)

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
Windows 95
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.1

People

(Reporter: bht237, Assigned: nisheeth_mozilla)

References

Details

(Keywords: dataloss, dom0, testcase, Whiteboard: [fix in hand, needs review] DIGBug)

Attachments

(13 files)

Simple test case (zip file)
928 bytes, application/octet-stream
Details
Additional testcase: document.write() in new window instead of frame
339 bytes, text/html
Details
Proposed partial fix, fixes simplest case
3.48 KB, patch
Details | Diff | Splinter Review
Tests the onLoad event of a frameset written with document.write()
684 bytes, text/html
Details
onload working with 3 pictures loading from 3 servers
1.61 KB, text/html
Details
Body/Image document.write test case
783 bytes, text/html
Details
First attempt at fixing the bug.
10.33 KB, patch
Details | Diff | Splinter Review
Updated patch
12.67 KB, patch
Details | Diff | Splinter Review
One more time...
23.29 KB, patch
Details | Diff | Splinter Review
Doc write of meta refresh tag - test case
737 bytes, text/html
Details
test.js
24 bytes, text/plain
Details
Doc write of a script src= tag: test case
925 bytes, text/html
Details
Final Patch
22.63 KB, patch
Details | Diff | Splinter Review
Two HTML files (index.htm and button.htm in attached zip file) test the the onload event of a simple document that is written with document.write(). The event does not fire as in all other JavaScript capable browsers.
Keywords: 4xp, dataloss, testcase
what buildid are you using?
Fails in both 2000102004 and Netscpae Preview 3 200092908.
over to DOM
Assignee: asa → jst
Status: UNCONFIRMED → NEW
Component: Browser-General → DOM Level 0
Ever confirmed: true
QA Contact: doronr → desale
Reassigning, Tom, I'm guessing this is a dup of one of your bugs...
Assignee: jst → joki
Is this really a critical dataloss bug? More testcases: succeeds javascript:'<HTML><BODY onload="alert(1)"></body></html>' fails javascript:document.write('<HTML><BODY onload="alert(1)"></body></html>'); fails data:text/html,<HTML><BODY onload="alert(1)"></body></html>
Yeah, I don't think this is really a dataloss bug, nor is this critical. Removing dataloss keyword and lowering severity to major.
Severity: critical → major
Keywords: dataloss
The comments made by Jesse Ruderman are interesting but complicate the case. Please Jesse by all means open a new bug if you feel that the javascript: URLs that you added must work. I would definitely agree with you to have them fixed and I offer my support for additional test cases etc. but this is more narrow in scope. This is NOT about javascript: URL's and JavaScript/DOM escape() problems. The case is fundamental: the onload event of a document.write() generated document is ignored/missing/not executed and this is data loss because the loading of a document fails. Not having onload events is not acceptable any more in a generation 6 browser. All other browsers execute them and web authors need them to build reliable applications. The application of this is the simple case where dynamic content is generated from objects. Any interaction with the dynamic content must be avoided until the generated document fires its onload event because otherwise the application will crash with a JavaScript error. If hte onload event is missing then errors cannot be avoided.
Keywords: dataloss
Adding martin.honnen@t-online.de, dannyg@dannyg.com to cc list, changing severity to blocker
Severity: major → blocker
I just have a comment on the dataloss keyword for now. It is my understanding that it is used for a case like, say, an email was lost (the server sent it to you but the reader lost it) or doing page refresh wipes out everything you have written on the form controls. load event not firing is not dataloss.
I feel sorry Heikki and I do agree with you on this (dataloss) as far as the definition goes. But I have no other means to express the severity of this. It's like you build a car for a customer, the customer buys it, takes the spark plugs out and complains that the car doesn't drive. Unfortunately, onload events are not less essential in dynamic web applications since Navigator 3 than spark plugs in a car. Whether the effect of the bug is dataloss or not then depends on the application and this is oviously debatable and I would tend to follow your definition anyway. However I am not removing the keyword for now.
This appears quite fundamental to me. Can this not have a higher priority? I wonder how http://bugzilla.mozilla.org/show_bug.cgi?id=60842 can be solved before the browser passes this test?
Priority: P3 → P1
If the additional testcase widens the scope and this is unacceptable then let me know and I will open a new bug for it.
Might be a dup of bug 35253.
I'am using the onLoad event on every page. And it seems to work more then 100% for me. Without the onLoad event, you can not even visit the pages! Take a look at: go.to/spacetubbies. Sorry for the jabadabadoe on the page. I'm working on a workaround for V3 banners. Need to know when I'am using 'Netschaap' as I'm using it, It seems sooooooooooooo slooooooooooooowwwwwwwwwwwwwwwwww compared to other browsers.So there is a workaround for! I'm using it.
Put this in onload.html ----------------------- <html> <head> <script language="JavaScript" src="onload.js" type="text/javascript"> </script> </head> <body onload="Init();alert('you see, it works, hit any key for further demonstration')"> This text is inside onload.html </body> </html> And this is onload.js --------------------- function Init() { document.write('And voila, it works.'); return; } Is this where you are looking for?? I know, it's not perfect. But it works. SM should do the job without tricks, but for now.... Henk-Johan (I'am not an expert, just a SM fool) More to watch: An unkown error has occured (804b0008) 'They forgot to assign a var!'
I also encountered this bug. My page is dynamically generating frame contents. onLoad handler is defined, but no onLoad is triggered. Looks like the existing test cases cover my instance. I'm surprised there aren't more reports of this. Note: I haven't run it, but I don't think HJ's workaround applies here. The problem here is that the page generated by document.write() contains an onLoad handler, but it's not called when the document is closed.
This works fine for me! Please take a look at it, and feel free to send me your comment. Regards HJ. <!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd"> <html> <head> <title>HJsProof.html</title> <script language="JavaScript1.2" type="text/javascript"> function HJsProof() { document.open(); var s1='<html><head><title>Title 2</title><script language=\"JavaScript1.2\" type=\"text/javascript\">'; var s2='function Init(){};function Baf(){ alert(\'This is my generated proof\');return;}Init();'; var s3='<\/script></head><body onload="Baf()">Inside body text</body></html>'; document.write(s1); document.write(s2); document.write(s3); document.close(); return; } </script> </head> <body> Say hello, Andy, this is HJsProof.html<br> <script language="JavaScript1.2" type="text/javascript"> <!-- HJsProof(); //--> </script> </body> </html>
The problem here is that the onload event is not fired in the special case where an out of bound (i.e. this happens *after* the document is done loading) document.open() .write('<body onload=...'); .close();. In that case the onload should IMO be fired in the document.close() call and I'll attach a patch that makes mozilla do that. The patch only fixes part of the problem tho, if someone document.write()'s out a frameset document the onload for the frameset document should be fired *after* the frames have fully loaded and that's a bit trickier, not sure who should look into that, cc:ing rpotts and nisheeth for input here.
The attached patch fixes the attached testcase (the first attachment) but it doesn't fix a testcase that involves framesets, could someone write up such a testcase?
Many thanks Johnny !!! :) I don't understand the internals of the browser, so please excuse my ignorance: You write that you use document.close() to fire the onLoad event. Does this mean that it can fire before any embedded media files such as images are loaded? This would be too early. Would it not be better to delegate the firing of the event to the document in such a way that onLoad event behaviour is inherited from a "normal" document? Otherwise this looks like a can of worms to me. I am assuming that a "normal" document has all this complex behaviour built-in and you don't want to re-invent the wheel. Anyway, I might just have misunderstood your statement.
Reporter, you're absolutely right, my patch only fixes the simplest case where there are no other external files (images or frames/iframes) involved. Reusing the code that normally fires the onload event is the obvious thing to do but that code relies on the document load being initiated and driven by the networking library so in this case where the document content is created and parsed in a script the current onload fireing code doesn't fit in well at all, AFAICT from looking at the code...
Maybe more time is needed to find a better solution. Sorry I can't help because I have no knowledge about Mozilla code. However, I can assure you that the onLoad event is a very precious device for JavaScript programmers. I definitely need it to avoid the most basic racing conditions for the simple reason that JavaScript often has no other way to determine whether objects in a document are ready for manipulation. A good example is a sound file that I want to control with LiveConnect. Embedded objects often have no events that can be used to trigger any scripts. Any example of course is debatable and some workaround may exist but from experience I know one cannot write stable, maintainable responsive user interfaces in JavaScript without onLoad events. As a thought, just consider the following code in a frame with a patch applied: <BODY onLoad="top.someFunction(self)"> I have this bad feeling that with the premature event one could get into the situation where "self" is not usable even in a very simple document. The other thing is security, see bug 57600 for a taste of this. With this understanding I cannot possibly ask for a patch that undermines the special purpose of the onLoad event. I would suggest to explore the possibilities of "Reusing the code that normally fires the onload event", as Johnny writes it, further. Maybe Netscape 4 code can give some clues because it does handle this correctly.
jst: fake a load group? The MozillaClassic code here worked because, as bht suggests, it reused the code paths used when loading a static document. In fact write looped data back into the layout engine via the old netlib stream mechanism. I don't think we have to go that far this time, but can we at least put all writes as well as loads in a big loadgroup? /be
Yup, we need something like that, or we might be able to reuse the loadgroup that already exists in the document (I think, unless document.open() clears it) and put a fake channel in the loadgroup and hook to document viewer (which normally does the onload fireing) so that it gets notified when all the channels in the loadgroup are done... I need to talk to Rick Potts about this...
Hi people, I need some help with this one. First of all the reporter was telling that the onload event did not work. But as you can see with my example, it does. And it still does with Build: 2000120504. And yes I'm using a javascript example generating the new document. So what's the point? It does work! Did you b.t.w. test my example? As you will see it works well. So now you jumping to frameset's. Hm. as I recall, I have developed at least 2 site's the last couple of weeks with working framesets on it. Using the document.write and onload event handler. So dynamically generated HTML/Framsets on it? So can you please clarify what the exact problem is? I would love to dig into this one! Friendly, HJ
I have just tested the attachment #20169 [details] and it works fine. Or I'm I missing some parts? I get a page, with a button on it, by pressing the button two frames are loading Netscape pages in it! I see the same with Netscape 4.76, so what the point? What else must I do? Friendly, HJ (Used version Build:2000120504 on WinNT4 SP6b)
HJ, when you click on the button in the last testcase you should get a new window with a frameset and once the two frames are loaded you should get an alert saying "FRAMESET onload fired", it works in NS4.x and IE but not in Mozilla nor Netscape 6.0. That's the problem.
Oh, my last comment isn't entirely clear, the problem is that the alert("FRAMESET onload fired") is not showing up in Mozilla/Netscape 6.0.
It seems I missed the train. And yes you are right, no alert at all! Sorry for the SPAM, but as I'm only human. Friendly, HJ.
No worries.
*** Bug 63267 has been marked as a duplicate of this bug. ***
Who took me off the CC list?
Is there any progress on this nested event stuff?
Attachement 21141 is placed in the wrong bug, sorry for that.
Milestone 0.7, Windows 95 <body onload="parent.check.document.location.href = 'chat.php?check=57'"> Does also not work.
Herbert, your example might be different, possibly worth another bug record or a duplicate. Would you mind attaching a simple standalone test case?
Herbert, please file that as a separate bug for now. Thanks!
Setting milestone
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.8
Set wrong milestone. Setting correct one.
Target Milestone: mozilla0.8 → mozilla0.9.1
*** Bug 35253 has been marked as a duplicate of this bug. ***
adding self to cc:
see http://www.webreference.com/tools/browser/javascript.html for possible workaround. in-page scripts work fine with window.onload=... but external scripts do not.
Keywords: dom0
Taking from Tom. This isn't really a bug in DOM events.
Assignee: joki → nisheeth
Status: ASSIGNED → NEW
Summary: Onload event fails in simple JavaScript document. → Onload doesn't fire in javascript-generated documents
See also bug 74230, "document.write() and <frameset> doesn't fire onload". That bug involves <frameset onload=""> (where the frameset itself is generated using document.write()) instead of <body onload="">.
Blocks: 71668
This should be fixed for nsbeta. Marking +.
Keywords: nsbeta1 → nsbeta1+
Whiteboard: [partial fix]
Whiteboard: [partial fix] → [partial fix] DIGBug
I created a slightly modified version of the last testcase and will attach it next. It tests a document.write of an image within a body and expects that the onload handler on the image is called before the onload handler on the body. I'm also about to attach a patch that fixes the above testcase as well as the three attached to this bug previously: http://bugzilla.mozilla.org/showattachment.cgi?attach_id=17765 http://bugzilla.mozilla.org/showattachment.cgi?attach_id=19706 http://bugzilla.mozilla.org/showattachment.cgi?attach_id=20169 Rick, would you please review the patch and answer some questions: a) Is mEODForCurrentDocument the only docshell state that needs to get reset during an out of band document.write? What else should we try and factor out of the CreateContentViewer() method that sets up docshell state? b) Is the place where I am resetting mEODForCurrentDocument the right place, or should I only reset it for the out of band document.write code path? c) Do I need to worry about resetting the DocumentViewerImpl::mLoaded variable? Right now it only gets checked in ::Stop() but it could be used in other places in the future... Thanks a lot!
Status: NEW → ASSIGNED
hey Nisheeth, I'm not sure if clearing mEODForCurrentDocument in STATE_START | STATE_IS_NETWORK is right or not :-( Currently, it is cleared in CreateContentViewer() which is called after data has started to arrive fom the channel... The closest OnStateChange equivalent is STATE_TRANSFERRING | STATE_IS_DOCUMENT. Unfortunately, I don't know if your dummy channel will cause a STATE_TRANSFERRING notification to fire :-( The STATE_TRANSFERRING is caused by a OnProgress(...) notification through the nsIProgressEventSink interface... I don't know if this will happen in your case or not :-( It may be safe to clear mEODForCurrentDocument in STATE_START | STATE_IS_NETWORK, but just make sure that it doesn't bust http://bugzilla.mozilla.org/show_bug.cgi?id=21358 This was the bug that caused the flag to be introduced in the first place :-) Because nsDocShell::CreateContentViewer(...) is not being called, a bunch of state initialization is not going to happen. Most notably, session history won't know about the load... I don't know if this is a bad thing or not :-) its just an observation... OnNewURI(...) is the other method that won't get called... There is some state in there that should probably be factored out... I think that the clearing of mInitialPageLoad to false should really be done in EndPageLoad(...) Hope these comments help -- rick
To address Rick's concerns: 1) Rick and I verified that bug 21358 does not recur with my changes. 2) Its ok for session history to not know about this load because we don't cache out of band document.written content in Mozilla yet. When we implement that (using a scheme equivalent to wysiwyg: urls in Netscape 4.x), we can ensure that the cached file gets entered into session history. 3) mInitialPageLoad is not used anywhere in the doc shell so I have removed it. Other stuff that I've done since last week: 1) I tested the out of band document.write of a META refresh tag along with a BODY tag with an onload handler. This caused a crash in the event listener manager (ELM) because the document changed out from underneath the ELM while the onload event was being processed on the earlier document. The fix was to ensure that a new document always gets a new ELM created for it. Thanks to Johnny for suggesting the fix and to Rick for suggesting the test case. 2) I tested document.writing into a frameset document to see whether the docshells get properly cleaned up. They do! Thanks to Rick for suggesting this test case. 3) I tested document.writing a META charset tag along with a BODY tag with an onload handler. Works fine! Thanks to Rick for suggesting this test case. 4) I tested an out of band document.write() without a subsequent document.close() call and noticed a slight discrepancy in behavior. The build with my changes does not stop the throbber for this case which, by the way, violates the open()/write()/close() usage guideline. IE 5.0 and the Mozilla tip build do stop the throbber. I've entered bug 81980 to track this problem but I'm not sure if it should hold up the checkin of this patch. Rick, jst, what do you think? I'm about to attach the updated patch for further scrutiny.
I forgot to thank Vidur earlier for suggesting that I test the case where the document.close() call is omitted. Thanks, Vidur!
Attached patch Updated patch — — Splinter Review
We have spent a lot of time fixing the throbber problem, so I feel we shouldn't regress. There are a couple of test sets you would need to go through in my opinion to be allowed to break it now. a) jrgm's page load test suite. Make sure that there are no pages in the suite that break (i.e. do not stop the throbber) because of this landing. b) ZDNet/CNN/or whoever also has a page load test suite (selmer? has the URL). We must make sure that we do not regress that test suite either. If we do not break any of the pages in test suites a) and b) then I could live with the regression. If you check this in and the regression appears, mark that bug with the regression keyword.
Tested jrgm's page loader test suite and didn't find any problems. Don't have the url to the zdnet performance test suite. Anybody? Adding a new patch with the following additions: 1) The URI on the dummy doc write request is set to the document URI. This is needed so that the doc loader and the document viewer's notion of the current document uri do not get out of sync. 2) Renamed the document viewer's BindToDocument() method to LoadStart() and moved initialization code from the doc viewer's constructors into a new method called PrepareToLoadDocument() that is called from the constructor and LoadStart(). Now, we have a clearer link between the pair of methods that get called during a document load: LoadStart() at the start of the doc load, LoadComplete() when the doc load finishes.
In the previous comment, substitute PrepareToLoadDocument() with PrepareToStartLoad().
Attached patch One more time... — — Splinter Review
Tested the case of document.writing a script that is externally loaded. Works fine. Attaching testcase...
Attached file test.js —
Whiteboard: [partial fix] DIGBug → [fix in hand, needs review] DIGBug
r=rpotts looks good.
a= asa@mozilla.org (still awaiting sr=)
I had a look at the patch, and here's what I found: do_QueryInterface() is null safe so there's no need to check if (cv) before calling do_QueryInterface(cv) so the code: + docshell->GetContentViewer(getter_AddRefs(cv)); + if (cv) { + nsCOMPtr<nsIDocumentViewer> docViewer = do_QueryInterface(cv); + if (docViewer) { is perfectly fine w/o the if (cv) check. Also, just below that code: + nsCOMPtr<nsISupports> doc = do_QueryInterface((nsIHTMLDocument*)this); + if (doc) { + docViewer->LoadStart(doc); + } Why not simply: + docViewer->LoadStart(NS_STATIC_CAST(nsIHTMLDocument *, this)); This would eliminate one QI call. In nsHTMLDocument::AddDocWriteDummyRequest(void): + nsCOMPtr<nsIURI> documentURI(do_QueryInterface(mDocumentURL)); + channel->SetOriginalURI(documentURI); There's no need to QI an nsIURL to an nsIURI, nsIURL inherits nsIURI so passing mDocumentURL to channel->SetOriginalURI() directly is all you need to do here. Looks like the nsGlobalWindow.cpp changes are already checked in. Nit pick of the day: In DocumentViewerImpl::PrepareToStartLoad(): - mIsPrinting = PR_FALSE; - + mIsPrinting = PR_FALSE;.. Extra spaces after PR_FALSE (marked with ..) I think you should eliminate the extra QI calls and I also encourage you to make the other changes mentioned above before checking in, but I won't hold my sr because of those. sr=jst
Thanks for the sr, Johnny. I've made the changes you suggested and checked in. About to attach the final patch.
Attached patch Final Patch — — Splinter Review
Marking fixed.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Verified with 2001-06-05-11.
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: