Import only history is adding duplicated entries

[GeetaSarvadnya]

Description

Import only history from b-l is adding history records trice in Brave Core

Steps to Reproduce

1. Launch b-l with clean profile
2. Add two sites to b-l and close browser laptop
3. Launch b-c with clean profile from CLI
4. Click on Book marks>import bookmarks and settings
5. Select Brave from import drop down
6. Un check all the check boxes except history
7. Click on import and done

Actual result:

Press ctrl+h to check the history, history page shows the imported sites trice

Expected result:

Imported history sites should display only once in b-c

Reproduces how often:

Always

Brave version (chrome://version info)

Brave	0.55.6 Chromium: 70.0.3538.16 (Official Build) dev (64-bit)
Revision	16ed95b41bb05e565b11fb66ac33c660b721f778-refs/branch-heads/3538@{#306}
OS	Windows

Reproducible on current release:

NA

Website problems only:

• Does the issue resolve itself when disabling Brave Shields? -NO
• Is the issue reproducible on the latest version of Chrome? - NO

Additional Information

@kjozwiak @LaurenWags @btlechowski @srirambv

[srirambv]

Reproduced on Linux.

[kjozwiak]

Also reproducible under `macOS 10.13.6 x64 using the following build:

Brave	0.55.6 Chromium: 70.0.3538.16 (Official Build) dev (64-bit)
Revision	16ed95b41bb05e565b11fb66ac33c660b721f778-refs/branch-heads/3538@{#306}

However, not always getting three copies, sometimes it's just duplicates. Either way, it's always three or two copies of the same history entry that's imported into b-c. Couldn't check if it's an issue with Chrome/Canary as they don't list Brave as one of the export options.

[garrettr]

The browser-laptop importer only appears to be triplicating or duplicating history entries: actually, it's importing all of the history entries from the Chromium/Muon history backend, which is a superset of the history entries that are listed in session-store-1 and thus are shown in browser-laptop's "Show History" view.

For example:

• Launch b-l using an empty/new user data directory: mkdir bl-test-udd && CHROME_USER_DATA_DIR=$(pwd)/bl-test-udd /Applications/Brave.app/Contents/MacOS/Brave.
• Type google.com in the address bar and press return/Enter.
• Open the history (about:history, Cmd-Y, etc.).
• You should see a single entry with title "Google" and domain "www.google.com"
• Quit b-l
• sqlite3 bl-test-udd/History "select * from urls where url glob '*google*';"
  • You will see at least 3 URLs containing google, each of which is part of the redirect chain from http://google.com --> https://google.com --> https://www.google.com.
• jq ".about.history.entries[] | .location" bl-test-udd/session-store-1
  • You will see one entry, for "https://www.google.com".

As we can see, only the final URL in the redirect chain is stored in session-store-1 and shown in "Show History," although every navigated URL is stored in the history db. The browser-laptop importer is currently importing every URL from the history db, including URLs that are usually hidden (because they are navigated from redirects, subframes, etc.).

Chromium similarly stores every navigated URL in the History db, but filters visits into "visible visits" (VisitDatabase::GetVisibleVisitsForURL) when displaying the history to the user. I did some further testing and confirmed that this issue also affects ChromeImporter, which is not surprising because BraveImporter inherits from ChromeImporter and their ImportHistory implementations are nearly identical.

Amusingly, since the original Muon implementation of ChromeImporter::ImportHistory appears to have been based on Firefox's implementation, I tested and confirmed that Firefox has this issue as well.

[garrettr]

I have considered the following potential fixes:

1. BraveImporter could import history entries from session-store-1, where they have already been filtered, instead of from the History sqlite db.
2. Follow the example of FirefoxImporter::ImportHistory, which filters the sqlite query to only import URLs that are "visible" in the importee's history.

I prefer the second proposal, because it should fix the issue for both ChromeImporter and BraveImporter. I think the fixes are probably equivalent, but have not yet dug into the browser-laptop source to confirm.

[kjozwiak]

I prefer the second proposal, because it should fix the issue for both ChromeImporter and BraveImporter.

I would agree as well regarding using the second method. In the past, we've seen session-store-1 files become corrupted or get into a strange state that caused other issues, particularly in Payments. If someone with session-store-1 that's corrupted or in a bad state, we might run into issues when attempting to import history.

CCing @bsclifton as he has a lot of experience dealing with session-store-1 files.

[garrettr]

After chatting with @bsclifton on Slack, it seems it would be best to import history for browser-laptop from session-store-1's historySites, since it's the data source for the "Show History"/about:history view.

[GeetaSarvadnya]

Verification Passed on

Brave	0.55.13 Chromium: 70.0.3538.54 (Official Build) beta (64-bit)
Revision	4f8e578b6680574714e9ed3bb9f02922b4dde40d-refs/branch-heads/3538@{#937}
OS	Windows

Verified passed with

Brave	0.55.13 Chromium: 70.0.3538.54 (Official Build) beta(64-bit)
Revision	4f8e578b6680574714e9ed3bb9f02922b4dde40d-refs/branch-heads/3538@{#937}
OS	Mac OS X

• Verified STR from description

Verification passed on

Brave	0.55.13 Chromium: 70.0.3538.54 (Official Build) beta(64-bit)
Revision	4f8e578b6680574714e9ed3bb9f02922b4dde40d-refs/branch-heads/3538@{#937}
OS	Linux