Closed Bug 57636 Opened 24 years ago Closed 23 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: 23 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: