Stop losing pinned tabs in a window when adding or removing another pinned tab #10531

petemill · 2017-08-16T08:50:45Z

Fixes #3760
Fixes #9635
Fixes #10037
Fixes #10510

Possibly addresses: #10117 and #10179

The previous code resulted in tabs being marked as no longer required to be pinned, and closed prematurely. This happened because it maintained a 'cache' of pinned sites (as Immutable Maps), and was using .equals to the incoming list of sites. Whilst Immutable Map's .equals() (and .is()) compare properties of two Maps, the incoming objects are often not equal to the object seen in the past, even though it represents the same 'site'. For example, if we re-order a site, it will have a different Order property.

In these situations, when pinning a tab, all or some previously pinned tabs (ones that had any property change) would be closed. Sometimes this appeared like a 'crash' when the window closed because all the tabs had been (undesirably) closed.

Request review from those involved in Issues: @bsclifton @LaurenWags @luixxiul

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.

Test Plan:

Case 1 (#9635)

Steps to reproduce:

Clear the browser profile
Open https://badssl.com
Open 5 domains from https-everywhere to longextended...
Pin all of them, including the parent tab
Reorder them randomely
Open a dashboard
Click the twitter tile
Pin the twitter tab
[Should not] Crash!
Restart the browser
Confirm you have the pinned twitter page at the front of the pinned tabs row

Case 2 (#10037)

Steps to reproduce:

Open github.com in a tab
Open twitter.com in a new tab
Pin github
Pin twitter
Reorder the two pinned tabs
Unpin github
[Previous] Actual result: twitter, the other pinned tab, disappears
Expected result: it should not disapper

Case 3 (#10510)

Clean profile
launch Brave
Don't close or use either of the tabs. Leave as is.
Open a new tab
visit a site
pin the tab
Open another new tab
visit a different site,
pin the tab
Move this tab to the first position.
Click on the about:welcome tab.
Change the URL to a different site.
Pin this tab.
two previously pinned tabs are not displayed.
[Previous] Actual result: Previously pinned tabs are not displayed. Must close/relaunch Brave to see them.
Expected result: Pinned tabs should not disappear.

Automated Test:

Created new test - pinnedTabs -> Moving pinned tabs -> reorders pins without forgetting about them
Fails on master:

  1 failing

  1) pinnedTabs Moving pinned tabs reorders pins without forgetting about them:
     element ("[data-test-id="tab-area"][data-frame-key="3"] + [data-test-id="tab-area"][data-frame-key="2"] + [data-test-id="tab-area"][data-frame-key="5"]") still not existing after 10000ms
  Error

Succeeds with this PR:

  pinnedTabs
    Moving pinned tabs
      ✓ reorders pins without forgetting about them (3266ms)

Reviewer Checklist:

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

codecov-io · 2017-08-16T09:11:22Z

Codecov Report

Merging #10531 into master will increase coverage by 0.22%.
The diff coverage is 96%.

@@            Coverage Diff             @@
##           master   #10531      +/-   ##
==========================================
+ Coverage   54.17%   54.39%   +0.22%     
==========================================
  Files         246      246              
  Lines       21374    21350      -24     
  Branches     3316     3317       +1     
==========================================
+ Hits        11579    11614      +35     
+ Misses       9795     9736      -59

Flag	Coverage Δ
#unittest	`54.39% <96%> (+0.22%)`	⬆️

Impacted Files	Coverage Δ
js/actions/appActions.js	`14.38% <ø> (+1.43%)`	⬆️
app/renderer/components/tabs/tab.js	`70.49% <100%> (ø)`	⬆️
app/browser/windows.js	`29.87% <95.83%> (+11.43%)`	⬆️
app/common/state/tabState.js	`78.58% <0%> (+1.55%)`	⬆️

NejcZdovc · 2017-08-16T09:14:48Z

@petemill this will solve a lot of problems with pinned tabs, thank you. Can you please add some test coverage for this new code and use cases?

diracdeltas · 2017-08-19T01:09:03Z

app/browser/windows.js

@@ -62,51 +61,44 @@ const updateWindow = (windowId) => {
 }
 }

+const siteMatchesTab = (site, tab) => {
+ const matchesLocation = getLocationIfPDF(tab.get('url')) === site.get('location')
+ const matchesPartition = (tab.get('partitionNumber') || 0) === (site.get('partitionNumber') || 0)


you could use Immutable's notSetValue parameter instead of ||. https://facebook.github.io/immutable-js/docs/#/Map/get

bsclifton · 2017-08-22T17:35:11Z

Note to reviewers: let's see if this PR closes #9763

petemill · 2017-08-23T23:26:14Z

I'm pretty sure #9763 is a duplicate of #10510 and #9635 @bsclifton. #3760 is more generic about all the issues with losing pinned tabs and #10037 is about unpinning something and seeing the behavior, compared to the others which only mention steps to pin. But essentially they are caused by the same issue.

darkdh · 2017-08-25T23:46:31Z

app/browser/windows.js

+ for (const tab of pinnedTabs.values()) {
+ const shouldBePinned = tabsWithSites.has(tab)
+ if (!shouldBePinned) {
+ appActions.tabCloseRequested(tab.get('tabId'), true)


In what case will require us to manually close tab?

@darkdh this happens when you have > 1 window open, you then unpin a tab in window 1. This code calls upon window 2+ to close the previously-pinned tab. Whether or not this is what we want to happen I guess is beyond the scope of this bug fix, since that is the previous functionality. I myself have noticed some edge-cases that could end up with losing user webview state in a tab, but I'll log a separate issue for that...

petemill · 2017-08-27T00:47:15Z

@NejcZdovc I added more info about what caused this bug - it's the fact that the previously-pinned sites were being stored as Immutable Maps, which were then equality compared to the incoming updated list. Often, the same site would not match one in the 'cache', because some other property changed (such as the order after re-ordering the pinned tabs).

I'm in the process of adding a webdriver test for this case.

petemill · 2017-08-27T05:02:11Z

Test added and described in description. Ready for next round of review.

NejcZdovc · 2017-08-28T10:59:33Z

app/common/lib/pinnedSitesUtil.js


- let location = siteDetail.get('location')
+const getKeyFromLocation = (location, partitionNumber) => {


Not sure why we need to move this in to the new function. You could just create Immutable.Map() in your test (as far as I can see is that the only place that you use it).

const key = pinnedSitesUtil. getKey(Immutable.fromJS({ location: 'something', partitionNumber: 1 }))

@NejcZdovc the aim here was to avoid having 2+ separate places where this format of string 'key' for pinned tabs is defined (${location}|${parititionNumber}). The action onPinnedTabReorder requires this key, the generation of which which was previously hidden inside another function.
Here, I expose the getKey function from one place, so that if we want to change that format (perhaps introduce a new property, or change the separator charactor, etc) it is controlled by a single function.
Now that I've explained the why, I'm happy to change it - maybe I can get it from the existing state, or the DOM, or something... ☺️

Do we have it defined in two places? 😃 I am not aware of it. For pinnedSites we have only one getKey and format AFAIK

Well there would be if I added a new place here 😀 , but I've taken your advice and instead made copies of the 'SiteDetail' structure the original function requires.

darkdh

++

diracdeltas · 2017-08-28T23:35:02Z

some of the pinned tab tests are failing but i don't know offhand if they are intermittent failures. i'll restart the travis job. https://travis-ci.org/brave/browser-laptop/jobs/269271637

update: looks like the same pinnedTab tests fail locally on master and this branch, so they are probably unrelated failures.

bridiver · 2017-08-28T23:37:03Z

this seems highly likely to conflict with @bbondy's changes

diracdeltas

test plan worked

bsclifton · 2017-08-29T05:33:02Z

app/browser/windows.js

 })
-


This (the new implementation) is definitely a lot easier to read; so basically we're just comparing the sites entries to the pinned tab app state.

entries in sites AND app state are noop (they already exist, no action needed)

entries in sites and NOT in app state are created

entries NOT in sites which are in app state are closed

bsclifton · 2017-08-29T05:43:49Z

app/browser/windows.js

 })
-
- const sitesToAdd = pinnedSites.filter((site) =>
- !win.__alreadyPinnedSites.find((pinned) => pinned.equals(site)))


so this here was the actual bug; comparing site entries from pinnedSites to the ones in __alreadyPinnedSites

Yes, though mainly on the previous L100 when determining which should be closed. The bug could have been remedied by hacking that line, but the __alreadyPinnedSites cache wasn't helping much and was growing in size on each call because of L82, so seemed better to rewrite.

…inned tab Fix brave#3760, fix brave#9635, fix brave#10037, fix brave#10510, and possibly addresses brave#10117 The previous code resulted in tabs being marked as no longer required to be pinned, and closed prematurely. This mostly occured after a pinned-tab reordering - afterwards, when pinning or unpinning a tab, all previously pinned tabs with a new order would be closed. Sometimes this appeared like a 'crash' when the window closed because all the tabs had been (undesirably) closed.

@petemill

Auditors: @petemill Test Plan: `npm run unittest -- --grep="window API unit tests"`

bsclifton

Changes look good! 😄 I added some unit tests if you wanted to check them out

petemill · 2017-08-29T15:39:13Z

Thanks all for your review and thanks @bsclifton for adding new unit tests for that module!

luixxiul added this to the 0.21.x (Nightly Channel) milestone Aug 16, 2017

luixxiul added the feature/pinned-tabs label Aug 16, 2017

luixxiul requested review from bridiver, diracdeltas and darkdh August 16, 2017 09:17

luixxiul assigned petemill Aug 16, 2017

NejcZdovc mentioned this pull request Aug 18, 2017

MacOS Specific - Pinning a normal tab causes browser crash but retains the pinned tabs upon restart #9763

Closed

This was referenced Aug 18, 2017

Browser crashes by pinning sites, reordering them, and visiting a site via dashboard #9635

Closed

pinned tab disappears by reordering and unpinning #10037

Closed

LaurenWags mentioned this pull request Aug 18, 2017

pinned tabs disappear until close/relaunch #10510

Closed

diracdeltas reviewed Aug 19, 2017

View reviewed changes

darkdh reviewed Aug 25, 2017

View reviewed changes

petemill force-pushed the fix-pinned-tabs branch from 6248c7c to f912e34 Compare August 26, 2017 05:06

petemill force-pushed the fix-pinned-tabs branch from f912e34 to a9443d5 Compare August 27, 2017 04:59

petemill force-pushed the fix-pinned-tabs branch from a9443d5 to f9a6772 Compare August 27, 2017 05:05

NejcZdovc requested review from diracdeltas and darkdh August 28, 2017 10:51

NejcZdovc reviewed Aug 28, 2017

View reviewed changes

darkdh approved these changes Aug 28, 2017

View reviewed changes

srirambv mentioned this pull request Aug 28, 2017

Brave crashes when tabs are pinned(Intermittent issues) #3760

Closed

petemill force-pushed the fix-pinned-tabs branch from f9a6772 to a8e12ab Compare August 28, 2017 16:56

bsclifton self-requested a review August 28, 2017 23:07

diracdeltas approved these changes Aug 29, 2017

View reviewed changes

bsclifton reviewed Aug 29, 2017

View reviewed changes

petemill and others added 2 commits August 29, 2017 00:54

Added unit tests, fixed lint error

4ced9dd

Auditors: @petemill Test Plan: `npm run unittest -- --grep="window API unit tests"`

bsclifton force-pushed the fix-pinned-tabs branch from 869ad09 to 4ced9dd Compare August 29, 2017 07:55

bsclifton approved these changes Aug 29, 2017

View reviewed changes

bsclifton merged commit c93dbd8 into brave:master Aug 29, 2017

This was referenced Aug 29, 2017

Brave should not exit when every tab is pinned #10117

Closed

pinned tabs either missing or duplicated after Brave relaunch #10179

Closed

petemill deleted the fix-pinned-tabs branch October 17, 2017 17:29

bbondy modified the milestones: 0.21.x (Developer Channel), 0.20.x (Beta Channel) Oct 25, 2017

petemill mentioned this pull request Nov 22, 2017

Hide windows until they are either rendered or a timeout expires #11764

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop losing pinned tabs in a window when adding or removing another pinned tab #10531

Stop losing pinned tabs in a window when adding or removing another pinned tab #10531

petemill commented Aug 16, 2017 •

edited

Loading

codecov-io commented Aug 16, 2017 •

edited

Loading

NejcZdovc commented Aug 16, 2017

diracdeltas Aug 19, 2017

bsclifton commented Aug 22, 2017

petemill commented Aug 23, 2017

darkdh Aug 25, 2017

petemill Aug 26, 2017

petemill commented Aug 27, 2017

petemill commented Aug 27, 2017

NejcZdovc Aug 28, 2017 •

edited

Loading

petemill Aug 28, 2017

NejcZdovc Aug 28, 2017

petemill Aug 28, 2017

darkdh left a comment

diracdeltas commented Aug 28, 2017 •

edited

Loading

bridiver commented Aug 28, 2017

diracdeltas left a comment

bsclifton Aug 29, 2017 •

edited

Loading

bsclifton Aug 29, 2017

petemill Aug 29, 2017

bsclifton left a comment

petemill commented Aug 29, 2017


		let location = siteDetail.get('location')
		const getKeyFromLocation = (location, partitionNumber) => {

Stop losing pinned tabs in a window when adding or removing another pinned tab #10531

Stop losing pinned tabs in a window when adding or removing another pinned tab #10531

Conversation

petemill commented Aug 16, 2017 • edited Loading

Case 1 (#9635)

Case 2 (#10037)

Case 3 (#10510)

Automated Test:

codecov-io commented Aug 16, 2017 • edited Loading

Codecov Report

NejcZdovc commented Aug 16, 2017

Choose a reason for hiding this comment

bsclifton commented Aug 22, 2017

petemill commented Aug 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petemill commented Aug 27, 2017

petemill commented Aug 27, 2017

NejcZdovc Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkdh left a comment

Choose a reason for hiding this comment

diracdeltas commented Aug 28, 2017 • edited Loading

bridiver commented Aug 28, 2017

diracdeltas left a comment

Choose a reason for hiding this comment

bsclifton Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

petemill commented Aug 29, 2017

petemill commented Aug 16, 2017 •

edited

Loading

codecov-io commented Aug 16, 2017 •

edited

Loading

NejcZdovc Aug 28, 2017 •

edited

Loading

diracdeltas commented Aug 28, 2017 •

edited

Loading

bsclifton Aug 29, 2017 •

edited

Loading