68074 - Lots of CreateInstances of nsEventListenerManagers at startup

Status: NEW

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

Description

[Simon Fraser [no longer active]]

Some analaysis of an XPCOM log for startup and display of a simple HTML page in a 
Mozilla browser window shows that we create 1054 instances of 
nsEventListenerManagers. This seems a lot, and perhaps could be optimized.

Comment 1

[Simon Fraser [no longer active]]

Most of these nsEventListenerManagers seem to be created by nsXULElement's call 
to element->AddScriptEventListener(). At 80 bytes a pop, this is quite a space 
hog too.

Comment 2

[Brendan Eich [:brendan]]

Is our XUL using a bunch of onclick attributes, or some such?  What's further up
the stack in these calls to AddScriptEventListener?

/be

Comment 3

[Chris Waterson]

Or worse, a XUL template with event handlers inside the <template>? 
alecf/ben/mcafee, you should do a pass through navigator.xul and its overlays. 
(sspitzer, same for mail.) Search for the `datasources' attribute; make sure 
that there are no event handlers contained inside a <template>.

Comment 4

[(not reading, please use [e-mail] instead)]

found a bunch of badness in the mailnews xul.

mailWindowOverlay.xul
    oncommand="MsgMoveMessage(event.target)"
    oncommand="MsgCopyMessage(event.target)"
    oncommand="MsgGetMessagesForAccount(event)"

messengercompose.xul 
    oncommand="MessageFcc(event.target)"

msgFolderPickerOverlay.xul
    oncommand="PickedMsgFolder(event.target,'msgNewFolderPicker')"
    etc.  (basically, every folder picker that I wrote is evil)

FilterListDialog.xul
    onclick="onToggleEnabled(event);"

pref-composing_messages.xul
    oncreate="dump('CREATE MENU NOW\n');"

I'll go start a new bug to track the clean up of the mailnews xul.
thanks for the heads up waterson / sfraser.

Comment 5

[(not reading, please use [e-mail] instead)]

the mail xul is covered by #68174

Comment 6

[Chris McAfee]

perf

Comment 7

[Chris McAfee]

Here's my list:
navigator.xul
 
oncommand="OpenBookmarkURL(event.target,document.getElementById('BookmarksMenu').database)"
/>
  oncommand="OpenBookmarkURL(event.target,
document.getElementById('innermostBox').database);"/>
oncommand="OpenBookmarkURL(event.target,
document.getElementById('innermostBox').database)"/>

navigatorOverlay.xul
  oncommand="selectLocale(event)" 
  oncommand="applyTheme(event.target)"
  oncommand="OpenBookmarkURL(event.target,
document.getElementById('BookmarksMenu').database)"

internetresults.xul
  oncommand="doEngineClick(event, this);" 

search-editor.xul
  oncommand="return chooseCategory(this);"
  oncommand="chooseCategory(this);" 
  oncommand="saveEngines();"

sidebarOverlays.xul
  oncommand="SidebarTogglePanel(event.target);"

charsetOverlay.xul
  oncommand="SelectDetectors( event )"

taskbarOverlay.xul
  oncommand="rdf:http://home.netscape.com/NC-rdf#content"

tasksOverlay.xul
  oncommand="ShowWindowFromResource(event.target)"

Comment 8

[Alec Flett]

ok here's what needs to be done:
1) you need to find the most logical container for the template - i.e. the
<tree> or <menulist> - and move the onclick or oncommand handler there
2) look at the event handler code, and figure out if it was dependent on the
command handler being in the child node.. look for references to event.target,
and more importantly, test it out. 9 times out of 10, it's not dependent and you
can just move the event handler.

Comment 9

[saari (gone)]

mcafee, I'm giving this bug to you since you've been working on this 

Comment 10

[Simon Fraser [no longer active]]

slee: what I mumbled in the meeting was to replace the CreateInstance of the 
nsEventListenerManager with a call to NS_NewEventListenerManager if you can (i.e. 
if the things are being created in the content DLL. This avoids a trip through 
XPCOM.

Comment 11

[Chris McAfee]

Attachment: patch moving some oncommand handlers outside of <template>

Comment 12

[Chris McAfee]

I went through the browser files and found some cases
where we could move oncommand stuff outside of templates.
I tested each one of these by first breaking it (removing
the command handler), then adding it back into the new
place to restore the functionality.

Some of the JS functions were used in other JS, and sometimes
it looked like the JS needed the handler in the template code.
I managed to get changes for navigator.xul, navigatorOverlay.xul,
and sidebarOverlays.xul. Files I had trouble with:

internetresults.xul : ? I couldn't understand how to test this
search-editor.xul:    Other JS usages of chooseCategory(), more changes needed.
taskbarOverlay.xul:   Didn't work
tasksOverlay.xul:     ShowWindowFromResource() not working on linux, couldn't test.

Comment 13

[Chris McAfee]

Attachment: 2nd patch, resolves cvs conflicts [Checkin: Comment 25]

Comment 14

[Alec Flett]

these changes look great - you don't need to add the comment about the handlers
moving to the template - they shouldn't have been there in the first place, and
everywhere else they're outside the template already
sr=alecf on the latest patch if you remove those comments

Comment 15

[Chris McAfee]

ok thanks alec

Comment 16

[Chris McAfee]

2nd patch checked in.

Comment 17

[Paul Chen]

nav triage team:

Chris, are there still any remaining issues? Why is this bug still open?

Comment 18

[Chris McAfee]

This is open per smfr's comment about avoiding xpcom/dll trip

Comment 19

[Paul Chen]

nav triage team:

Marking nsbeta1+, mozilla0.9.2, reassigning to pchen

Comment 20

[Paul Chen]

nav triage team:

Marking p3 and mozilla0.9.3

Comment 21

[Madhur Bhatia]

QA contact updated

Comment 22

[Paul Chen]

nav triage team:

Avoiding trip through xpcom not that critical. Marking nsbeta1- and future

Comment 23

[Asa Dotzler [:asa]]

->default assignee

Comment 24

[Pham]

.

Comment 25

[Serge Gautherie (:sgautherie)]

Comment on attachment 27430 [details] [diff] [review]
2nd patch, resolves cvs conflicts
[Checkin: Comment 25]

(In reply to comment #16)
> 2nd patch checked in.

A (code and comment) modified version was checked in:
[
2006-09-13 22:46	mcafee%netscape.com 	mozilla/suite/browser/navigator.xul 	1.278
2006-09-13 22:46	mcafee%netscape.com 	mozilla/suite/browser/navigatorOverlay.xul 	1.179
2006-07-27 07:47	mcafee%netscape.com 	mozilla/suite/common/sidebar/sidebarOverlay.xul 	1.56
]

NB: The (2006) timestamps are unexpected ... There seems to be something wrong with Bonsaï (for this, and other, checkins) :-<

Comment 26

[Serge Gautherie (:sgautherie)]

blocking1.9=?:
not mandatory, but as it is marked as 'perf' and seems easy enough to do...

Comment 27

[Mike Schroepfer]

(In reply to comment #26)
> blocking1.9=?:
> not mandatory, but as it is marked as 'perf' and seems easy enough to do...
> 

What is left to do here?   Likely not going to block but would *happily* take a patch.

Comment 28

[Serge Gautherie (:sgautherie)]

(In reply to comment #27)
> What is left to do here?

I don't know precisely yet:
see comment 12 and bug 68174 comment 19...

Comment 29

[Mike Schroepfer]

When we know what we want you can re-nom but not clear to me we should block on this

Comment 30

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

Currently when starting FF, ~320 ELMs are created.

This seems to be in wrong component. Firefox-generic might be a better one, if
the count of event listener attributes in UI should be reduced?