Fix tab auto-discarding ability #13120

petemill · 2018-02-13T07:09:40Z

Fix #13210 (13120 / 13210 - what are the chances...)
Fix #1796
~~Depends on #13118 (new Debug menu for manually discarding a tab)~~

~~Depends on new muon version with brave/muon#504~~

Blocking issues

As described in #13210:

What this fixes

Ensure that tab/WebContents objects are added to memory as soon as they are created.

Use muon's new tab-replaced-at event (instead of .tab-id-changed) to ensure state gets updated correctly when a tab is discarded, as well as communicating temporary contents replacement to a window's frame (such as when detaching a tab to a new window, the tab in the old window gets a temporary WebContents that then gets destroyed).

In the case of tabs which get discarded, they are replaced with a fresh, clean WebContents. We were not receiving this new object, so when the renderer asked for this new object's tabId to be made active, the cache did not have that object.

We must also handle updating references to the old TabId to the new TabId in state, in the module which remembers opener tab IDs and the module which handles historical tab 'active' history for a window.

The guestInstanceId for a tab also changes when a discarded tab is reloaded, so make sure we pass that new value in the frame options when a discarded tab is detached to a new window.

Adds more relevant log entries under the --debug-tab-events flag.

Testing

Verification steps specified on #13210

And in addition, some modified tests from #12193, since now a tab ID can change, and we must update history for which tabs were active with the new tab ID.
Here those modified tests are:

NOTE: on Windows, you should disable tab preview

Closing

Parent tab

Preferences: default (When closing an active tab, select its parent tab)
Visit a site
Cmd-click 5 links to open 'child' tabs from that site
Switch active focus to the 3rd tab
Right-click the 1st tab and choose Discard
Close the 3rd (current) tab
Expected: Tab active focus switches to the 1st tab (parent)

Parent tab 2

Preferences: default (When closing an active tab, select its parent tab)
Visit a site
Cmd-click 5 links to open 'child' tabs from that site
Right-click the 3rd tab and choose Discard
Switch active focus to the 3rd tab, so it loads
Close the 3rd (current) tab
Expected: Tab active focus switches to the 1st tab (parent)

Next tab

Select the last tab
Right-click the previously-active 1st tab (new tab page) and choose Discard
Switch active focus back to that 1st tab, so it loads
Right-click -> Discard all the other tabs (except the current active tab)
Close the (active) new tab page
Expected: Tab active focus switches to the tab that was 2nd, and now is 1st
Preferences: When closing an active tab Select the next tab
Open the new tab page
Cmd-click all the top sites icons to open 'child' tabs
Switch active focus to the 3rd tab
Close the 3rd (current) tab
Expected: Tab active focus switches to the tab that was 4th, and is now 3rd

Last active tab

Preferences: When closing an active tab Select the last viewed tab
Open at least 5 tabs
Make the 1st tab active
Make the 3rd tab active
Make the last tab active
Right-click -> Discard all the tabs (except the current active tab)
Close the currently-active tab (the last tab)
Expected: Tab active focus switches to the 3rd tab
Close the currently-active tab (the 3rd tab)
Expected: Tab active focus switches to the 1st tab
Close the currently-active tab (the 1st tab)
Expected: Tab active focus switches to the 2nd tab

Detaching

Parent tab

Preferences: detault (When closing an active tab, select its parent tab)
Open the new tab page
Cmd-click all the top sites icons to open 'child' tabs
Switch active focus to the 3rd tab
Right-click / drag -> Detach the 3rd (current) tab
Expected: Tab active focus switches to the 1st tab (new tab page)

Next tab

Right-click / drag -> Detach the (active) new tab page
Expected: Tab active focus switches to the tab that was 2nd, and now is 1st
Preferences: When closing an active tab Select the next tab
Open the new tab page
Cmd-click all the top sites icons to open 'child' tabs
Switch active focus to the 3rd tab
Right-click / drag -> Detach the 3rd (current) tab
Expected: Tab active focus switches to the tab that was 4th, and is now 3rd

Last active tab

Preferences: When closing an active tab Select the last viewed tab
Open at least 5 tabs
Make the 1st tab active
Make the 3rd tab active
Make the last tab active
Right-click / drag -> Detach the currently-active tab (the last tab)
Expected: Tab active focus switches to the 3rd tab
Right-click / drag -> Detach the currently-active tab (the 3rd tab)
Expected: Tab active focus switches to the 1st tab
Right-click / drag -> Detach the currently-active tab (the 1st tab)
Expected: Tab active focus switches to the 2nd tab

For reference:

When a Tab is discarded, it is given a new Tab ID. It will use that new Tab ID when contents are then loaded in the future. The old Tab ID is not used anymore.
When a Tab is discarded, its blank replacement contents will cause app.on('web-contents-created') to be fired.
We will never receive a process.on('add-new-contents') event for the tab's new contents / tabId. So we rely on muon's new tab-replaced-at event which fires on the original tab.

codecov-io · 2018-02-13T18:47:26Z

Codecov Report

Merging #13120 into master will decrease coverage by 0.19%.
The diff coverage is 16.8%.

@@            Coverage Diff            @@
##           master   #13120     +/-   ##
=========================================
- Coverage   56.14%   55.95%   -0.2%     
=========================================
  Files         282      282             
  Lines       28074    28190    +116     
  Branches     4615     4639     +24     
=========================================
+ Hits        15763    15774     +11     
- Misses      12311    12416    +105

Flag	Coverage Δ
#unittest	`55.95% <16.8%> (-0.2%)`	⬇️

Impacted Files	Coverage Δ
js/constants/appConstants.js	`100% <ø> (ø)`	⬆️
js/stores/windowStore.js	`27.66% <0%> (-0.29%)`	⬇️
js/actions/appActions.js	`18.8% <0%> (-0.11%)`	⬇️
app/browser/webContentsCache.js	`54.76% <14.28%> (-20.24%)`	⬇️
app/browser/activeTabHistory.js	`77.77% <28.57%> (-12.55%)`	⬇️
app/browser/tabs.js	`23.91% <30.43%> (-0.22%)`	⬇️
app/common/state/tabState.js	`61.08% <6.06%> (-3.5%)`	⬇️
app/browser/reducers/tabsReducer.js	`40% <7.14%> (-1.2%)`	⬇️

NejcZdovc · 2018-02-14T05:59:39Z

#13118 was merged, so removing PR blocked label

petemill · 2018-02-22T19:26:55Z

This is ready for review but not merge until brave/muon#504 is merged, released and referenced in whichever branch this PR should end up in (currently 0.21.x).

bridiver · 2018-02-23T17:32:00Z

app/browser/tabs.js

+        appActions.tabReplaced(tabId, getTabValue(newTabId), getTabValue(tabId).get('windowId'), !isPlaceholder)
+        if (!isPlaceholder) {
+          // update in-memory caches
+          webContentsCache.tabIdChanged(tabId, newTabId)


I think this could introduce a race condition because the webContentsCache and activeTabHistory will be temporarily out-of-sync with the app state. Can we make them reducers instead that update on the tabReplaced action?

activeTabHistory and webContentsCache[].openerTabId (openerTabId is the only prop changed here) are only called in response to tab events coming from muon, not app state actions. Is that enough to satisfy your concern @bridiver? My thinking was to change them immediately to actually avoid a race condition, in case we're getting the tab 'detached' / 'destroyed' event quicker than the store can update those values - and we need the values updated then so that the next active TabId can be correctly chosen. Or is your concern that the tabReplaced action may rely on that data?

Either way, I should test that detaching a tab correctly sets the appropriate parent tab on detach, because there may be an issue with the 'temporary' contents id, in which case even if isPlaceholder === true we should call webContentsCache.tabIdChanged and activeTabHistory.tabIdChanged since that is window-specific and the info is needed on tab.on('will-destroy') which is fired very soon after tab-id-changed.

^ detaching is fine, and we shouldn't change the activehistory / openerTabId tabId for placeholder replacements since tabs.moveTo directly calls tabs.getNextActiveTabId passing in the moved tabId (and not the temporary replacement tabId) - so detaching and setting the appropriate next tab active in the departing window, depending on preference, works nicely.

petemill · 2018-02-23T19:03:29Z

I added some more test cases

petemill · 2018-02-23T19:21:27Z

Looks like the 'Parent Tab 2' discard test isn't working, I'm looking in to it
Edit: fixed

Requires brave/muon#504 Ensure that tab/WebContents objects are added to memory as soon as they are created. Use muon's new `tab-replaced-at` event (instead of <webview>.tab-id-changed) to ensure state gets updated correctly when a tab is discarded, as well as communicating temporary contents replacement to a window's frame (such as when detaching a tab to a new window, the tab in the old window gets a temporary WebContents that then gets destroyed). In the case of tabs which get discarded, they are replaced with a fresh, clean WebContents. We were not receiving this new object, so when the renderer asked for this new object's tabId to be made active, the cache did not have that object. We must also handle updating references to the old TabId to the new TabId in state, in the module which remembers opener tab IDs and the module which handles historical tab 'active' history for a window. The guestInstanceId for a tab also changes when a discarded tab is reloaded, so make sure we pass that new value in the frame options when a discarded tab is detached to a new window. Adds more relevant log entries under the `--debug-tab-events` flag.

bsclifton

Ran through entire test plan - everything working great 😄 Love the extra debugging info too

petemill · 2018-02-23T22:33:32Z

Thanks for the approval @bsclifton. Looking forward to getting it merged and out on beta release to test auto-discarding in the wild.

Fix tab auto-discarding ability

bsclifton · 2018-02-23T22:49:56Z

master f5db19e
0.22.x 73e52ac
0.21.x 541f346

Fix tab auto-discarding ability

petemill added perf regression PR/work-in-progress ⚒ PR/blocked labels Feb 13, 2018

petemill self-assigned this Feb 13, 2018

petemill requested review from bridiver, bbondy and bsclifton February 13, 2018 07:09

petemill added this to the 0.21.x (Beta Channel) milestone Feb 13, 2018

petemill force-pushed the fix/tabs-discard-dead-tab branch from 1685f5d to ab7b6a5 Compare February 13, 2018 18:47

alexwykoff added the priority/P1 Blocks development or testing. Product cannot run. Must be fixed immediately, shipped next release. label Feb 13, 2018

NejcZdovc removed the PR/blocked label Feb 14, 2018

petemill mentioned this pull request Feb 14, 2018

Dead tabs when system wakes up from sleep #9865

Closed

petemill added the PR/blocked label Feb 14, 2018

petemill force-pushed the fix/tabs-discard-dead-tab branch from ab7b6a5 to 26ba05c Compare February 14, 2018 07:42

petemill changed the title ~~Fix tab auto-discarding usability~~ Fix tab auto-discarding ability Feb 21, 2018

petemill force-pushed the fix/tabs-discard-dead-tab branch from 26ba05c to 2d576e5 Compare February 22, 2018 00:21

bbondy mentioned this pull request Feb 22, 2018

Temporarily disable autodiscardable tabs #12916

Closed

This was referenced Feb 22, 2018

Only one tab is stuck; it won't close or open #11145

Closed

Detaching a discarded tab and attaching to a new window causes a duplicate navigation entry to be added brave/muon#505

Closed

petemill force-pushed the fix/tabs-discard-dead-tab branch from c817fb0 to 4f80c90 Compare February 22, 2018 19:06

petemill removed PR/work-in-progress ⚒ regression labels Feb 22, 2018

petemill force-pushed the fix/tabs-discard-dead-tab branch 2 times, most recently from 74e4033 to 4cf22f8 Compare February 22, 2018 19:24

bsclifton mentioned this pull request Feb 23, 2018

Update to Muon 5.0.6 (which has tab discarding fixes!) #13282

Merged

10 tasks

bridiver reviewed Feb 23, 2018

View reviewed changes

petemill mentioned this pull request Feb 23, 2018

Tab content should be discarded when memory usage is too high #13210

Closed

4 tasks

bsclifton removed the PR/blocked label Feb 23, 2018

bsclifton force-pushed the fix/tabs-discard-dead-tab branch from 4cf22f8 to f77e33f Compare February 23, 2018 18:53

petemill force-pushed the fix/tabs-discard-dead-tab branch from f77e33f to c784d2b Compare February 23, 2018 19:56

bsclifton approved these changes Feb 23, 2018

View reviewed changes

bsclifton merged commit f5db19e into master Feb 23, 2018

bsclifton deleted the fix/tabs-discard-dead-tab branch February 23, 2018 22:37

bsclifton added a commit that referenced this pull request Feb 23, 2018

Merge pull request #13120 from brave/fix/tabs-discard-dead-tab

73e52ac

Fix tab auto-discarding ability

bsclifton added a commit that referenced this pull request Feb 23, 2018

Merge pull request #13120 from brave/fix/tabs-discard-dead-tab

541f346

Fix tab auto-discarding ability

petemill mentioned this pull request Mar 21, 2018

Tabs which start at about: urls (like new tab) won't ever be automatically discarded #13547

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix tab auto-discarding ability #13120

Fix tab auto-discarding ability #13120

petemill commented Feb 13, 2018 •

edited

Loading

codecov-io commented Feb 13, 2018 •

edited

Loading

NejcZdovc commented Feb 14, 2018

petemill commented Feb 22, 2018

bridiver Feb 23, 2018

petemill Feb 23, 2018

petemill Feb 23, 2018

petemill commented Feb 23, 2018

petemill commented Feb 23, 2018 •

edited

Loading

bsclifton left a comment

petemill commented Feb 23, 2018

bsclifton commented Feb 23, 2018 •

edited

Loading

Fix tab auto-discarding ability #13120

Fix tab auto-discarding ability #13120

Conversation

petemill commented Feb 13, 2018 • edited Loading

Blocking issues

What this fixes

Testing

Closing

Parent tab

Parent tab 2

Next tab

Last active tab

Detaching

Parent tab

Next tab

Last active tab

For reference:

codecov-io commented Feb 13, 2018 • edited Loading

Codecov Report

NejcZdovc commented Feb 14, 2018

petemill commented Feb 22, 2018

bridiver Feb 23, 2018

Choose a reason for hiding this comment

petemill Feb 23, 2018

Choose a reason for hiding this comment

petemill Feb 23, 2018

Choose a reason for hiding this comment

petemill commented Feb 23, 2018

petemill commented Feb 23, 2018 • edited Loading

bsclifton left a comment

Choose a reason for hiding this comment

petemill commented Feb 23, 2018

bsclifton commented Feb 23, 2018 • edited Loading

petemill commented Feb 13, 2018 •

edited

Loading

codecov-io commented Feb 13, 2018 •

edited

Loading

petemill commented Feb 23, 2018 •

edited

Loading

bsclifton commented Feb 23, 2018 •

edited

Loading