Closed Bug 1442694 Opened 7 years ago Closed 4 months ago

Pinned tabs populating tab bar create visual noise on startup

Categories

(Firefox :: Session Restore, defect, P2)

Product:
Firefox
Component:
Session Restore
Type:
defect

Tracking

()

Status:
RESOLVED INACTIVE
Project Flags:
Performance Impact medium

People

(Reporter: alexical, Unassigned)

References

Details

(Keywords: perf:frontend, perf:startup)

Attachments

(5 files)

Bug 1442694 - Preopen pinned tabs before session restore
59 bytes, text/x-review-board-request
Details
withchange.mp4
1.56 MB, video/mp4
Details
Bug 1442694 - Preopen pinned tabs before session restore r?Gijs
47 bytes, text/x-phabricator-request
Details | Review
Bug 1442694 - Add tests for preopened pinned tabs r?Gijs
47 bytes, text/x-phabricator-request
Details | Review
Bug 1442694 - Fix failures due to removing selected tab r?Gijs
47 bytes, text/x-phabricator-request
Details | Review
STR: - Have pinned tabs - Start the browser - Notice the pinned tabs come in after first paint, creating visual noise I understand we want to get the first paint out as soon as possible, and knowing what pinned tabs to load before first paint might require more work than we'd like, but we should at least be able to store the number of pinned tabs somewhere quickly accessible and simply display a placeholder loading icon.
Component: Tabbed Browser → Session Restore
Looking into this to see how difficult it would be.
Assignee: nobody → dothayer
Status: NEW → ASSIGNED
Whiteboard: [fxperf:p1]
Comment on attachment 8960382 [details] Bug 1442694 - Preopen pinned tabs before session restore https://reviewboard.mozilla.org/r/229166/#review235026 This doesn't seem to help in my testing, likely because gBrowser is initialized after the first pait.
Attachment #8960382 - Flags: review?(dao+bmo) → review-
That's strange - on my machine the dummy pinned tabs are present in the first paint with this change, so I'm not actually sure what's going on: are these changes just racing with the actual paint?
Attached video withchange.mp4 —
Attaching a video of startup with the change applied for me. There's one frame of what appears to be random GPU memory, then a blank paint, then the window with the pinned tabs visible.
Oh I see - this patch was bitrotted by your changes in bug 1443849. Looking into whether this approach is salvageable given those changes.
Comment on attachment 8960382 [details] Bug 1442694 - Preopen pinned tabs before session restore https://reviewboard.mozilla.org/r/229166/#review235546 ::: toolkit/content/widgets/tabbox.xml:265 (Diff revision 2) > else > this.selectedIndex = 0; > ]]> > </constructor> > > + <method name="_preopenPinnedTabs"> This doesn't belong in tabbox.xml at all, and generally I don't think we want code to manually add tabs without going through gBrowser.addTab. What I think you really want is initialize gBrowser before the first paint.
Attachment #8960382 - Flags: review?(dao+bmo) → review-
Comment on attachment 8960382 [details] Bug 1442694 - Preopen pinned tabs before session restore https://reviewboard.mozilla.org/r/229166/#review235546 > This doesn't belong in tabbox.xml at all, and generally I don't think we want code to manually add tabs without going through gBrowser.addTab. What I think you really want is initialize gBrowser before the first paint. All right. I was assuming its initialization had been places after first paint for a concrete performance win, and didn't want to mess with that. To avoid wasting your time with another review, do you have any suggestions for where to initialize it? Maybe gBrowserInit.onBeforeInitialXULLayout()?
(In reply to Doug Thayer [:dthayer] from comment #10) > Comment on attachment 8960382 [details] > Bug 1442694 - Preopen pinned tabs before session restore > > https://reviewboard.mozilla.org/r/229166/#review235546 > > > This doesn't belong in tabbox.xml at all, and generally I don't think we want code to manually add tabs without going through gBrowser.addTab. What I think you really want is initialize gBrowser before the first paint. > > All right. I was assuming its initialization had been places after first > paint for a concrete performance win, and didn't want to mess with that. As a rule of thumb I think we want to initialize things both as early as needed (so they're ready when we want that) and as late as possible (so they don't delay other things that are needed before). > To > avoid wasting your time with another review, do you have any suggestions for > where to initialize it? Maybe gBrowserInit.onBeforeInitialXULLayout()? Yeah.
I'm a little bit out of my depth here. When I move the initial open of the pinned tabs to onBeforeInitialXULLayout, the tab's XBL properties are not initialized after this.addTab(...). I can hack around this by setting something like tab._initializer = () => {...} on the tab and checking it on the tabbrowser-tab constructor, but that hardly seems elegant, and I'd like to understand why this is manifesting first.
So using XBL in onBeforeInitialXULLayout won't work because XBL being ready depends on the initial layout. Instead gBrowserInit.onDOMContentLoaded is probably the right place to do this.
(In reply to Dão Gottwald [::dao] from comment #13) > So using XBL in onBeforeInitialXULLayout won't work because XBL being ready > depends on the initial layout. Instead gBrowserInit.onDOMContentLoaded is > probably the right place to do this. That was where this was occurring when you initially rejected it because it wasn't fixing the issue. That being said, it seems to be working in onDOMContentLoaded now. Is there any reason why this was happening after first paint before but after first paint now?
I'm not sure if there are meaningful guarantees about DOMContentLoaded and the first paint. I would normally expect DOMContentLoaded to fire first, but it seems that there are ways to trigger a paint before that. Maybe this doesn't happen anymore, which would be good, but I definitely saw it when I tried your first patch. You could also try requestAnimationFrame in onBeforeInitialXULLayout -- that should occur before the first paint, I think, but then you'd need to make sure it doesn't race with gBrowser.init in onDOMContentLoaded.
Usually onDOMContentLoaded is the last point at which we can do things before first paint in the browser window.
How are you ensuring that browser.tabs.pinnedTabCount isn't the number of pinned tabs of some random window, but of the window that sessionstore is going to restore as the first one? This isn't really obvious to me from looking at the patch.
Flags: needinfo?(dothayer)
(In reply to Dão Gottwald [::dao] from comment #18) > How are you ensuring that browser.tabs.pinnedTabCount isn't the number of > pinned tabs of some random window, but of the window that sessionstore is > going to restore as the first one? This isn't really obvious to me from > looking at the patch. These lines: let pinnedTabCount = Services.prefs.getIntPref("browser.tabs.pinnedTabCount", 0); let initialWindow = root.windows.find(w => { return w.tabs.filter(t => t.pinned).length == pinnedTabCount; It catches most cases, but now that you mention it the patch could do more to ensure that this correctly finds a window. A case that breaks this would be Window A has one pinned tab Window B has one pinned tab Unpin tab in Window A (updates the pref's value to be 0) Close all windows Start FF, it will look for a window with 0 pinned tabs, which won't exist if it's not an automatic session restore I suppose on window close we could update the pref if that window's gBrowser._numPinnedTabs is nonzero?
Flags: needinfo?(dothayer)
(In reply to Doug Thayer [:dthayer] from comment #19) > (In reply to Dão Gottwald [::dao] from comment #18) > > How are you ensuring that browser.tabs.pinnedTabCount isn't the number of > > pinned tabs of some random window, but of the window that sessionstore is > > going to restore as the first one? This isn't really obvious to me from > > looking at the patch. > > These lines: > > let pinnedTabCount = > Services.prefs.getIntPref("browser.tabs.pinnedTabCount", 0); > let initialWindow = root.windows.find(w => { > return w.tabs.filter(t => t.pinned).length == pinnedTabCount; That's backwards. browser.tabs.pinnedTabCount shouldn't determine what window sessionstore restores first. > I suppose on window close we could update the pref if that window's > gBrowser._numPinnedTabs is nonzero? Windows don't close in some particular order when quitting.
(In reply to Dão Gottwald [::dao] from comment #20) > (In reply to Doug Thayer [:dthayer] from comment #19) > > (In reply to Dão Gottwald [::dao] from comment #18) > > > How are you ensuring that browser.tabs.pinnedTabCount isn't the number of > > > pinned tabs of some random window, but of the window that sessionstore is > > > going to restore as the first one? This isn't really obvious to me from > > > looking at the patch. > > > > These lines: > > > > let pinnedTabCount = > > Services.prefs.getIntPref("browser.tabs.pinnedTabCount", 0); > > let initialWindow = root.windows.find(w => { > > return w.tabs.filter(t => t.pinned).length == pinnedTabCount; > > That's backwards. browser.tabs.pinnedTabCount shouldn't determine what > window sessionstore restores first. > > > I suppose on window close we could update the pref if that window's > > gBrowser._numPinnedTabs is nonzero? > > Windows don't close in some particular order when quitting. I'm fine to change it, but I went with this because there wasn't an established notion of what window ought to be restored first, since they are just collected from SessionStoreInternal._windows[1], which is an object, and which I believe has an undefined ordering of keys when iterated. If I misunderstood something in the implementation here let me know. If we want to establish which window should be restored first, I think that's a good idea, but it seemed like a separate bug. [1] https://searchfox.org/mozilla-central/rev/12af4303ffd384bc2406b67f54ba68d8264d72c8/browser/components/sessionstore/SessionStore.jsm#3203
(In reply to Doug Thayer [:dthayer] from comment #21) > (In reply to Dão Gottwald [::dao] from comment #20) > > (In reply to Doug Thayer [:dthayer] from comment #19) > > > (In reply to Dão Gottwald [::dao] from comment #18) > > > > How are you ensuring that browser.tabs.pinnedTabCount isn't the number of > > > > pinned tabs of some random window, but of the window that sessionstore is > > > > going to restore as the first one? This isn't really obvious to me from > > > > looking at the patch. > > > > > > These lines: > > > > > > let pinnedTabCount = > > > Services.prefs.getIntPref("browser.tabs.pinnedTabCount", 0); > > > let initialWindow = root.windows.find(w => { > > > return w.tabs.filter(t => t.pinned).length == pinnedTabCount; > > > > That's backwards. browser.tabs.pinnedTabCount shouldn't determine what > > window sessionstore restores first. > > > > > I suppose on window close we could update the pref if that window's > > > gBrowser._numPinnedTabs is nonzero? > > > > Windows don't close in some particular order when quitting. > > I'm fine to change it, but I went with this because there wasn't an > established notion of what window ought to be restored first, since they are > just collected from SessionStoreInternal._windows[1], which is an object, > and which I believe has an undefined ordering of keys when iterated. No, that doesn't define the restoration order. Bug 1034036 should have made this more predictable -- we should be restoring the previous session's top-most window first.
Dao, do you know why selectedWindow isn't changed with the patches for but 1034036? It produces a really noisy experience where the topmost window on close is opened first, but then any window can be the one that ends up on top after that. Adjusting the patch to use the topmost window after bug 1034036 is trivial, but I noticed this oddness while implementing it and it seems worth addressing in another bug.
Flags: needinfo?(dao+bmo)
(In reply to Doug Thayer [:dthayer] from comment #23) > Dao, do you know why selectedWindow isn't changed with the patches for but > 1034036? It produces a really noisy experience where the topmost window on > close is opened first, but then any window can be the one that ends up on > top after that. Adjusting the patch to use the topmost window after bug > 1034036 is trivial, but I noticed this oddness while implementing it and it > seems worth addressing in another bug. Sorry, misread the code - selectedWindow is well defined, but I'm still seeing the behavior I mentioned.
(In reply to Doug Thayer [:dthayer] from comment #24) > (In reply to Doug Thayer [:dthayer] from comment #23) > > Dao, do you know why selectedWindow isn't changed with the patches for but > > 1034036? It produces a really noisy experience where the topmost window on > > close is opened first, but then any window can be the one that ends up on > > top after that. Adjusting the patch to use the topmost window after bug > > 1034036 is trivial, but I noticed this oddness while implementing it and it > > seems worth addressing in another bug. > > Sorry, misread the code - selectedWindow is well defined, but I'm still > seeing the behavior I mentioned. Not sure what's wrong -- better file a new bug and involve Mike.
Flags: needinfo?(dao+bmo)
Priority: -- → P2
Hey Dao, is this still on your radar to review? I should have been more clear but I think the issue in comment #23 is separate from this and doesn't need to block it.
Flags: needinfo?(dao+bmo)
(In reply to Doug Thayer [:dthayer] (PTO on May 17) from comment #27) > Hey Dao, is this still on your radar to review? I should have been more > clear but I think the issue in comment #23 is separate from this and doesn't > need to block it. Also, AFAICT bug 1235231 covers this.
Comment on attachment 8960382 [details] Bug 1442694 - Preopen pinned tabs before session restore https://reviewboard.mozilla.org/r/229166/#review253620 ::: browser/base/content/tabbrowser.js:319 (Diff revision 4) > + _preopenPinnedTabs() { > + let pinnedTabCount = 0; > + // __pinnedTabCount is set by SessionStore when opening new windows > + // for a session restore. > + if (window.__pinnedTabCount) { > + pinnedTabCount = window.__pinnedTabCount; So, as I understand is, the point of this bug is to let the first window prepare the tab strip before the session file has been read. When sessionstore itself opens the window, this optimization doesn't seem to make much sense. ::: browser/base/content/tabbrowser.js:326 (Diff revision 4) > + // This is a little weird, but it seems to be the simplest way to tell > + // if we're the first window. If only one window exists, it's us. This > + // will break if we open another browser window before running this. > + let windows = browserWindows(); > + windows.next(); > + let isFirstWindow = windows.next().done; Isn't this wrong on Mac where you can close the last browser window, then open a new one without quitting? ::: browser/base/content/tabbrowser.js:341 (Diff revision 4) > + skipBackgroundNotify: true, > + createLazyBrowser: true, > + }); > + tab._preopened = true; > + tab.setAttribute("busy", "true"); > + this.pinTab(tab, {skipNotify: true}); should use addTab's pinned option ::: browser/components/sessionstore/SessionStore.jsm:3517 (Diff revision 4) > - } > + } > - } > + } > + } > + > + if (aWindow == this.windowToFocus) { > + Services.prefs.setIntPref("browser.tabs.pinnedTabCount", numPinnedTabs); Why are you setting this here?
Attachment #8960382 - Flags: review?(dao+bmo) → review-
Flags: needinfo?(dao+bmo)
Comment on attachment 8960382 [details] Bug 1442694 - Preopen pinned tabs before session restore https://reviewboard.mozilla.org/r/229166/#review253620 > So, as I understand is, the point of this bug is to let the first window prepare the tab strip before the session file has been read. When sessionstore itself opens the window, this optimization doesn't seem to make much sense. I certainly added this for a reason, but I can't seem to reproduce any difference now with it gone, so I'll pull it out. > Isn't this wrong on Mac where you can close the last browser window, then open a new one without quitting? It's a bit of a misnomer in that scenario, and it's an unnecessary optimization, but I don't see it being harmful. If there's an easy way around this, I'm happy to implement it, but I can't think of one other than shoving some state in a jsm which we would then have to load? Or shoving it into an unrelated jsm that we've already loaded. > should use addTab's pinned option Updated, but we still have the problem of immediately inserting the browser due to the messageManager access from notifyPinnedStatus. I added a special parameter to avoid this rather than checking aCreateLazyBrowser to avoid giving people a footgun. > Why are you setting this here? Woops - outdated from when we were doing the tracking inside pinTab/unpinTab.
Comment on attachment 8960382 [details] Bug 1442694 - Preopen pinned tabs before session restore https://reviewboard.mozilla.org/r/229166/#review263094 ::: browser/base/content/tabbrowser.js:2513 (Diff revision 6) > t.setAttribute("fadein", "true"); > }); > } > > // Additionally send pinned tab events > - if (aPinned) { > + if (aPinned && !aSkipPinnedNotification) { Why not just (aPinned && !aCreateLazyBrowser)? ::: browser/components/sessionstore/SessionStore.jsm:3474 (Diff revision 6) > tabbrowser.updateBrowserRemoteness(tab.linkedBrowser, true); > } > } > > + if (tabData.pinned && > + tabbrowser.tabs[pinnedTabsReused] && nit: use t here rather than pinnedTabsReused ::: browser/components/sessionstore/SessionStore.jsm:3475 (Diff revision 6) > } > } > > + if (tabData.pinned && > + tabbrowser.tabs[pinnedTabsReused] && > + tabbrowser.tabs[pinnedTabsReused]._preopened) { Can't you just use linkedPanel instead of _preopened? ::: browser/modules/BrowserWindowTracker.jsm:96 (Diff revision 6) > if (idx >= 0) > _trackedWindows.splice(idx, 1); > } > > +function _trackPinnedTabs(window) { > + Services.prefs.setIntPref("browser.tabs.pinnedTabCount", Looks like this does't get updated when changing pinned tabs in the top-most window. ::: browser/modules/BrowserWindowTracker.jsm:96 (Diff revision 6) > if (idx >= 0) > _trackedWindows.splice(idx, 1); > } > > +function _trackPinnedTabs(window) { > + Services.prefs.setIntPref("browser.tabs.pinnedTabCount", nit: let's rename this pref to something more explicit to make it clear that this is a performance optimization for restoring the first window. Otherwise people might get confused about what this pref is about.
Attachment #8960382 - Flags: review?(dao+bmo)
Comment on attachment 8960382 [details] Bug 1442694 - Preopen pinned tabs before session restore https://reviewboard.mozilla.org/r/229166/#review263094 > Why not just (aPinned && !aCreateLazyBrowser)? I mentioned it briefly in one of the earlier comments. Essentially I wanted to make a special parameter to avoid the footgun. If I just used aCreateLazyBrowser, and someone tried to create a lazy, pinned browser in the future, they wouldn't know that they needed to ensure they sent the notification at a later time. This way they'll get a warning when the browser is prematurely inserted and (ideally) find this.
Comment on attachment 8960382 [details] Bug 1442694 - Preopen pinned tabs before session restore https://reviewboard.mozilla.org/r/229166/#review265660 ::: browser/base/content/tabbrowser.js:2525 (Diff revision 7) > }); > } > > // Additionally send pinned tab events > - if (aPinned) { > + if (aPinned && !aIsForFirstWindowRestore) { > this._notifyPinnedStatus(t); This method both sends the Browser:AppTab message and dispatches the TabPinned event. The event shouldn't be deferred, I don't think; these kind of tab events generally doen't care about what's going on with the tab's browser.
Attachment #8960382 - Flags: review?(dao+bmo)
Comment on attachment 8960382 [details] Bug 1442694 - Preopen pinned tabs before session restore https://reviewboard.mozilla.org/r/229166/#review267126 ::: browser/base/content/tabbrowser.js:568 (Diff revision 8) > this.tabContainer._positionPinnedTabs(); > this.tabContainer._updateCloseButtons(); > }, > > - _notifyPinnedStatus(aTab) { > + _notifyPinnedStatus(aTab, aDeferContentMessage = false) { > + if (!aDeferContentMessage) { Can you just check aTab.linkedPanel here? ::: browser/base/content/tabbrowser.js:587 (Diff revision 8) > + }, > + > + activatePreopenedPinnedTab(aTab) { > + delete aTab._preopened; > + this._insertBrowser(aTab); > + this.getBrowserForTab(aTab).messageManager.sendAsyncMessage("Browser:AppTab", { isAppTab: aTab.pinned }); Should we split the message and event parts of _notifyPinnedStatus into two methods so this code can be shared? ::: browser/base/content/tabbrowser.js:2213 (Diff revision 8) > referrerURI, > relatedToCurrent, > sameProcessAsFrameLoader, > skipAnimation, > skipBackgroundNotify, > + isForFirstWindowRestore, Please maintain the alphabetical order.
Attachment #8960382 - Flags: review?(dao+bmo)
Whiteboard: [fxperf:p1] → [fxperf:p2]
Doug, when you get back from leave, can you check what (if any) the next steps for this bug would be? Thank you!
Flags: needinfo?(dothayer)

Short answer: I don't know. There was work to be done when I last looked at this and I'm sure it has rotted.

Leaving the needinfo - I'll do some work to determine what this actually takes soon.

When we open firefox with pinned tabs, we first paint a window with
one tab open, and then that tab gets displaced after the pinned tabs
come in. This aims to ensure that our first paint contains the
pinned tab, so that we don't have tabs moving around after first
paint.

MozReview-Commit-ID: GC1y6NlgLTd

Dao, feel free to do a drive by review here - I put Gijs on the review in this case to not add more to your plate. Also Gijs feel free to punt this back if you don't feel comfortable reviewing it.

Also, regarding:

Can you just check aTab.linkedPanel here?

I went with an explicit parameter inside _notifyPinnedStatus to avoid a footgun where someone adds a lazy browser and we silently don't send the message. They will get a warning in this case when they prematurely create the browser.

Flags: needinfo?(dothayer)
Pushed by dothayer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7a20c79a0e12 Preopen pinned tabs before session restore r=Gijs https://hg.mozilla.org/integration/autoland/rev/f100f8631f78 Add tests for preopened pinned tabs r=Gijs

https://hg.mozilla.org/mozilla-central/rev/7a20c79a0e12
https://hg.mozilla.org/mozilla-central/rev/f100f8631f78

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67

I think this change had me lose my whole session on Windows nightly rev/110ea2a7c3d4f34b5079c195f7ea57966748e6da

I think I hit a condition where something is null (checked browser console). That something was very likely this.getBrowserForTab(aTab) @ chrome://browser/content/tabbrowser.js:621 and so anything after that just failed.

Unfortunately I can't confirm this anymore that it failed exactly there but it would make sense. I got to a state where I had two pinned tabs at one window - both without any content. That window and two others also had one "unloaded" normal tab.

I had several pinned tabs there (6?) but it is pretty likely that at least one of them was not loaded since I hadn't activated the tab in a long while - days, at least maybe even few weeks.

Backed out 2 changesets (Bug 1442694) for breaking session restores on update / requested by :dthayer on irc.

Status: RESOLVED → REOPENED
Flags: needinfo?(dothayer)
Resolution: FIXED → ---
Backout by shindli@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/4a87b6ac14c0 Backed out 2 changesets for breaking session restores on update a=backout
Depends on: 1530683

This adds test which reproduce the failure as well as the fix. Essentially,
if we hit the edited case in SessionStore with tab equal to tabbrowser.tabs[t],
we remove the tab and then try to pin it, which obviously blows up.

Note: the additional method in SessionStore.jsm was largely to get around
complexity requirements inside restoreWindow. Cleaner solutions welcome.

Depends on D19463

All right. I think I've done all I can reasonably do to be confident about this, but I am feeling a little timid about landing it due to the previous failures. I'm going to wait until Monday though and not land this on a Friday afternoon just in case. If anyone has any concerns in the mean time and would like me to test a particular case please let me know.

Flags: needinfo?(dothayer)
Pushed by dothayer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/59c8fffe9e41 Preopen pinned tabs before session restore r=Gijs https://hg.mozilla.org/integration/autoland/rev/3bd9591627ce Add tests for preopened pinned tabs r=Gijs https://hg.mozilla.org/integration/autoland/rev/8b3fe0426ffc Fix failures due to removing selected tab r=Gijs

https://hg.mozilla.org/mozilla-central/rev/59c8fffe9e41
https://hg.mozilla.org/mozilla-central/rev/3bd9591627ce
https://hg.mozilla.org/mozilla-central/rev/8b3fe0426ffc

Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
Depends on: 1532498
Performance Impact: --- → P2
Keywords: perf:frontend, perf:startup
Whiteboard: [fxperf:p2]
Severity: normal → S3
Assignee: dothayer → nobody
Status: REOPENED → RESOLVED
Closed: 6 years ago → 4 months ago
Flags: needinfo?(dothayer)
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: