Closing tabs trigger imports of FinderParent.jsm
Categories
(Toolkit :: Find Toolbar, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox88 | --- | fixed |
People
(Reporter: florian, Assigned: florian)
Details
(Keywords: perf, Whiteboard: [fxperf:p2])
Attachments
(1 file)
I started the profiler before closing about 300 tabs. In the profile I was surprised to see that we import FinderParent.jsm many times: https://share.firefox.dev/3k5Dm8f
It looks like this code in findbar.js:
if (this.browser && this.browser.finder) {
this.browser.finder.destroy();
}
Triggers the lazy finder getter that imports FinderParent.jsm, to call its destroy
no-op method:
FinderParent.prototype = {
// Called by findbar.js
destroy() {},
Possible ways to avoid this:
- in findbar.js, only run this code for non-remote browsers. This would still be wasteful, but would avoid this silly behavior for most tabs.
- if we are OK with calling a 'private' field, checking for
this.browser._finder
instead ofthis.browser.finder
would let us know if a non-remote finder that needs to be destroyed has ever been initialized for the current browser, and would let us remove the emptydestroy
method inFinderParent
. I think that would be OK, as findbar.js already accessesbrowser._lastSearchString
.
Comment 1•4 years ago
|
||
(In reply to Florian Quèze [:florian] from comment #0)
- if we are OK with calling a 'private' field, checking for
this.browser._finder
instead ofthis.browser.finder
would let us know if a non-remote finder that needs to be destroyed has ever been initialized for the current browser, and would let us remove the emptydestroy
method inFinderParent
. I think that would be OK, as findbar.js already accessesbrowser._lastSearchString
.
I like this solution best and is in line with patterns I've seen across the code base.
The perhaps silly code is I think a result of mirroring code paths resulting from Neil's work on making the findbar Fission compatible.
Also a common pattern, I believe.
Assignee | ||
Comment 2•4 years ago
|
||
Updated•4 years ago
|
Assignee | ||
Comment 3•4 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #1)
I like this solution best and is in line with patterns I've seen across the code base.
Thanks! I just put up for review a patch doing just this. Any specific tests that I should run on try to ensure I'm not breaking things?
Also a common pattern, I believe.
Do you mean uninit code triggering lazy getters and starting initialization work that really isn't intended to happen at that point? It's not the first time I see this. I wish there would be a good way to write automated tests to catch that case, but I can't think of any easy way.
Updated•4 years ago
|
Comment 5•4 years ago
|
||
bugherder |
Description
•