BookmarkModel needs to be refactored to allow the history related functions to be called after ~BookmarkModel |
||||||||||||||||||||||||
Issue description"OmniboxViewTest.DeleteItem" is flaky. This issue was created automatically by the chromium-try-flakes app. Please find the right owner to fix the respective test/step and assign this issue to them. If the step/test is infrastructure-related, please add Infra-Troopers label and change issue status to Untriaged. When done, please remove the issue from Sheriff Bug Queue by removing the Sheriff-Chromium label. We have detected 12 recent flakes. List of all flakes can be found at https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyJQsSBUZsYWtlIhpPbW5pYm94Vmlld1Rlc3QuRGVsZXRlSXRlbQw. Flaky tests should be disabled within 30 minutes unless culprit CL is found and reverted. Please see more details here: https://sites.google.com/a/chromium.org/dev/developers/tree-sheriffs/sheriffing-bug-queues#triaging-auto-filed-flakiness-bugs This flaky test/step was previously tracked in issue 679124 .
,
Jan 12 2017
Logging output (e.g. https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.win%2Fwin_chromium_x64_rel_ng%2F343754%2F%2B%2Frecipes%2Fsteps%2Finteractive_ui_tests__with_patch_%2F0%2Flogs%2FOmniboxViewTest.DeleteItem%2F0 ) says: [4424:4792:0106/124121.761:FATAL:waitable_event_win.cc(62)] Check failed: ((((DWORD )0x00000000L) ) + 0 ) == result (0 vs. 4294967295)WaitForSingleObject failed Backtrace: base::debug::StackTrace::StackTrace [0x000000014156AA31+33] logging::LogMessage::~LogMessage [0x00000001414F716A+74] base::WaitableEvent::Wait [0x0000000141529773+163] ChromeHistoryBackendClient::IsBookmarked [0x0000000140B0F326+34] IsBookmarked() is blocking on the bookmark model loading, and the underlying WaitForSingleObject() call is failing. First, waitable_event_win.cc should probably print out the results of GetLastError() in that DCHECK failure. The return code 0xFFFFFFFF is just WAIT_FAILED, GetLastError() says why. WIthout knowing this it's hard to know why the test failed. Second, it's bad that calling IsBookmarked() will indefinitely block one thread (the UI thread? The history thread? I'm not sure) on another thread that's loading something. That's a fundamentally unsafe pattern. Instead, either this function should just return false if things aren't loaded, or it and the caller(s) need to understand that this is an async request, and implement some kind of callback/observer mechanism to deal with things correctly. Fixing this should fix the failure here as a side effect. Codesearch says the only two callers of BookmarkModel::BlockTillLoaded are in chrome_history_backend_client.cc, so hopefully this is feasible to fix. I would expect this to affect any code that directly or indirectly calls IsBookmarked(), which could be a lot of code. Just disabling this test doesn't seem like a good response even as a bandaid. sdefresne and sky are the OWNERS for chrome/browser/history/chrome_history_backend_client.cc; reassigning for further triage.
,
Jan 13 2017
BlockTillLoaded is called on the history thread when history needs to ensure bookmarks are loaded. We can't make IsBookmarked() return false if not loaded, as that could result in doing the wrong thing (say deleting some urls that are bookmarked). To change history to deal differently would effectively require history to queue up operations until loaded and then continue on. What is the test flakiness you are seeing that results from this? What is the test doing/relying?
,
Jan 13 2017
Detected 5 new flakes for test/step "OmniboxViewTest.DeleteItem". To see the actual flakes, please visit https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyJQsSBUZsYWtlIhpPbW5pYm94Vmlld1Rlc3QuRGVsZXRlSXRlbQw. This message was posted automatically by the chromium-try-flakes app. Since flakiness is ongoing, the issue was moved back into Sheriff Bug Queue (unless already there).
,
Jan 16 2017
Detected 3 new flakes for test/step "OmniboxViewTest.DeleteItem". To see the actual flakes, please visit https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyJQsSBUZsYWtlIhpPbW5pYm94Vmlld1Rlc3QuRGVsZXRlSXRlbQw. This message was posted automatically by the chromium-try-flakes app.
,
Jan 17 2017
+pkasting@, it looks like sky@ had a question for you, but you didn't see it because you weren't cc'd.
,
Jan 17 2017
I'm the chromium sheriff, and I'm going to disable this test anyway, as per my instructions. Please fix the actual issue as soon as possible so we can re-enable the test.
,
Jan 17 2017
@3: I already posted everything I know. The test is trying to delete an item from history. History is trying to wait on the bookmark model being loaded. The WaitForSingleObject() call is failing, causing a DCHECK failure. I have no idea why that call is failing. The test itself is at https://cs.chromium.org/chromium/src/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc?rcl=0&l=1221 and a sample stack trace is at https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.win%2Fwin_chromium_x64_rel_ng%2F343754%2F%2B%2Frecipes%2Fsteps%2Finteractive_ui_tests__with_patch_%2F0%2Flogs%2FOmniboxViewTest.DeleteItem%2F0 , but those won't tell you more than what I've just said :)
,
Jan 17 2017
,
Jan 17 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/90fc0c189acd57a5365f07b6613d25b78011cddd commit 90fc0c189acd57a5365f07b6613d25b78011cddd Author: meade <meade@chromium.org> Date: Tue Jan 17 04:43:23 2017 Disable OmniboxViewTest.DeleteItem on windows due to continuing flakiness BUG= 680698 TBR=thomasanderson@chromium.org Review-Url: https://codereview.chromium.org/2637763004 Cr-Commit-Position: refs/heads/master@{#443988} [modify] https://crrev.com/90fc0c189acd57a5365f07b6613d25b78011cddd/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc
,
Jan 17 2017
Detected 7 new flakes for test/step "OmniboxViewTest.DeleteItem". To see the actual flakes, please visit https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyJQsSBUZsYWtlIhpPbW5pYm94Vmlld1Rlc3QuRGVsZXRlSXRlbQw. This message was posted automatically by the chromium-try-flakes app. Since flakiness is ongoing, the issue was moved back into Sheriff Bug Queue (unless already there).
,
Jan 17 2017
This test is no longer being run, therefore it can't be flaky so removing sheriff label. If other related things are flaky, we can deal with them when they come up.
,
Jan 19 2017
,
Jan 20 2017
,
Jan 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8f3596ad8588a28907ccfca0f9c9af984e3c5681 commit 8f3596ad8588a28907ccfca0f9c9af984e3c5681 Author: gab <gab@chromium.org> Date: Fri Jan 20 01:56:12 2017 Revert of Enable BrowserScheduler.RedirectHistoryService experiment on trunk. (patchset #7 id:140001 of https://codereview.chromium.org/2592813002/ ) Reason for revert: Suspected culprit http://crbug.com/680698 and http://crbug.com/682219 per HistoryService tasks no longer being guaranteed to be flushed in ~HistoryService -- which BookmarksModel depends on. Original issue's description: > Enable BrowserScheduler.RedirectHistoryService experiment on trunk. > > BUG=661143 > Committed: https://crrev.com/4aa30591e0953da7e0c54ffdb91856e496d16b5f > Cr-Commit-Position: refs/heads/master@{#440418} > > Reverted: https://crrev.com/2885403f92c9ab0a8769c2c41e859502da5a7e6f > Cr-Commit-Position: refs/heads/master@{#440479} > Cause of revert fixed in https://codereview.chromium.org/2611053003/. > > Review-Url: https://codereview.chromium.org/2592813002 > Cr-Commit-Position: refs/heads/master@{#441997} > Committed: https://chromium.googlesource.com/chromium/src/+/344517411399fc467e5a6938e4793650729b3506 TBR=rkaplow@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=661143, 680698 , 682219 Review-Url: https://codereview.chromium.org/2642253002 Cr-Commit-Position: refs/heads/master@{#444942} [modify] https://crrev.com/8f3596ad8588a28907ccfca0f9c9af984e3c5681/testing/variations/fieldtrial_testing_config.json
,
Jan 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6c59727f79b04675fdc9dc663fa36d16b23535b4 commit 6c59727f79b04675fdc9dc663fa36d16b23535b4 Author: gab <gab@chromium.org> Date: Fri Jan 20 07:50:48 2017 Revert of Disable OmniboxViewTest.DeleteItem on windows due to continuing flakiness (patchset #1 id:1 of https://codereview.chromium.org/2637763004/ ) Reason for revert: Trying to re enable after flake culprit was reverted Original issue's description: > Disable OmniboxViewTest.DeleteItem on windows due to continuing flakiness > > BUG= 680698 > TBR=thomasanderson@chromium.org > > Review-Url: https://codereview.chromium.org/2637763004 > Cr-Commit-Position: refs/heads/master@{#443988} > Committed: https://chromium.googlesource.com/chromium/src/+/90fc0c189acd57a5365f07b6613d25b78011cddd TBR=alancutter@chromium.org,thomasanderson@chromium.org,meade@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= 680698 Review-Url: https://codereview.chromium.org/2648623003 Cr-Commit-Position: refs/heads/master@{#445011} [modify] https://crrev.com/6c59727f79b04675fdc9dc663fa36d16b23535b4/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc
,
Jan 20 2017
The test flake was the result of changing when history is destroyed. Gab reverted the patch, so that shutdown order is now as it was and the test has been reenabled. In order for gab to land his patch again he'll have to fix the shutdown ordering. I'm passing to him.
,
Jan 25 2017
,
Jun 13 2017
Issue 719326 has been merged into this issue.
,
Nov 3 2017
sky, fdoray: Do we have a plan for how to clean up the BookmarkModel/HistoryService/HistoryBackend lifetime management to interoperate correctly? Reading through the various bits of implementation, the problem seems to be that the BookmarkModel is a strange mixture of UI-thread APIs (e.g. root_node, other_node, etc getters, Remove, Move & Copy APIs) and multi-thread APIs (e.g. HasBookmarks()). The multi-threaded APIs used by callers like HistoryBackendClient seem to be read-only (e.g. IsBookmarked, HasBookmarks, GetBookmarks) ones used to query the |nodes_ordered_by_url_set_|, so I think we could improve things, without boiling the ocean, by: - Moving the set & lock out to a ref-counted BookmarkModel::Data container. The container would provide the read-only APIs to callers, and private APIs for the BookmarkModel to use to modify the ordered-by-URL set. - Fixing the BookmarkModel to be sequence-affine (i.e. non-thread-safe). - Updating callers to be passed a BookmarkModel::Data reference rather than a bare pointer to the BookmarkModel itself. WDYT?
,
Nov 3 2017
,
Nov 3 2017
wez, I believe what you've outlined will work.
,
Jan 23 2018
This seems to be still flaky: <https://uberchromegw.corp.google.com/i/chromium.win/builders/Win%207%20Tests%20x64%20%281%29/builds/33763>
,
Feb 20 2018
fdoray@, sky@: This is apparently blocking progress on resolving a long standing crasher, Bug 477975. Can this be prioritized for M66, or M67 at the latest?
,
Feb 20 2018
,
Mar 23 2018
Ping for response to #24
,
Mar 27 2018
fdoray: Any updates here? This is still on stability sheriff queue so you'll keep getting pinged while it remains open. Are you still the right owner for this or is there someone better?
,
Mar 31 2018
Re #23: Looking at the flake mentioned in #23, that appears to be issue 751031, so I've mentioned the flake on that bug. sky, fdoray: Ideally we really want to remove the BlockTillLoaded() function, replacing it with a proper asynchronous on-loaded notification that the ChromeHistoryBackendClient can act on. My proposal in #20 is only helpful if the |loaded_signal_| is held by the ref-counted data object, so that it is still live when the ChromeHistoryBackendClient calls BlockTillLoaded() - IIUC the WaitForSingleObject() is failing when re redirect to TaskScheduler because BlockTillLoaded() ends up being called after the BookmarksModel::loaded_signal_ handle has been closed.
,
Mar 31 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/50f4dddc5c8d695a2a0487f84a6c792e44cf5244 commit 50f4dddc5c8d695a2a0487f84a6c792e44cf5244 Author: Wez <wez@chromium.org> Date: Sat Mar 31 04:03:18 2018 Add a DPCHECK() to WaitableEvent::Wait(), to GetLastError on failure. The existing DCHECK_EQ(result, ..) allows us to detect conditions other than wait completion, but doesn't provide any detail in case of a WAIT_FAILED result. Bug: 680698 Change-Id: Icfcda8d7f7a1664e0f966cf3774dd6aa6b5c1b13 Reviewed-on: https://chromium-review.googlesource.com/989309 Reviewed-by: Mark Mentovai <mark@chromium.org> Commit-Queue: Wez <wez@chromium.org> Cr-Commit-Position: refs/heads/master@{#547373} [modify] https://crrev.com/50f4dddc5c8d695a2a0487f84a6c792e44cf5244/base/synchronization/waitable_event_win.cc
,
Apr 11 2018
Not sure the stability sheriff can add much value to this specific issue, and this is unlikely to land in M66. I will note in Bug 477975 that there are specific steps here (C#20) that could help resolve it.
,
Apr 11 2018
,
Apr 11 2018
I outlined in comment 3 why we can't move to an async loading model. As Wez mentions in #28 his proposal doesn't really fix the issue because the core issue is that history may end up relying on bookmarks to load, so that if that load doesn't complete before history tries to shutdown, Chrome deadlocks. I think history has to be responsible for loading bookmarks. Similar to what Wez outlines in #28 I think we should do the following: . refactor calls out of BookmarkModel that mutate urls (and lock) into a standalone class called BookmarkModelData. . Add an interface, perhaps named HistoryBookmarkModel (bad name, needs a better one) that contains *only* the functions needed by history. BookmarkModelData implements this interface. . BookmarkModelData is ref counted. . Make history load BookmarkModelData. . Make history pass BookmarkModelData to main thread and BookmarkModel. HistoryBackend could trigger the loading of the bookmarks on a separate thread, and would only need to block if gets to a point where it requires the data. Assuming we don't shutdown load thread before the history thread, we should be good. Given this seems to be blocking a bunch of stuff, I'll see if I can find time for this soonish.
,
Apr 12 2018
,
Apr 16 2018
For clarity there is no deadlock. The issue is that HistoryBackend has to outlive BookmarkModel, which means we need ~HistoryService to block until the History thread has been shutdown. We would like ~HistoryService not to rely on that. As to blocking in ChromeHistoryBackend, that is necessary at times, and there is no way around that.
,
Apr 25 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8aa6d5760c107e45e43f525132d64fff1cdfffac commit 8aa6d5760c107e45e43f525132d64fff1cdfffac Author: Scott Violet <sky@chromium.org> Date: Wed Apr 25 15:46:21 2018 bookmarks: refactors BookmarkModel::URLAndTitle into its own struct BUG= 680698 TEST=this is a straight rename, covered by tests Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: Ibada505d1a47feca32ef2c5b96883e35fc241594 Reviewed-on: https://chromium-review.googlesource.com/1011842 Commit-Queue: Scott Violet <sky@chromium.org> Reviewed-by: Michael Wasserman <msw@chromium.org> Cr-Commit-Position: refs/heads/master@{#553573} [modify] https://crrev.com/8aa6d5760c107e45e43f525132d64fff1cdfffac/chrome/browser/android/history_report/data_provider.cc [modify] https://crrev.com/8aa6d5760c107e45e43f525132d64fff1cdfffac/chrome/browser/android/history_report/delta_file_commons.cc [modify] https://crrev.com/8aa6d5760c107e45e43f525132d64fff1cdfffac/chrome/browser/android/history_report/delta_file_commons.h [modify] https://crrev.com/8aa6d5760c107e45e43f525132d64fff1cdfffac/chrome/browser/android/history_report/delta_file_commons_unittest.cc [modify] https://crrev.com/8aa6d5760c107e45e43f525132d64fff1cdfffac/chrome/browser/engagement/important_sites_util.cc [modify] https://crrev.com/8aa6d5760c107e45e43f525132d64fff1cdfffac/chrome/browser/history/android/bookmark_model_sql_handler_unittest.cc [modify] https://crrev.com/8aa6d5760c107e45e43f525132d64fff1cdfffac/chrome/browser/history/chrome_history_backend_client.cc [modify] https://crrev.com/8aa6d5760c107e45e43f525132d64fff1cdfffac/chrome/browser/importer/profile_writer_unittest.cc [modify] https://crrev.com/8aa6d5760c107e45e43f525132d64fff1cdfffac/chrome/browser/sync/test/integration/single_client_bookmarks_sync_test.cc [modify] https://crrev.com/8aa6d5760c107e45e43f525132d64fff1cdfffac/chrome/browser/ui/bookmarks/bookmark_browsertest.cc [modify] https://crrev.com/8aa6d5760c107e45e43f525132d64fff1cdfffac/components/bookmarks/browser/BUILD.gn [modify] https://crrev.com/8aa6d5760c107e45e43f525132d64fff1cdfffac/components/bookmarks/browser/bookmark_model.cc [modify] https://crrev.com/8aa6d5760c107e45e43f525132d64fff1cdfffac/components/bookmarks/browser/bookmark_model.h [modify] https://crrev.com/8aa6d5760c107e45e43f525132d64fff1cdfffac/components/bookmarks/browser/bookmark_model_unittest.cc [add] https://crrev.com/8aa6d5760c107e45e43f525132d64fff1cdfffac/components/bookmarks/browser/url_and_title.h [modify] https://crrev.com/8aa6d5760c107e45e43f525132d64fff1cdfffac/components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc [modify] https://crrev.com/8aa6d5760c107e45e43f525132d64fff1cdfffac/ios/chrome/browser/history/history_backend_client_impl.cc [modify] https://crrev.com/8aa6d5760c107e45e43f525132d64fff1cdfffac/ios/chrome/browser/ui/external_file_remover_impl.mm
,
May 3 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4f6f43dc5122f5404a9b7b4d696251eada4d4627 commit 4f6f43dc5122f5404a9b7b4d696251eada4d4627 Author: Scott Violet <sky@chromium.org> Date: Thu May 03 16:25:40 2018 bookmarks: adds public list to bookmarks BUILD.gn There are no new files, this just splits sources into public and sources. This is in preparation to adding some implementation specific files. BUG= 680698 TEST=build file change only Change-Id: I198f7961094909ea9015719efb4ec563b83abb7d Reviewed-on: https://chromium-review.googlesource.com/1041526 Reviewed-by: Jay Civelli <jcivelli@chromium.org> Commit-Queue: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#555761} [modify] https://crrev.com/4f6f43dc5122f5404a9b7b4d696251eada4d4627/components/bookmarks/browser/BUILD.gn
,
May 3 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3c9140062c02d06a8f3cd7b9efa8e3efb7b64f8a commit 3c9140062c02d06a8f3cd7b9efa8e3efb7b64f8a Author: Scott Violet <sky@chromium.org> Date: Thu May 03 18:23:05 2018 bookmarks: makes BookmarkModel set the NodeSorter when loading is complete This is a precursor to having the loading parts of bookmarks create the TitledUrlIndex. This should result in no change as the index is only used once loaded. BUG= 680698 TEST=covered by existing tests Change-Id: Ia059a26509f09472021bcfb31f24e9dcccd07d59 Reviewed-on: https://chromium-review.googlesource.com/1041528 Commit-Queue: Scott Violet <sky@chromium.org> Reviewed-by: Jay Civelli <jcivelli@chromium.org> Cr-Commit-Position: refs/heads/master@{#555817} [modify] https://crrev.com/3c9140062c02d06a8f3cd7b9efa8e3efb7b64f8a/components/bookmarks/browser/bookmark_model.cc [modify] https://crrev.com/3c9140062c02d06a8f3cd7b9efa8e3efb7b64f8a/components/bookmarks/browser/bookmark_storage.cc [modify] https://crrev.com/3c9140062c02d06a8f3cd7b9efa8e3efb7b64f8a/components/bookmarks/browser/bookmark_storage.h [modify] https://crrev.com/3c9140062c02d06a8f3cd7b9efa8e3efb7b64f8a/components/bookmarks/browser/titled_url_index.cc [modify] https://crrev.com/3c9140062c02d06a8f3cd7b9efa8e3efb7b64f8a/components/bookmarks/browser/titled_url_index.h
,
May 3 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e349e963a7d1a4d8674673fdbd8d174360b9e76a commit e349e963a7d1a4d8674673fdbd8d174360b9e76a Author: Scott Violet <sky@chromium.org> Date: Thu May 03 20:45:23 2018 bookmarks: moves creation of various state to BookmarkLoadDetails In particular the permenent node creation moves to BookmarkLoadDetails, and LoadBookmarks() becomes a bare function. BUG= 680698 TEST=covered by existing tests Change-Id: Ifdc7ef84ee73ac7b59c122976a37cb85d5d27b4f Reviewed-on: https://chromium-review.googlesource.com/1041575 Commit-Queue: Scott Violet <sky@chromium.org> Reviewed-by: Jay Civelli <jcivelli@chromium.org> Cr-Commit-Position: refs/heads/master@{#555859} [modify] https://crrev.com/e349e963a7d1a4d8674673fdbd8d174360b9e76a/components/bookmarks/browser/bookmark_model.cc [modify] https://crrev.com/e349e963a7d1a4d8674673fdbd8d174360b9e76a/components/bookmarks/browser/bookmark_model.h [modify] https://crrev.com/e349e963a7d1a4d8674673fdbd8d174360b9e76a/components/bookmarks/browser/bookmark_node.cc [modify] https://crrev.com/e349e963a7d1a4d8674673fdbd8d174360b9e76a/components/bookmarks/browser/bookmark_node.h [modify] https://crrev.com/e349e963a7d1a4d8674673fdbd8d174360b9e76a/components/bookmarks/browser/bookmark_storage.cc [modify] https://crrev.com/e349e963a7d1a4d8674673fdbd8d174360b9e76a/components/bookmarks/browser/bookmark_storage.h [modify] https://crrev.com/e349e963a7d1a4d8674673fdbd8d174360b9e76a/components/bookmarks/test/test_bookmark_client.cc
,
May 7 2018
Detected 3 new flakes for test/step "OmniboxViewTest.DeleteItem". To see the actual flakes, please visit https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyJQsSBUZsYWtlIhpPbW5pYm94Vmlld1Rlc3QuRGVsZXRlSXRlbQw. This message was posted automatically by the chromium-try-flakes app. Since flakiness is ongoing, the issue was moved back into Sheriff Bug Queue (unless already there).
,
May 8 2018
Removing the sheriff label since this is actively being working on.
,
May 11 2018
The above r555859 seems a likely cause of bug 841375 as per bisect, PTAL.
,
May 17 2018
Detected 3 new flakes for test/step "OmniboxViewTest.DeleteItem". To see the actual flakes, please visit https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyJQsSBUZsYWtlIhpPbW5pYm94Vmlld1Rlc3QuRGVsZXRlSXRlbQw. This message was posted automatically by the chromium-try-flakes app. Since flakiness is ongoing, the issue was moved back into Sheriff Bug Queue (unless already there).
,
May 17 2018
Bug assigned and actively being worked on, removing Sheriff label.
,
May 23 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/82ef0faa0619203d30f686226dd17aa3f0b06bad commit 82ef0faa0619203d30f686226dd17aa3f0b06bad Author: Scott Violet <sky@chromium.org> Date: Wed May 23 18:23:39 2018 bookmarks: creates UrlIndex UrlIndex owns all the nodes as well as the mapping from node to url. This is the functionality used by history. BUG= 680698 TEST=covered by existing tests Change-Id: I3540e676fd23336dec1c54fa5790a9563823b4b7 Reviewed-on: https://chromium-review.googlesource.com/1069946 Commit-Queue: Scott Violet <sky@chromium.org> Reviewed-by: Jay Civelli <jcivelli@chromium.org> Cr-Commit-Position: refs/heads/master@{#561165} [modify] https://crrev.com/82ef0faa0619203d30f686226dd17aa3f0b06bad/components/bookmarks/browser/BUILD.gn [modify] https://crrev.com/82ef0faa0619203d30f686226dd17aa3f0b06bad/components/bookmarks/browser/bookmark_model.cc [modify] https://crrev.com/82ef0faa0619203d30f686226dd17aa3f0b06bad/components/bookmarks/browser/bookmark_model.h [modify] https://crrev.com/82ef0faa0619203d30f686226dd17aa3f0b06bad/components/bookmarks/browser/bookmark_storage.cc [modify] https://crrev.com/82ef0faa0619203d30f686226dd17aa3f0b06bad/components/bookmarks/browser/bookmark_storage.h [add] https://crrev.com/82ef0faa0619203d30f686226dd17aa3f0b06bad/components/bookmarks/browser/url_index.cc [add] https://crrev.com/82ef0faa0619203d30f686226dd17aa3f0b06bad/components/bookmarks/browser/url_index.h [modify] https://crrev.com/82ef0faa0619203d30f686226dd17aa3f0b06bad/components/bookmarks/test/test_bookmark_client.cc
,
May 31 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a772b27dd05ef9e6589d134b73a7776fb54708d2 commit a772b27dd05ef9e6589d134b73a7776fb54708d2 Author: Scott Violet <sky@chromium.org> Date: Thu May 31 23:23:21 2018 bookmarks: creates interface for functions used by history This adds the HistoryBookmarkModel interface, which contains the functions used by history. This interfaces is RefCountedThreadSafe as history holds a reference to it from a background thread. The concrete implementation of this is UrlIndex. UrlIndex may outlive BookmarkModel. BookmarkModel::BlockUntilLoaded moves to a separate class, ModelLoader. ModelLoader may also outlive BookmarkModel. BookmarkModel currently creates and holds a reference the ModelLoader. I may change that, but this is a good point to land code. BUG= 680698 TEST=covered by existing tests Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: Id5d0bade960eaa76668213b9d221eba0a688b16d Reviewed-on: https://chromium-review.googlesource.com/1039151 Commit-Queue: Scott Violet <sky@chromium.org> Reviewed-by: Jay Civelli <jcivelli@chromium.org> Cr-Commit-Position: refs/heads/master@{#563426} [modify] https://crrev.com/a772b27dd05ef9e6589d134b73a7776fb54708d2/chrome/browser/android/history_report/data_observer.cc [modify] https://crrev.com/a772b27dd05ef9e6589d134b73a7776fb54708d2/chrome/browser/android/history_report/data_observer.h [modify] https://crrev.com/a772b27dd05ef9e6589d134b73a7776fb54708d2/chrome/browser/android/history_report/data_provider.cc [modify] https://crrev.com/a772b27dd05ef9e6589d134b73a7776fb54708d2/chrome/browser/android/provider/bookmark_model_observer_task.cc [modify] https://crrev.com/a772b27dd05ef9e6589d134b73a7776fb54708d2/chrome/browser/history/chrome_history_backend_client.cc [modify] https://crrev.com/a772b27dd05ef9e6589d134b73a7776fb54708d2/chrome/browser/history/chrome_history_backend_client.h [modify] https://crrev.com/a772b27dd05ef9e6589d134b73a7776fb54708d2/chrome/browser/history/chrome_history_client.cc [modify] https://crrev.com/a772b27dd05ef9e6589d134b73a7776fb54708d2/chrome/browser/history/chrome_history_client.h [modify] https://crrev.com/a772b27dd05ef9e6589d134b73a7776fb54708d2/components/bookmarks/browser/BUILD.gn [modify] https://crrev.com/a772b27dd05ef9e6589d134b73a7776fb54708d2/components/bookmarks/browser/bookmark_model.cc [modify] https://crrev.com/a772b27dd05ef9e6589d134b73a7776fb54708d2/components/bookmarks/browser/bookmark_model.h [modify] https://crrev.com/a772b27dd05ef9e6589d134b73a7776fb54708d2/components/bookmarks/browser/bookmark_storage.cc [modify] https://crrev.com/a772b27dd05ef9e6589d134b73a7776fb54708d2/components/bookmarks/browser/bookmark_storage.h [add] https://crrev.com/a772b27dd05ef9e6589d134b73a7776fb54708d2/components/bookmarks/browser/history_bookmark_model.h [add] https://crrev.com/a772b27dd05ef9e6589d134b73a7776fb54708d2/components/bookmarks/browser/model_loader.cc [add] https://crrev.com/a772b27dd05ef9e6589d134b73a7776fb54708d2/components/bookmarks/browser/model_loader.h [modify] https://crrev.com/a772b27dd05ef9e6589d134b73a7776fb54708d2/components/bookmarks/browser/url_index.cc [modify] https://crrev.com/a772b27dd05ef9e6589d134b73a7776fb54708d2/components/bookmarks/browser/url_index.h [modify] https://crrev.com/a772b27dd05ef9e6589d134b73a7776fb54708d2/ios/chrome/browser/history/history_backend_client_impl.cc [modify] https://crrev.com/a772b27dd05ef9e6589d134b73a7776fb54708d2/ios/chrome/browser/history/history_backend_client_impl.h [modify] https://crrev.com/a772b27dd05ef9e6589d134b73a7776fb54708d2/ios/chrome/browser/history/history_client_impl.cc [modify] https://crrev.com/a772b27dd05ef9e6589d134b73a7776fb54708d2/ios/chrome/browser/history/history_client_impl.h
,
Jun 1 2018
This now done. The HistoryService can outlive the BookmarkModel. |
||||||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||||||
Comment 1 by xlai@chromium.org
, Jan 12 2017Labels: -Sheriff-Chromium
Owner: pkasting@chromium.org
Status: Assigned (was: Untriaged)