onZoomChange delazifies all browsers at once

RESOLVED FIXED in Firefox 56

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

({perf})

Trunk
Firefox 57
Points:
---

Firefox Tracking Flags

(firefox55 wontfix, firefox56 fixed, firefox57 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
from mozillazine:

> [bug 1345098] Lazy browser prematurely inserted via 'fullZoom' property access:
> getter@chrome://browser/content/tabbrowser.xml:2293:45
> ZoomManager_getZoomForBrowser@chrome://global/content/viewZoomOverlay.js:45:1
> getZoomLevel@chrome://browser/content/ext-tabs.js:687:20
> getAPI/self.tabs.onZoomChange<@chrome://browser/content/ext-tabs.js:698:39
> addListener@resource://gre/modules/ExtensionCommon.jsm:1441:42
> addListener@resource://gre/modules/ExtensionCommon.jsm:1479:50
> addListener@resource://gre/modules/ExtensionParent.jsm:760:35
> async*receiveMessage@resource://gre/modules/ExtensionParent.jsm:606:11
> tabbrowser.xml:2293:23
> 
> Several thousands of errors like that

http://searchfox.org/mozilla-central/rev/39ccebaf1890f8731d534b5eb3818b66d2b25221/browser/components/extensions/ext-tabs.js#694-700
(Assignee)

Comment 1

2 years ago
When accessing fullZoom on a lazy browser, the browser gets inserted and then fullZoom always returns 1 regardless because the browser doesn't have anything loaded yet. So we might as well add a shim returning 1 without inserting the browser.
Component: WebExtensions: General → Tabbed Browser
Product: Toolkit → Firefox
(Assignee)

Comment 2

2 years ago
Posted patch patchSplinter Review
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Attachment #8895037 - Flags: review?(kevinhowjones)

Comment 3

2 years ago
Comment on attachment 8895037 [details] [diff] [review]
patch

Review of attachment 8895037 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.  Interesting syntax, thanks for cleaning everything up.
Attachment #8895037 - Flags: review?(kevinhowjones) → review+

Comment 4

2 years ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bbc4ded4bae
Add lazy browser shims for fullZoom and textZoom. r=kevinjones

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9bbc4ded4bae
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
(Assignee)

Comment 6

2 years ago
Approval Request Comment
[Feature/Bug causing the regression]: bug 906076
[User impact if declined]: bad performance with the onZoomChange webextension API
[Is this code covered by automated tests?]: not sure if there are webextension tests for this
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: /
[Is the change risky?]: hardly
[Why is the change risky/not risky?]: small, isolated and straightforward fix
[String changes made/needed]: /
Attachment #8895711 - Flags: approval-mozilla-beta?
Comment on attachment 8895711 [details] [diff] [review]
patch for uplift

Fix for a perf issue with web extensions, let's uplift for beta 3.
Attachment #8895711 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 9

2 years ago
Someone please add "perf" key word. Thanks.
You need to log in before you can comment on or make changes to this bug.