51211 - "load" event listeners in chrome may not fire as expected

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Tim Hill]

In navigator.js, event listeners are added to respond to page load events:
contentArea.addEventListener("load", UpdateBookmarksLastVisitedDate, true);
contentArea.addEventListener("load", UpdateInternetSearchResults, true);
contentArea.addEventListener("load", checkForDirectoryListing, true);
contentArea.addEventListener("load", getContentAreaFrameCount, true);
contentArea.addEventListener("load", postURLToNativeWidget, true);

However, these events may not always fire when desired.  
- Bug 45296 is about how they do not fire if the page is aborted by the user.  
Adding code to the same place the throbber is turned off would account for the 
case of aborting the page load.
- Once Bug 22710 is implemented, images will also fire load events which I 
assume will bubble up to the chrome, which wouldn't be correct.
- Do all these functions need to be fired for each frame's "load" event?

Comment 1

[Tim Hill]

With Bug 22710 fixed, these handlers are indeed firing after every image load.  
I don't know if there's a huge impact on performance, but accessing the 
bookmark service every time an image loads can't make things faster.

Comment 2

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

I have a fix in my tree which stops load events from bubbling (or actually stops 
calling the event handlers) if it comes from image load.

Comment 3

[Bill Law]

*** Bug 53470 has been marked as a duplicate of this bug. ***

Comment 4

[Bill Law]

This might have performance implications.  See bug 53470 (which I resolved as a 
dup of this one).  There might actually be much more expensive code (than 
checkForDirectoryListing) that gets called here (e.g., postURLToNativeWidget).

Heikki, can you take this bug?  Or is your patch to somebody else's code (in 
which case, please tell us whose so we can reassign to them).  If you could, 
attach your patch, also, please.

I'm nominating this nsbeta3 (since 53470 was).  Also, marking [NEED INFO] 
pending input from Heikki.

Comment 5

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

Image load events no longer bubble (see bug 52526). Or actually, the event does 
bubble but we do not trigger the onload handlers except for the image element. 
This is what all events do, as far as I can see. joki explained that XUL (at 
least at some point in history) required everything to bubble even though the 
specs say otherwise. This implies that XUL can respond to events differently 
than normal content.

Also there is at least one filter that stops load events (and some other events) 
from really bubbling to chrome handlers. That filter has been in place for quite 
some time. I noticed that I had to add a filter for NS_IMAGE_LOAD event because 
otherwise the app was just continuously reloading the user's home page. This 
filter is in GlobalWindowImpl::HandleDOMEvent():

http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsGlobalWindow.cpp#509

Comment 6

[Adam Lock]

*** Bug 53587 has been marked as a duplicate of this bug. ***

Comment 7

[Adam Lock]

It strikes me as very inefficient to register 5 javascript handlers for the same 
load event. That means five entries, five JS stubs, five marshalled calls. 
Shouldn't navigator.js just register one handler and call these 5 methods from 
there? 

Comment 8

[Simon Fraser [no longer active]]

Is this really an XPapps problem? Trying a new component...

Comment 9

[don]

P1 nsbeta3+ bug #53587 was marked as a duplicate of this by Adam Lock, so I'm
marking this P1 and nsbeta3+ also.

Comment 10

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

I'' take a stab at this... Adding joki to CC just in case...

First I will see if the loads are caused by image loads. 

Does anyone have data on the performance issues Adam pointed out? If not, I 
could also look at that a bit. 

We are not firing abort events at all, and do not fire error events for page 
load (I think). The possibility of abuse with abort event is so great that we 
want to be able to turn it off from the prefs -> Future. Error is a bit more 
important, but it is quite problematic with documents. Anyway, if there is some 
code registered for document load and the assumption is that it will always be 
called for every document being loaded is flawed.

Comment 11

[clayton]

PDT:  Consequences for UI aren't clear.  Bad behavior, need more info.
Downgrading to P2 unless this looks like a critical blocker.

Comment 12

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

Attachment: Patch, filter image load event in capturing stage (global window)

Comment 13

[Tim Hill]

What about the load event for frames?  I suggest that all the load handlers be 
replaced with a single handler which calls them.  Then you can just add:
if (e.target == window._content.document)
to filter out the load events.  This will filter out the individual frame loads 
as well.

Comment 14

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

Seems like adding a filter to the the capturing stage in global window will fix 
abcnews.com giving multiple undefined messages in console. This also fixes the 
search sidebar.

The bubbling stage already had a filter for this, in fact it is filtering a lot 
more events. It seems strange that there is no filter for capturing stage...!

I see this as a low risk fix. Image load sending load events is a relatively new 
feature, and I do not think the chrome was ready for it. Also, I do not believe 
the chrome to be depending on image load events.

joki, can you review?

Comment 15

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

Attachment: Patch to navigator.js

Comment 16

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

The second patch is to navigator.js. It moves all the load event handlers into a 
new function which is now the only registered event listener. I do not have the 
stats yet how much this saves in time.

I also added the filter Tim Hill suggested. It does indeed filter out the frame 
load events (I presume) as well. For example, I put a counter to see how many 
events were filtered and on http://developer.netscape.com we filter two events. 
However, I do not know if this is safe. Anyone know if the functions in 
question would in fact be interested in frame loads as well?

Comment 17

[Bill Law]

Might you have subtly changed the behavior if one of the previous handlers does
something like generate a JS exception?  Previously that would ripple back to
the event listener calling code which would (theoretically?) go ahead and call
the next listener.  Now, that would ripple back for the one listener call and
other pseudo listeners wouldn't get a crack at the load event.

