Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable History datatype for Sync #20329

Merged
merged 15 commits into from
Oct 19, 2023
Merged

Enable History datatype for Sync #20329

merged 15 commits into from
Oct 19, 2023

Conversation

AlexeyBarabash
Copy link
Contributor

@AlexeyBarabash AlexeyBarabash commented Sep 28, 2023

Resolves brave/brave-browser#32876

This PR enables History datatype in Brave, becasue after CR119 the old TypedUrls datatype, which was used for History Sync, is wiped out from Chromium codebase.

Related server PR brave/go-sync#176 .

Key points of the PR:

  • reverts the PRs which were using TypedUrl: PR#16705 (Sync all history), PR#16478 (Display history sync diagnostic info), PR#20071 (Disable history sync datatype to fallback to typed urls datatype);
  • Overrides HistoryModelTypeController and HistoryDeleteDirectivesModelTypeController to allow start when encrypt everything option is enabled;
  • HISTORY datatype is included into encrypteble types, so only clients in the chain can see raw urls, server can't;
  • HISTORY_DELETE_DIRECTIVES is kept unencrypted, because it is a technical data; HistoryDeleteDirectiveSpecifics::UrlDirective::url field is forcibly cleaned out not to leak, chrome.history.deleteUrl extension API will not work properly;
  • modified LowPriorityUserTypes - HISTORY is removed, HISTORY_DELETE_DIRECTIVES added. We need this to make device first download History and then delete directives download and apply after. This patch will be reverted when the server will support delete directives.

Note: history entries created before enabling sync and turn history type on will not be sent to the server - inherited from upstream behavior .

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have [requested] . Requested https://github.com/brave/reviews/issues/1427
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Please note, this PR requires updated server, either built from brave/go-sync#176 or wait till it will be merged/deployed.

I. Browse/manual remove history entries

  1. Enable Sync on deviceA, turn on History datatype;
  2. Join deviceB to the chain, also turn on History datatype;
  3. From deviceA visit urls
    https://search.brave.com/search?q=1000001&source=web
    https://search.brave.com/search?q=1000002&source=web
    https://search.brave.com/search?q=1000003&source=web
  4. From deviceB visit urls
    https://search.brave.com/search?q=2000001&source=web
    https://search.brave.com/search?q=2000002&source=web
    https://search.brave.com/search?q=2000003&source=web
  5. Ensure both devices have those urls in history. Maybe need to wait ~1 minute max.
  6. From deviceA remove https://search.brave.com/search?q=2000002&source=web url from history; from deviceB remove https://search.brave.com/search?q=1000002&source=web url from history;
  7. Wait a little (< 1min), refresh history pages. Expected not to have https://search.brave.com/search?q=2000002&source=web and https://search.brave.com/search?q=1000002&source=web on both devices.
  8. Join deviceC to the sync chain, enable history
  9. Check brave://history page on deviceC - expected not to have urls https://search.brave.com/search?q=2000002&source=web , https://search.brave.com/search?q=1000002&source=web

II. Time-based history cleanup
You may use what you have left from test (I), or you join two devices to the sync chain, enable history, and do some browsing.

  1. open Clear browsing data dialog, brave://settings/clearBrowserData .
  2. On deviceA choose only Browsing history and Time range Last hour, press Clear data
  3. Ensure corresponding history entries were deleted on other devices.

