Closed Bug 1693877 Opened 4 years ago Closed 4 years ago

Closing tabs trigger imports of FinderParent.jsm

Categories

(Toolkit :: Find Toolbar, defect)

defect

Tracking

()

RESOLVED FIXED
88 Branch
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 of this.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 empty destroy method in FinderParent. I think that would be OK, as findbar.js already accesses browser._lastSearchString.

(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 of this.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 empty destroy method in FinderParent. I think that would be OK, as findbar.js already accesses browser._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: nobody → florian
Status: NEW → ASSIGNED

(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.

Whiteboard: [fxperf] → [fxperf:p2]
Pushed by fqueze@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/dde53a179abe Closing tabs should not trigger the finder lazy getter, r=mikedeboer.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: