Bookmarks model not loaded #4588

AlexeyBarabash · 2020-02-11T14:57:35Z

Resolves brave/brave-browser#8178

Submitter Checklist:

Test Plan:

See in comments below

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

AlexeyBarabash · 2020-02-11T14:59:45Z

Here is a way how to emulate such case.

Revert PR Append --disable-sync swtich to disable sync temporarily #4565
Build browser
Launch profiles A and B and enable chain between them
Close browsers of profiles A and B
Put huge delay into model_loader.cc and build browser like below

git diff browser/model_loader.cc
diff --git a/components/bookmarks/browser/model_loader.cc b/components/bookmarks/browser/model_loader.cc
index 12fe8d32547f..b77e87ec8180 100644
--- a/components/bookmarks/browser/model_loader.cc
+++ b/components/bookmarks/browser/model_loader.cc
@@ -10,7 +10,8 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/bookmarks/browser/bookmark_storage.h"
 #include "components/bookmarks/browser/url_index.h"
-
+  #include "base/synchronization/waitable_event.h"
+  #include "base/threading/thread_restrictions.h"
 namespace bookmarks {
 
 namespace {
@@ -62,9 +63,17 @@ void ModelLoader::DoLoadOnBackgroundThread(
     scoped_refptr<base::SequencedTaskRunner> main_sequenced_task_runner,
     std::unique_ptr<BookmarkLoadDetails> details,
     LoadCallback callback) {
   LoadBookmarks(profile_path, emit_experimental_uma, details.get());
   history_bookmark_model_ = details->url_index();
+  const base::TimeDelta kExecutionTime(base::TimeDelta::FromMilliseconds(20*1000));
+  base::ScopedAllowBaseSyncPrimitivesForTesting allow_base_sync_primitives;
+  base::WaitableEvent hang_event_;
+  hang_event_.TimedWait(kExecutionTime);
   loaded_signal_.Signal();
   main_sequenced_task_runner->PostTask(
       FROM_HERE, base::BindOnce(&ModelLoader::OnFinishedLoad, this,
                                 std::move(details), std::move(callback)));
@@ -72,6 +81,7 @@ void ModelLoader::DoLoadOnBackgroundThread(
 
 void ModelLoader::OnFinishedLoad(std::unique_ptr<BookmarkLoadDetails> details,
                                  LoadCallback callback) {
   std::move(callback).Run(std::move(details));
 }

Launch profile A
See crash on pre-PR master

Test plan

Prepare a build with a patch introducing delay in bookmarks loading ^^^ .

Start a new sync chain

Take clean profileA
Go to about:flags#brave-sync to enable it on profileA, restart
As fast as you can, open brave://sync
Press Start a new Sync Chain
Wait 30 sec
Close device type dialog
Ensure browser hadn't crashed and there is one device in list
Press Add new device => Computer and save the code words, you will need them for the next test

Connect to existing sync chain

Take clean profileB
Go to about:flags#brave-sync to enable it on profileB, restart
As fast as you can, open brave://sync
Press I have a sync code, enter the code from previous test p.8
Wait 30 sec
Ensure browser hadn't crashed and there are devices in list

Sync is running after the browser restart
After above tests you have profileA and profileB in the same sync chain

Restart profiles A and B
Wait 30 sec
Ensure browsers hadn't crashed and there is Brave Sync extension activity in console in brave://inspect/#extensions

Sync successfully sends the bookmarked site at early point

Assume we have sync chain of profileA and profileB
Do quick:
2.1 Launch profileA
2.2 Open brave://bookmarks, it should be empty, even without permanent nodes
2.3 Once you will see the root node of Bookmarks , quick bookmark the current page through the button near the address bar;
you may not succeed from the first time, the botton may not be marked - then press it again until bookmark button will be marked;
2.4 Close profileA browser
Launch again profileA
Launch profileB
Ensure profileB will have bookmarked page Bookmarks after ~3 minutes

bridiver · 2020-02-12T23:19:01Z

components/brave_sync/brave_profile_sync_service_impl.h

+  // Methods to run specified actions right after bookmark model was loaded
+  void RunWhenModelIsLoaded(base::OnceCallback<void()> fn,
+                            const std::string& fn_name);
+  void CtorBkmLoaded(Profile* profile);


please don't abbreviate things like this in method names. We should use the same terminology everywhere for consistency

thanks, now this is fixed

bridiver · 2020-02-12T23:21:18Z

components/brave_sync/brave_profile_sync_service_impl.cc

@@ -280,6 +357,14 @@ void BraveProfileSyncServiceImpl::OnSetupSyncNewToSync(

  brave_sync_initializing_ = true;

+  RunWhenModelIsLoaded(


I'm concerned about running parts of these methods, but not completing them. I think it's likely to introduce subtle bugs. I think it would be better to just wait on everything so we'd have OnSetupSyncNewToSync that would call OnSetupSyncNewToSyncImpl when the bookmark model is loaded

thanks, now this is redone at 361b930

bridiver

this looks really dangerous to me in terms of introducing new race conditions. I think we need to preserve the order of operations in their entirety instead of piecemeal

components/brave_sync/brave_profile_sync_service_impl.h

darkdh

except some leftovers, otherwise looks good to me. Please squash some commits if necessary

components/brave_sync/brave_profile_sync_service_impl.h

components/brave_sync/brave_profile_sync_service_impl.cc

darkdh · 2020-02-13T22:53:42Z

components/brave_sync/brave_sync_service_unittest.cc

@@ -826,6 +826,20 @@ TEST_F(BraveSyncServiceTest, OnSyncReadyNewToSync) {
  EXPECT_TRUE(profile()->GetPrefs()->GetBoolean(syncer::prefs::kSyncBookmarks));
 }

+TEST_F(BraveSyncServiceTest, OnSyncReadyModelNotYetLoaded) {
+  EXPECT_NE(sync_service()->model_, nullptr);


I think we have better way to test if model is truly loaded not just nullptr
see components/bookmarks/test/test_bookmark_client.cc, we can have our own create method but not calling DoneLoading and then after calling OnSyncReady, we call DoneLoading

cool, thanks for the advise, will redo

redone, though I had to add patch to access private BookmarkModel::DoneLoading.
cc @bridiver

AlexeyBarabash · 2020-02-14T16:27:20Z

Updated tests and rebased.

components/brave_sync/brave_sync_service_unittest.cc

patches/components-bookmarks-browser-bookmark_model.h.patch

AlexeyBarabash · 2020-02-15T12:58:15Z

CI failed for iOS

loning into '/Users/jenkins/jenkins/workspace/ld-pr_bookmarks_model_not_loaded/src/_gclient_brave_qXZE2t'...
12:43:39  2>done.
12:43:55  Error: Command 'git ls-tree HEAD third_party/libgav1/src' returned non-zero exit status 128 in /Users/jenkins/jenkins/workspace/ld-pr_bookmarks_model_not_loaded/src
12:43:55  fatal: not a tree object

AlexeyBarabash · 2020-02-15T15:03:47Z

Have failed tests on CI

15:52:22  2 tests failed:
15:52:22      AutoplayPermissionContextBrowserTest.AskAutoplay (../../brave/browser/autoplay/autoplay_permission_context_browsertest.cc:227)
15:52:22      BraveRewardsBrowserTest.MultipleTipsProduceMultipleFeesWithVerifiedWallet (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:3021)

AlexeyBarabash · 2020-02-18T21:30:21Z

@bridiver , I have addressed your comments, could please re-review?

AlexeyBarabash · 2020-02-18T21:36:18Z

CI failed because of version mismatch though version in b-c was correct; deleted branch bookmarks_model_not_loaded at b-b and restarted ci

darkdh · 2020-02-19T00:56:08Z

0   libbase.dylib                       0x0000000107c76259 base::debug::CollectStackTrace(void**, unsigned long) + 9
1   libbase.dylib                       0x0000000107b31b63 base::debug::StackTrace::StackTrace() + 19
2   libbase.dylib                       0x0000000107c76101 base::debug::(anonymous namespace)::StackDumpSignalHandler(int, __siginfo*, void*) + 2385
3   libsystem_platform.dylib            0x00007fff6a4a542d _sigtramp + 29
4   ???                                 0x000000010bdbc668 0x0 + 4493919848
5   libbase.dylib                       0x0000000107baf7be base::SequenceCheckerImpl::CalledOnValidSequence() const + 30
6   brave_unit_tests                    0x0000000101fe9032 bookmarks::BookmarkModel::RemoveObserver(bookmarks::BookmarkModelObserver*) + 34
7   brave_unit_tests                    0x00000001010d83b3 brave_sync::BraveProfileSyncServiceImpl::~BraveProfileSyncServiceImpl() + 323
8   brave_unit_tests                    0x00000001010d859e brave_sync::BraveProfileSyncServiceImpl::~BraveProfileSyncServiceImpl() + 14
9   libkeyed_service_core.dylib         0x000000010b1e86e0 KeyedServiceFactory::Disassociate(void*) + 192
10  libkeyed_service_core.dylib         0x000000010b1e8762 KeyedServiceFactory::ContextDestroyed(void*) + 18
11  libkeyed_service_core.dylib         0x000000010b1e782c DependencyManager::PerformInterlockedTwoPhaseShutdown(DependencyManager*, void*, DependencyManager*, void*) + 668
12  brave_unit_tests                    0x00000001012c2a17 TestingProfile::~TestingProfile() + 119
13  brave_unit_tests                    0x00000001012c2e1e TestingProfile::~TestingProfile() + 14

5 tests crashed:
    AutoplayPermissionContextTests.TestNonAutoRefresh (../../brave/browser/autoplay/autoplay_permission_context_unittest.cc:126)
    BraveAutocompleteProviderClientUnitTest.SyncURLSuggestedNotSyncInternal (../../brave/browser/autocomplete/brave_autocomplete_provider_client_unittest.cc:52)
    BraveExtensionsAPIClientTests.IsBraveProtectedUrl (../../brave/browser/extensions/api/brave_extensions_api_client_unittest.cc:32)
    PushMessagingNotificationManagerTest.IsTabVisible (../../chrome/browser/push_messaging/push_messaging_notification_manager_unittest.cc:23)
    PushMessagingNotificationManagerTest.IsTabVisibleViewSource (../../chrome/browser/push_messaging/push_messaging_notification_manager_unittest.cc:41)

AlexeyBarabash · 2020-02-19T06:56:01Z

Pushed fix for crashed tests.
https://github.com/brave/brave-core/compare/36acb17cbb745a5a48aede95c31c206c591309e6..c73e82987cb02554c4535f855e3faad47ec55f76

@bridiver could you look on that please, as this fix reverts on of your review notices, now we check again, whether the observer was set before remove.

AlexeyBarabash · 2020-02-19T10:45:38Z

failed iOS build with

08:59:31  ERROR at //build/timestamp.gni:31:19: Script returned non-zero exit code.
08:59:31  build_timestamp = exec_script(compute_build_timestamp,
08:59:31                    ^----------
08:59:31  Current dir: /Users/jenkins/jenkins/workspace/ld-pr_bookmarks_model_not_loaded/src/out/ios_Release/
08:59:31  Command: python /Users/jenkins/jenkins/workspace/ld-pr_bookmarks_model_not_loaded/src/build/compute_build_timestamp.py official
08:59:31  Returned 1.
08:59:31  stderr:

failed linux browser test

09:48:54  ../../brave/browser/autoplay/autoplay_permission_context_browsertest.cc:510: Failure
09:48:54  Expected: (result) != (kVideoPlaying), actual: "Video playing" vs "Video playing"
09:48:54  Stack trace:
09:48:54  #0 0x56402bb1e209 (/home/ubuntu/jenkins/workspace/ld-pr_bookmarks_model_not_loaded/src/out/Release/brave_browser_tests+0x501c208)
....
09:50:51  1 test failed:
09:50:51      AutoplayPermissionContextBrowserTest.BlockAutoplay (../../brave/browser/autoplay/autoplay_permission_context_browsertest.cc:468)

failed MacOS build

10:07:58  In file included from ../../brave/browser/tor/tor_profile_service_impl.cc:14:
10:07:58  ../../brave/browser/brave_browser_process_impl.h:14:10: fatal error: 'brave/components/brave_referrals/buildflags/buildflags.h' file not found
10:07:58  #include "brave/components/brave_referrals/buildflags/buildflags.h"
10:07:58           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
10:07:58  1 error generated.

AlexeyBarabash · 2020-02-19T12:45:41Z

CI failed because could not apply patch to version, maybe correct chromium src dir wasn't fetched.

14:19:14  1 failed patches:
14:19:14  chrome/VERSION
14:19:14    - Patch applied because: No corresponding .patchinfo file was found.
14:19:14    - Error - Program git exited with error code 1.
14:19:14  error: patch failed: chrome/VERSION:1
14:19:14  error: chrome/VERSION: patch does not apply

removed b-b branch and rebased

AlexeyBarabash added bug feature/sync labels Feb 11, 2020

AlexeyBarabash requested review from bridiver and darkdh February 11, 2020 14:57

AlexeyBarabash self-assigned this Feb 11, 2020

AlexeyBarabash force-pushed the bookmarks_model_not_loaded branch from e23b7ca to e71adea Compare February 11, 2020 15:17

bridiver reviewed Feb 12, 2020

View reviewed changes

bridiver requested changes Feb 12, 2020

View reviewed changes

darkdh requested changes Feb 13, 2020

View reviewed changes

components/brave_sync/brave_profile_sync_service_impl.h Outdated Show resolved Hide resolved

components/brave_sync/brave_profile_sync_service_impl.h Outdated Show resolved Hide resolved

darkdh approved these changes Feb 13, 2020

View reviewed changes

components/brave_sync/brave_profile_sync_service_impl.h Outdated Show resolved Hide resolved

components/brave_sync/brave_profile_sync_service_impl.h Outdated Show resolved Hide resolved

components/brave_sync/brave_profile_sync_service_impl.cc Outdated Show resolved Hide resolved

darkdh reviewed Feb 13, 2020

View reviewed changes

AlexeyBarabash force-pushed the bookmarks_model_not_loaded branch 7 times, most recently from 25ef9c4 to 8e4b6c5 Compare February 14, 2020 16:26

AlexeyBarabash requested a review from bridiver February 14, 2020 16:29

darkdh reviewed Feb 14, 2020

View reviewed changes

components/brave_sync/brave_sync_service_unittest.cc Outdated Show resolved Hide resolved

darkdh reviewed Feb 14, 2020

View reviewed changes

patches/components-bookmarks-browser-bookmark_model.h.patch Outdated Show resolved Hide resolved

AlexeyBarabash force-pushed the bookmarks_model_not_loaded branch 3 times, most recently from 3f7b8f6 to 30342eb Compare February 15, 2020 10:36

AlexeyBarabash force-pushed the bookmarks_model_not_loaded branch from 30342eb to 6dbf658 Compare February 15, 2020 21:47

brave deleted a comment from AlexeyBarabash Feb 18, 2020

AlexeyBarabash force-pushed the bookmarks_model_not_loaded branch 4 times, most recently from a837357 to 36acb17 Compare February 18, 2020 21:22

AlexeyBarabash requested a review from bridiver February 18, 2020 21:29

bridiver approved these changes Feb 18, 2020

View reviewed changes

kjozwiak mentioned this pull request Feb 19, 2020

Sync crash on BraveMigrateOtherNode brave/brave-browser#8178

Closed

AlexeyBarabash force-pushed the bookmarks_model_not_loaded branch from 36acb17 to c73e829 Compare February 19, 2020 06:52

AlexeyBarabash requested a review from bridiver February 19, 2020 06:56

AlexeyBarabash force-pushed the bookmarks_model_not_loaded branch from c73e829 to 6a5794a Compare February 19, 2020 11:59

AlexeyBarabash added 2 commits February 19, 2020 14:42

OnSyncReady waits for bookmark model to load; fixes brave-browser#8178

380bbd4

Test for situation when OnSyncReady is loaded before model is loaded

0b659f2

AlexeyBarabash force-pushed the bookmarks_model_not_loaded branch from 6a5794a to 0b659f2 Compare February 19, 2020 12:43

AlexeyBarabash merged commit 6a22989 into master Feb 19, 2020

AlexeyBarabash deleted the bookmarks_model_not_loaded branch February 19, 2020 15:36

AlexeyBarabash added a commit that referenced this pull request Feb 19, 2020

Uplift of #4588 (squashed) to dev

07ed620

AlexeyBarabash added a commit that referenced this pull request Feb 19, 2020

Uplift of #4588 (squashed) to beta

242b0ed

This was referenced Feb 19, 2020

Bookmarks model not loaded (uplift to 1.5.x) #4687

Merged

Bookmarks model not loaded (uplift to 1.4.x) #4688

Merged

Remove wrong DCHECK; fixes brave-browser#8352 #4706

Merged

bbondy added this to the 1.7.x - Release milestone Jun 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bookmarks model not loaded #4588

Bookmarks model not loaded #4588

AlexeyBarabash commented Feb 11, 2020

AlexeyBarabash commented Feb 11, 2020 •

edited

Loading

bridiver Feb 12, 2020

AlexeyBarabash Feb 13, 2020

bridiver Feb 12, 2020

AlexeyBarabash Feb 13, 2020

bridiver left a comment

darkdh left a comment

darkdh Feb 13, 2020

AlexeyBarabash Feb 13, 2020

AlexeyBarabash Feb 14, 2020

AlexeyBarabash commented Feb 14, 2020

AlexeyBarabash commented Feb 15, 2020

AlexeyBarabash commented Feb 15, 2020

AlexeyBarabash commented Feb 18, 2020

AlexeyBarabash commented Feb 18, 2020

darkdh commented Feb 19, 2020

AlexeyBarabash commented Feb 19, 2020

AlexeyBarabash commented Feb 19, 2020

AlexeyBarabash commented Feb 19, 2020

		@@ -280,6 +357,14 @@ void BraveProfileSyncServiceImpl::OnSetupSyncNewToSync(

		brave_sync_initializing_ = true;

		RunWhenModelIsLoaded(

Bookmarks model not loaded #4588

Bookmarks model not loaded #4588

Conversation

AlexeyBarabash commented Feb 11, 2020

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

AlexeyBarabash commented Feb 11, 2020 • edited Loading

Test plan

bridiver Feb 12, 2020

Choose a reason for hiding this comment

AlexeyBarabash Feb 13, 2020

Choose a reason for hiding this comment

bridiver Feb 12, 2020

Choose a reason for hiding this comment

AlexeyBarabash Feb 13, 2020

Choose a reason for hiding this comment

bridiver left a comment

Choose a reason for hiding this comment

darkdh left a comment

Choose a reason for hiding this comment

darkdh Feb 13, 2020

Choose a reason for hiding this comment

AlexeyBarabash Feb 13, 2020

Choose a reason for hiding this comment

AlexeyBarabash Feb 14, 2020

Choose a reason for hiding this comment

AlexeyBarabash commented Feb 14, 2020

AlexeyBarabash commented Feb 15, 2020

AlexeyBarabash commented Feb 15, 2020

AlexeyBarabash commented Feb 18, 2020

AlexeyBarabash commented Feb 18, 2020

darkdh commented Feb 19, 2020

AlexeyBarabash commented Feb 19, 2020

AlexeyBarabash commented Feb 19, 2020

AlexeyBarabash commented Feb 19, 2020

AlexeyBarabash commented Feb 11, 2020 •

edited

Loading