III. Ensure url from chrome.history.deleteUrl is not leaked. This is test plan for developers or for security engineers, imho QA engineers may not go over the steps below

  1. Run browser with command line ./brave --enable-logging=stderr --v=0 --vmodule=commit.cc=3,*/sync/engine/*=3,traffic_logger=1 from terminal;
  2. Enable/join sync chain, enable history;
  3. Open page brave://inspect/#extensions
  4. Click inspect under Brave extension
  5. Paste at console
chrome.history.deleteUrl({url: 'https://developer.chrome.com/docs/extensions/reference/history/#method-search'}, function() {console.log("deleted");})
  1. search for "url_directive": { in terminal.
    Expected not to find such records.

IV. Android

  1. Join Android device to chain, enable history datatype;
  2. Expected to sync History between Android device and other devices in chain.

@AlexeyBarabash AlexeyBarabash added the CI/skip Do not run CI builds (except noplatform) label Sep 28, 2023
@github-actions github-actions bot added the CI/run-upstream-tests Run upstream unit and browser tests on Linux and Windows (otherwise only on Linux) label Sep 28, 2023
@AlexeyBarabash AlexeyBarabash removed the CI/skip Do not run CI builds (except noplatform) label Sep 28, 2023
@AlexeyBarabash
Copy link
Contributor Author

Removed CI/Skip to get some installers for draft PR

@AlexeyBarabash AlexeyBarabash changed the title Reverted PR#16705 (Sync all history) Enable History datatype for Sync Sep 28, 2023
@DJAndries DJAndries force-pushed the sync_history_datatype branch from c4da1d5 to 8a58dfb Compare October 6, 2023 23:41
@AlexeyBarabash AlexeyBarabash force-pushed the sync_history_datatype branch 5 times, most recently from 3c8701e to 0ba403c Compare October 12, 2023 10:10
@AlexeyBarabash AlexeyBarabash marked this pull request as ready for review October 12, 2023 11:41
@AlexeyBarabash AlexeyBarabash requested review from a team as code owners October 12, 2023 11:41
@AlexeyBarabash AlexeyBarabash merged commit dea0a62 into master Oct 19, 2023
6 checks passed
@AlexeyBarabash AlexeyBarabash deleted the sync_history_datatype branch October 19, 2023 21:11
@github-actions github-actions bot added this to the 1.61.x - Nightly milestone Oct 19, 2023
@github-actions github-actions bot added the CI/run-network-audit Run network-audit label Oct 19, 2023
emerick pushed a commit that referenced this pull request Oct 25, 2023
* Reverted PR#16705 (Sync all history)

* Reverted PR#16478 (Display history sync diagnostic info)

* Reverted PR#20071 (Disable history sync datatype to fallback to typed urls datatype)

* Added overrides for HistoryModelTypeController and HistoryDeleteDirectivesModelTypeController to work
when encrypt everything option is on

* Add patch to sync history delete directives and history entities together

* Added unit test for LowPriorityUserTypes

* Enable encryption of history sync entities

* Added test for precondition state of history datatype controllers

* Modified: clean out HistoryDeleteDirectiveSpecifics::UrlDirective::url field, as it is unencrypted

* Added test to ensure we don't send url at HistoryDeleteDirectiveSpecifics::UrlDirective

* Modified ModelTypeTest.EncryptableUserTypes test to check History type

* Removed 'path_excludes' for the files removed in the current PR

---------

Co-authored-by: Darnell Andries <[email protected]>
kjozwiak pushed a commit that referenced this pull request Oct 26, 2023
* Enable History datatype for Sync (#20329)

* Reverted PR#16705 (Sync all history)

* Reverted PR#16478 (Display history sync diagnostic info)

* Reverted PR#20071 (Disable history sync datatype to fallback to typed urls datatype)

* Added overrides for HistoryModelTypeController and HistoryDeleteDirectivesModelTypeController to work
when encrypt everything option is on

* Add patch to sync history delete directives and history entities together

* Added unit test for LowPriorityUserTypes

* Enable encryption of history sync entities

* Added test for precondition state of history datatype controllers

* Modified: clean out HistoryDeleteDirectiveSpecifics::UrlDirective::url field, as it is unencrypted

* Added test to ensure we don't send url at HistoryDeleteDirectiveSpecifics::UrlDirective

* Modified ModelTypeTest.EncryptableUserTypes test to check History type

* Removed 'path_excludes' for the files removed in the current PR

---------

Co-authored-by: Darnell Andries <[email protected]>

* Fix #33746: Cleanup Brave-Core iOS chromium_src overrides & initialization code (#20598)

- Cleanup Brave-Core IOS Chromium_SRC overrides.
- Add new cleaner Chromium_SRC overrides and simplify app launch logic.

* Don't redefine static_assert (#20440)

(Instead, we make sure the static_assert passes here by locally restoring
kCoarseResolutionMicroseconds back to its original Chromium value.)

* Use `PrefService` with `MigrateObsoleteProfilePrefs` (#20396)

Upstream is changing `MigrateObsoleteProfilePrefs` to take a
`PrefService` pointer, rather than `Profile`, as this is more expressive
of the data actually being accessed.

This change aligns all places where we have procedures similar to
`MigrateObsoleteProfilePrefs`, to receive a `PrefService*` instance as
well.

Chromium change:
https://chromium.googlesource.com/chromium/src/+/d848587fe1f19123940de4e4c0cec328bc23001f

commit d848587fe1f19123940de4e4c0cec328bc23001f
Author: Mikel Astiz <[email protected]>
Date:   Mon Oct 2 09:28:03 2023 +0000

    [sync] Remove legacy SyncRequested pref

    It has been practically unused for some time, and most of it is dead
    code ever since the feature toggle was cleaned up in
    https://crrev.com/c/4886256.

    On Ash, it needs to be migrated to a newly-introduced preference
    tailored to the actual underlying need, which is being able to remember
    that the server returned DISABLE_SYNC_ON_CLIENT (i.e. the user reset
    sync via dashboard).

    Change-Id: I0239447173b32d7d007ebf733dde23a5de1859e1
    Bug: 1443446, 1219990

* Ignore formatting for crates under `third_party` (#20390)

This changes adds a patch to add brave's own third party crates path to
the list of rust paths to be ignored during formatting.

* Merge pull request #20642 from brave/ios/bugfix/sync_all_history

* Merge pull request #20664 from brave/cr119-followup-disable-power-bookmarks

Disable kPowerBookmarksSidePanel feature flag

* [Android] Fix for private tab badge padding

Chromium change:
https://source.chromium.org/chromium/chromium/src/+/37d04b2089c15cb152a8a559887a7d10c66572be

[APS] Update tooltip visibility and remove hover highlight for StatusView

- Remove tooltip when StatusIconView is not visible.
- remove hover highlight when StatusIconView is not visible(e.g. default engine is not Google).
- remove hover highlight when VerboseStatusText is visible(e.g. offline).

demo(default engine not Google): https://drive.google.com/file/d/1DPIhHtWN4RnjPvkysyCSfV_gd73ZMjPb/view?usp=sharing

demo(offline)
https://drive.google.com/file/d/1-1Y_t59i8AcxFv6sLpBE-kG2tsSGoi4s/view?usp=sharing

TODO: relocate the logic in StatusView to StatusViewBinder or StatusProperties.

Bug: 1476816, 1476802

* Merge pull request #20314 from brave/cr119

Upgrade from Chromium 118 to Chromium 119

---------

Co-authored-by: AlexeyBarabash <[email protected]>
Co-authored-by: Darnell Andries <[email protected]>
Co-authored-by: Brandon-T <[email protected]>
Co-authored-by: Arthur Edelstein <[email protected]>
Co-authored-by: cdesouza-chromium <[email protected]>
Co-authored-by: Soner YUKSEL <[email protected]>
Co-authored-by: Artem Samoilenko <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/run-network-audit Run network-audit CI/run-upstream-tests Run upstream unit and browser tests on Linux and Windows (otherwise only on Linux) feature/sync
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Sync] Enable History sync data type
5 participants