That probably doesn't matter, but perhaps a try/catch around each handler call
would be safer?

Also, can you go ahead and pass the event into the 3 listeners that don't use
it?  That would match the semantics of the call currently being made to the load
listeners, right?

Comment 18

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

Hmm, didn't think about exceptions... That's what you get for coding too long in 
our subset of C++ features to use... And XUL isn't my area, really. 

Well, it seems unlikely that any of the methods would throw anything except 
under extreme situations (like out of memory condition). These methods have 
internal try/catch code to catch normal cases. It would be nice get some data on 
the speed gain before going forward with this... My Quantify refuses to 
cooperate, though :(

I don't see the benefit of passing the event into the methods that don't use it. 
It is a minor optimization, but still. And does JavaScript running in strict 
mode then give warnings about unused parameters?

Comment 19

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

This is just speculation, but it seems likely that adding try/catch around each 
call would emulate the current model even in low memory condition because we do 
a pretty good job in forgetting to check return values from functions. I cannot 
be totally sure about this, of course. But then again, how expensive is the 
try/catch code, would we lose the speed gain in the extra try/catch code?

Comment 20

[Jim Roskind]

Adding PDTP2 comment per clayton's verdict above.  Removing need info.

Comment 21

[Claudius Gayle]

clearing PDTP2 comment for reconsideration. I changed the priority to P1 as well (don't think
I was supposed to do that but dammit, it felt right :-).

Anyway PDT, you can look at the dupe Don cited for just ONE manifestation of this bug.
Analysis had led to this bug as the root cause of that problem and that problem is hella
annoying for the end user. The search sidebar results continue to refresh incessantly, a process
that can for instance last several minutes on my machine - long after I've stopped caring.

Comment 22

[Nisheeth Ranjan]

Nominating for rtm and marking rtm+ to make sure that this gets fixed for rtm

Comment 23

[Michael La Guardia]

Marking nsbeta3- as we should not hold PR3 for this.

Comment 24

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

Attachment: navigator.js: Call the load listeners in BrowserStop()

Comment 25

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

My latest patch adds calls to the load listeners into BrowserStop() as well. I 
am not 100% sure this is what should happen, though. There is also one case 
where we call appCore.stop() but I am wondering if it should all BrowserStop() 
instead (see onLoadViaOpenDialog()). I removed the event argument from all the 
load listeners because they were not using it.

If this bug gets okayed by the higher forces I would like to check in the fix to 
global window and then let the XUL gurus decide the rest of the issues. 

Comment 26

[Phil Peterson]

PDT marking [rtm need info]. It's really not clear what this bug is about anymore.

Comment 27

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

This should really be separate bugs, don't know why the other bugs were duped to 
this. But since we have all of it here, he is my summary:

1. Image load event bubbling into chrome causes performance problems, and causes 
the search sidebar to reload every time an image is loaded (claudius reported 
the search sidebar reloading for several minutes on his machine). See 
duplicates bug 53470 and bug 53587. This can be fixed by first patch. See 
comments from me on 2000-09-22 16:42. I am proposing I should check this fix in, 
and then hand this bug to someone in charge of navigator.js. This is the most 
important part of this bug in my opinion, and with lowest risk.

2. Some load event listeners are not called if the document load stops because 
of an error or the user stops it. This is related to bug 45296. This can be 
fixed by third patch. See comments from me on 2000-09-27 11:53. I do not know 
of the risks involved in this.

3. We could gain some performance boost by registering only one load event 
listener which would then call the real load event listeners. According to 
vidur's gut feeling the gain would be insignificant, vidur thinks that 10 or so 
event listeners would benefit from moving them into single event listener. But 
there is not hard data on this! This has been addressed in the second patch. See 
comments from me and law between 2000-09-22 18:04 and 2000-09-22 19:31. I see 
some additional risk in this change.

4. It has been identified that also frame load events travel to the chrome, and 
this is causing slight performance problems as well as search sidebar reloads. 
This has also been addressed in the second patch. I do not know if something is 
depending on the frame load events so I would consider this a high risk change 
unless proven otherwise. I think the gain is minimal.

Comment 28

[vidur (gone)]

r=vidur for Heikki's first patch. It's a low risk way of dealing with the
incorrect delivery of image onload events to chrome in the capturing phase. The
right short-term fix, but in the long term, we should figure out a more generic
way to target events that should be sandboxed from chrome.

Comment 29

[Claudius Gayle]

*** Bug 54277 has been marked as a duplicate of this bug. ***

Comment 30

[Nisheeth Ranjan]

Marking [rtm+], r=nisheeth for the first patch.

PDT: Please see Heikki's comment timestamped "2000-10-02 18:44".  Requesting
rtm++ for the first patch that fixes the search sidebar reloading problem.

Comment 31

[David Hyatt]

First patch is ok for the capturing case.  a=hyatt

Comment 32

[Phil Peterson]

PDT marking [rtm++] *ONLY* for the first patch to check for NS_IMAGE_LOAD.
Please break out other issues into separate bugs.

Comment 33

[Robert John Churchill]

*** Bug 52884 has been marked as a duplicate of this bug. ***

Comment 34

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

First patch checked in to trunk and Netscape_20000922_BRANCH. Marking fixed.

I will create new bugs for the other identified items, and add note of them in here.

Comment 35

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

I opened new bugs for cases 2-4: bug 55410, bug 55413 and bug 55414.

Comment 36

[Chris Petersen]

Marking verified in the Oct 6th build.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Now that bug 55413 and company are fixed, we actually properly check the
target... So I will likely be removing the code checked in to "fix" this bug, as
well as the whole NS_IMAGE_LOAD event type....