GenAIChild spams the parent process with `HideShortcuts` messages and should manage event listeners more efficiently
Categories
(Core :: Machine Learning, defect)
Tracking
()
People
(Reporter: Gijs, Unassigned)
Details
(Keywords: perf)
Attachments
(1 file)
167.20 KB,
image/png
|
Details |
In an unrelated profile in a security bug (from Firefox 134, as well, and the code has changed), I noticed that the profiler marker thread had lots of GenAI:HideShortcuts
messages in a very short space of time. This was surprising because the screenshots did not show the sidebar or any interaction with the chatbot or the shortcuts. In just over 1 minute, 280 such messages were received. There were 0 ShowShortcuts
markers.
This led me to look at the code, and I was surprised how many events it's listening to. In the actor registration, it listens for:
- DOMContentLoaded
- mousemove
- resize
- scroll
so any of these events will cause the actor to be created. Because it also has allFrames
, this means the actor is eagerly created whenever any web frame loads content (including any/all subframes, ads, tracker frames, etc., regardless of whether the user interacts with them).
Then in the actor creation handling, it adds more event listeners, namely mousedown
, mouseup
and pagehide
as well as selectionchange
.
The event handling code in the actor ignores DOMContentLoaded
and mousemove
(so they, AFAICT, are just used to create the actor and will also activate the handleEvent
method but will not actually do anything). mousedown
and mouseup
will be handled and, potentially, at mouseup
, will cause us to start showing shortcuts. mousedown
, scroll
, resize
, pagehide
and selectionchange
all trigger hiding the popup again.
My suggestion to make this perform better would be:
- remove mousemove/DOMContentLoaded from the list as they seem unnecessary.
- don't register any of these events as necessitating creating the actor except
mouseup
. This avoids creating the actor in ad subframes etc. etc., unless/until the user actually interacts with it. - use event listeners in the parent to trap
mousedown
(as we don't care where the mousedown happens so might as well stick the listener on thebrowser
element). - only add the scroll/resize/selectionchange/pagehide listeners after sending the "show shortcuts" message, and remove the listeners as soon as we've told the parent process to hide the shortcut (as both scroll/resize can fire many times in a short span of time so sending a message each time is not cheap)
Also, as a minor thing, the code seems to use new Date()
to do timestamp computation for when events happen - event.timeStamp
would be better suited to this (avoids impact of other listeners etc.) and should be faster. :-)
Reporter | ||
Comment 1•8 days ago
|
||
(In reply to :Gijs (he/him) from comment #0)
- use event listeners in the parent to trap
mousedown
(as we don't care where the mousedown happens so might as well stick the listener on thebrowser
element).
I guess the child still needs this in order to check if the selection has changed since the mousedown when the mouseup fires. The reason I suggested the parent is that I thought it was only used for the timestamp and to provide the "the user clicked outside the popup so it should close" functionality - which we could do in the parent.
Description
•