Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2538763002:
Data from the autocomplete predictor wasn't deleted immediately when deleting browsing history. (Closed)
DescriptionData from the autocomplete predictor wasn't deleted immediately when deleting browsing history. The cleanup method DeleteOldEntries() only deleted the data on next start because the corresponding history entries didn't exist anymore.
The problem is that the AutocompleteActionPredictor unsubscribes from HistoryService events and doesn't get notified about history deletion.
The deletion method was added in this cl: http://crrev.com/773103004 but it was never called as OnHistoryServiceLoaded() removed the subscription after initialization.
The same issue exists and is fixed in the ResourcePrefetchPredictor code.
BUG=668715
Committed: https://crrev.com/6a66fa4f05011ff93f2f4f5466662af9c7b16b9a
Cr-Commit-Position: refs/heads/master@{#439077}
Patch Set 1 #Patch Set 2 : fix prefetch predictor as well #
Total comments: 14
Patch Set 3 : remove boolean return declaration #Patch Set 4 : change initialization check to DCHECK #Patch Set 5 : fix destruction order #
Total comments: 6
Patch Set 6 : assume history_service always exist to ensure we definitly delete data when requested #
Total comments: 2
Patch Set 7 : revert dcheck(history_service) add dcheck(initialized_) #
Total comments: 1
Patch Set 8 : move TryDeleteOldEntries in condition #
Messages
Total messages: 44 (30 generated)
The CQ bit was checked by dullweber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Data from the autocomplete predictor wasn't deleted immediately when deleting browsing history. The problem is that the AutocompleteActionPredictor unsubscribes from HistoryService events and doesn't get notified about history deletion. BUG=668715 ========== to ========== Data from the autocomplete predictor wasn't deleted immediately when deleting browsing history. The problem is that the AutocompleteActionPredictor unsubscribes from HistoryService events and doesn't get notified about history deletion. The deletion method was added in this cl: https://codereview.chromium.org/773103004 but it was probably never called as OnHistoryServiceLoaded() removed the subscription. The same issue exists in the ResourcePrefetchPredictor code. BUG=668715 ==========
Description was changed from ========== Data from the autocomplete predictor wasn't deleted immediately when deleting browsing history. The problem is that the AutocompleteActionPredictor unsubscribes from HistoryService events and doesn't get notified about history deletion. The deletion method was added in this cl: https://codereview.chromium.org/773103004 but it was probably never called as OnHistoryServiceLoaded() removed the subscription. The same issue exists in the ResourcePrefetchPredictor code. BUG=668715 ========== to ========== Data from the autocomplete predictor wasn't deleted immediately when deleting browsing history. The cleanup method DeleteOldEntries() only deleted the data on next start because the corresponding history entries didn't exist anymore. The problem is that the AutocompleteActionPredictor unsubscribes from HistoryService events and doesn't get notified about history deletion. The deletion method was added in this cl: http://crrev.com/773103004 but it was probably never called as OnHistoryServiceLoaded() removed the subscription after initialization. The same issue exists in the ResourcePrefetchPredictor code. BUG=668715 ==========
Description was changed from ========== Data from the autocomplete predictor wasn't deleted immediately when deleting browsing history. The cleanup method DeleteOldEntries() only deleted the data on next start because the corresponding history entries didn't exist anymore. The problem is that the AutocompleteActionPredictor unsubscribes from HistoryService events and doesn't get notified about history deletion. The deletion method was added in this cl: http://crrev.com/773103004 but it was probably never called as OnHistoryServiceLoaded() removed the subscription after initialization. The same issue exists in the ResourcePrefetchPredictor code. BUG=668715 ========== to ========== Data from the autocomplete predictor wasn't deleted immediately when deleting browsing history. The cleanup method DeleteOldEntries() only deleted the data on next start because the corresponding history entries didn't exist anymore. The problem is that the AutocompleteActionPredictor unsubscribes from HistoryService events and doesn't get notified about history deletion. The deletion method was added in this cl: http://crrev.com/773103004 but it was probably never called as OnHistoryServiceLoaded() removed the subscription after initialization. The same issue exists and is fixed in the ResourcePrefetchPredictor code. BUG=668715 ==========
Description was changed from ========== Data from the autocomplete predictor wasn't deleted immediately when deleting browsing history. The cleanup method DeleteOldEntries() only deleted the data on next start because the corresponding history entries didn't exist anymore. The problem is that the AutocompleteActionPredictor unsubscribes from HistoryService events and doesn't get notified about history deletion. The deletion method was added in this cl: http://crrev.com/773103004 but it was probably never called as OnHistoryServiceLoaded() removed the subscription after initialization. The same issue exists and is fixed in the ResourcePrefetchPredictor code. BUG=668715 ========== to ========== Data from the autocomplete predictor wasn't deleted immediately when deleting browsing history. The cleanup method DeleteOldEntries() only deleted the data on next start because the corresponding history entries didn't exist anymore. The problem is that the AutocompleteActionPredictor unsubscribes from HistoryService events and doesn't get notified about history deletion. The deletion method was added in this cl: http://crrev.com/773103004 but it was never called as OnHistoryServiceLoaded() removed the subscription after initialization. The same issue exists and is fixed in the ResourcePrefetchPredictor code. BUG=668715 ==========
dullweber@chromium.org changed reviewers: + zhenw@chromium.org
Zhen, please review these changes
zhenw@chromium.org changed reviewers: + lizeb@chromium.org, pasko@chromium.org
+pasko, lizeb, can you review this as you guys worked on this lately?
https://codereview.chromium.org/2538763002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): https://codereview.chromium.org/2538763002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/autocomplete_action_predictor.cc:445: TryDeleteOldEntries(history_service); after this change the return value is never checked for this function, let's make it stop returning it? https://codereview.chromium.org/2538763002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/autocomplete_action_predictor.cc:447: history_service_observer_.Add(history_service); what if the history service is not ready by the time AutocompleteActionPredictor attempts to CreateCaches()? https://codereview.chromium.org/2538763002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/autocomplete_action_predictor.cc:570: return; In other words: if we've got a request to delete URLs, but have not gotten to be initialized, we would forget about deleting. This looks wrong to me. https://codereview.chromium.org/2538763002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/autocomplete_action_predictor.cc:580: if (!initialized_) would it break something if we try to delete old entries after being initialized? https://codereview.chromium.org/2538763002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2538763002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:1147: OnHistoryAndCacheLoaded(); Would it be possible for HistoryServiceFactory::GetForProfile to return nullptr while we were initializing? This would skip calling OnHistoryAndCacheLoaded() entirely, right? Am I missing something? Can the async tasks be simplified to only record the flag that they were loaded and call the continuation that does the job only if all init bits are set? I believe that would simplify the spaghetti a bit.
zhenw@chromium.org changed reviewers: - zhenw@chromium.org
https://codereview.chromium.org/2538763002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): https://codereview.chromium.org/2538763002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/autocomplete_action_predictor.cc:445: TryDeleteOldEntries(history_service); On 2016/11/29 18:01:00, pasko wrote: > after this change the return value is never checked for this function, let's > make it stop returning it? Done. https://codereview.chromium.org/2538763002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/autocomplete_action_predictor.cc:447: history_service_observer_.Add(history_service); On 2016/11/29 18:01:00, pasko wrote: > what if the history service is not ready by the time AutocompleteActionPredictor > attempts to CreateCaches()? I think I handled the following cases: 1. history_service and InMemoryDatabase are loaded. Then TryDeleteOldEntries from line 445 will succeed and OnHistoryServiceLoaded is never called because we missed it. 2. InMemoryDatabase is not yet ready. In this case TryDeleteOldEntries() fails but we get the OnHistoryServiceLoaded() event and call TryDeleteOldEntries() from there again. There are two other cases I can think of: 3. HistoryServiceFactory::GetForProfile() returns nullptr. I don't think this can actually happen except when history is disabled? The factory method calls GetServiceForBrowserContext() with create = true https://cs.chromium.org/chromium/src/chrome/browser/history/history_service_f... I don't think we could do anything if history is disabled. 4. InMemoryDatabase is loaded but we didn't get OnHistoryServiceLoaded() yet because of some weird concurrency issue. I have no idea if that is possible. But if it happens the if(!initialized_) prevents a second call to TryDeleteOldEntries() https://codereview.chromium.org/2538763002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/autocomplete_action_predictor.cc:570: return; On 2016/11/29 18:01:00, pasko wrote: > In other words: if we've got a request to delete URLs, but have not gotten to be > initialized, we would forget about deleting. This looks wrong to me. That's true. This is normally called when the user selects to delete the history. The history_service should be loaded by then. Are there other cases where it is needed? I could add a flag to remember the deletion and call it again in OnHistoryServiceLoaded. I'm not sure if that is neccessary, what do you think about this? https://codereview.chromium.org/2538763002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/autocomplete_action_predictor.cc:580: if (!initialized_) On 2016/11/29 18:01:00, pasko wrote: > would it break something if we try to delete old entries after being > initialized? I don't think anything would break but TryDeleteOldEntries() has a DCHECK() for !initialized_. I explained the rare case where this could be needed above. https://codereview.chromium.org/2538763002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2538763002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:1147: OnHistoryAndCacheLoaded(); On 2016/11/29 18:01:00, pasko wrote: > Would it be possible for HistoryServiceFactory::GetForProfile to return nullptr > while we were initializing? This would skip calling OnHistoryAndCacheLoaded() > entirely, right? Am I missing something? > > Can the async tasks be simplified to only record the flag that they were loaded > and call the continuation that does the job only if all init bits are set? I > believe that would simplify the spaghetti a bit. I added this check for the same rare reason as for initialized_ in the other file.
thank you for responses. I still think we can make the code more straightforward and not giving up too early on deleting :) Asking more questions below to figure out if it is possible and actually the goal. https://codereview.chromium.org/2538763002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): https://codereview.chromium.org/2538763002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/autocomplete_action_predictor.cc:447: history_service_observer_.Add(history_service); On 2016/11/30 11:49:21, dullweber wrote: > On 2016/11/29 18:01:00, pasko wrote: > > what if the history service is not ready by the time > AutocompleteActionPredictor > > attempts to CreateCaches()? > > I think I handled the following cases: > 1. history_service and InMemoryDatabase are loaded. Then TryDeleteOldEntries > from line 445 will succeed and OnHistoryServiceLoaded is never called because we > missed it. > 2. InMemoryDatabase is not yet ready. In this case TryDeleteOldEntries() fails > but we get the OnHistoryServiceLoaded() event and call TryDeleteOldEntries() > from there again. > > There are two other cases I can think of: > 3. HistoryServiceFactory::GetForProfile() returns nullptr. I don't think this > can actually happen except when history is disabled? The factory method calls > GetServiceForBrowserContext() with create = true > https://cs.chromium.org/chromium/src/chrome/browser/history/history_service_f... > I don't think we could do anything if history is disabled. > 4. InMemoryDatabase is loaded but we didn't get OnHistoryServiceLoaded() yet > because of some weird concurrency issue. I have no idea if that is possible. But > if it happens the if(!initialized_) prevents a second call to > TryDeleteOldEntries() I think calling TryDeleteOldEntries twice is not such a big problem (modulo removable DCHECKs), but failing to add a history observer when the history service loads slowly is a big problem. Even if something in code currently guarantees that this does not happen, that guarantee may go away subtly without anyone noticing. Can we add the observer in OnHistoryServiceLoaded()? I think the current code did not do it only because it did not want to observe history all the time, but this is exactly your proposal to always listen to history, right? Let me know if I misunderstood. https://codereview.chromium.org/2538763002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/autocomplete_action_predictor.cc:570: return; On 2016/11/30 11:49:21, dullweber wrote: > On 2016/11/29 18:01:00, pasko wrote: > > In other words: if we've got a request to delete URLs, but have not gotten to > be > > initialized, we would forget about deleting. This looks wrong to me. > > That's true. This is normally called when the user selects to delete the > history. The history_service should be loaded by then. ah, right, we cannot get into OnURLsDeleted() without adding the observer, and that one is gated behind the service being loaded. Then this should be a DCHECK, right?
The CQ bit was checked by dullweber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dullweber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2538763002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): https://codereview.chromium.org/2538763002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/autocomplete_action_predictor.cc:447: history_service_observer_.Add(history_service); On 2016/12/01 17:26:41, pasko wrote: > On 2016/11/30 11:49:21, dullweber wrote: > > On 2016/11/29 18:01:00, pasko wrote: > > > what if the history service is not ready by the time > > AutocompleteActionPredictor > > > attempts to CreateCaches()? > > > > I think I handled the following cases: > > 1. history_service and InMemoryDatabase are loaded. Then TryDeleteOldEntries > > from line 445 will succeed and OnHistoryServiceLoaded is never called because > we > > missed it. > > 2. InMemoryDatabase is not yet ready. In this case TryDeleteOldEntries() fails > > but we get the OnHistoryServiceLoaded() event and call TryDeleteOldEntries() > > from there again. > > > > There are two other cases I can think of: > > 3. HistoryServiceFactory::GetForProfile() returns nullptr. I don't think this > > can actually happen except when history is disabled? The factory method calls > > GetServiceForBrowserContext() with create = true > > > https://cs.chromium.org/chromium/src/chrome/browser/history/history_service_f... > > I don't think we could do anything if history is disabled. > > 4. InMemoryDatabase is loaded but we didn't get OnHistoryServiceLoaded() yet > > because of some weird concurrency issue. I have no idea if that is possible. > But > > if it happens the if(!initialized_) prevents a second call to > > TryDeleteOldEntries() > > I think calling TryDeleteOldEntries twice is not such a big problem (modulo > removable DCHECKs), but failing to add a history observer when the history > service loads slowly is a big problem. Even if something in code currently > guarantees that this does not happen, that guarantee may go away subtly without > anyone noticing. > > Can we add the observer in OnHistoryServiceLoaded()? I think the current code > did not do it only because it did not want to observe history all the time, but > this is exactly your proposal to always listen to history, right? Let me know if > I misunderstood. OnHistoryServiceLoaded() and OnURLsDeleted() are both callbacks from the history_service_observer_, which is registered on a history_service. In order to get OnHistoryServiceLoaded() we already need to be registered to it. OnHistoryServiceLoaded() indicates that the HistoryService is loaded completely.(https://cs.chromium.org/chromium/src/components/history/core/browser/history_service_observer.h?l=56) If HistoryServiceFactory::GetForProfile() would be changed to not return a HistoryService immediately the whole system would need to be changed anyway like HistoryServiceFactory returning something like a Promise. I hope it is understandable, why I think there is no safer way to register to the history :D https://codereview.chromium.org/2538763002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/autocomplete_action_predictor.cc:570: return; On 2016/12/01 17:26:41, pasko wrote: > On 2016/11/30 11:49:21, dullweber wrote: > > On 2016/11/29 18:01:00, pasko wrote: > > > In other words: if we've got a request to delete URLs, but have not gotten > to > > be > > > initialized, we would forget about deleting. This looks wrong to me. > > > > That's true. This is normally called when the user selects to delete the > > history. The history_service should be loaded by then. > > ah, right, we cannot get into OnURLsDeleted() without adding the observer, and > that one is gated behind the service being loaded. Then this should be a DCHECK, > right? That should be right. I changed it to a DCHECK here and in the other predictor.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I was OOO most of previous week, only had time to look at the change today. Sorry. Thank you for looking into details. I would like more conditions to be converted to DCHECKs (or would like to be convinced that they should not be converted). https://codereview.chromium.org/2538763002/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2538763002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:1145: if (initialization_state_ == INITIALIZING) { DCHECK instead? I cannot see how we can end up with != INITIALIZED here. https://codereview.chromium.org/2538763002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:1155: if (!history_service) GetForProfile() with EXPLICIT_ACCESS never returns nullptr, right? Can we use DCHECK here and in similar cases of the other predictor? The codepaths with some initialization and !history_service look privacy-sensitive. It would be better to make sure they are impossible.
The CQ bit was checked by dullweber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
No problem. I removed the conditions and now always assume that a valid HistoryService is returned from HistoryServiceFactory. https://codereview.chromium.org/2538763002/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2538763002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:1145: if (initialization_state_ == INITIALIZING) { On 2016/12/12 19:15:43, pasko wrote: > DCHECK instead? > > I cannot see how we can end up with != INITIALIZED here. Ok, I removed it. The DCHECK() in OnHistoryAndCacheLoaded() will also tell us if it ever happens. https://codereview.chromium.org/2538763002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:1155: if (!history_service) On 2016/12/12 19:15:43, pasko wrote: > GetForProfile() with EXPLICIT_ACCESS never returns nullptr, right? Can we use > DCHECK here and in similar cases of the other predictor? > > The codepaths with some initialization and !history_service look > privacy-sensitive. It would be better to make sure they are impossible. That's right. I removed the conditions and use a DCHECK instead. I had to fix the AutocompleteActionPredictorUnittest because the TestProfile needs to initialize the history service manually and it was done after the creation of the predictor.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2538763002/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2538763002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:1155: if (!history_service) On 2016/12/13 15:47:35, dullweber wrote: > On 2016/12/12 19:15:43, pasko wrote: > > GetForProfile() with EXPLICIT_ACCESS never returns nullptr, right? Can we use > > DCHECK here and in similar cases of the other predictor? > > > > The codepaths with some initialization and !history_service look > > privacy-sensitive. It would be better to make sure they are impossible. > > That's right. I removed the conditions and use a DCHECK instead. I had to fix > the AutocompleteActionPredictorUnittest because the TestProfile needs to > initialize the history service manually and it was done after the creation of > the predictor. I was wrong here. Sorry. The history service may go away on shutdown, and if we attempt to connect here after that time - boom, UAF (usual caveat: I might be missing some guarantees, though others may be missing them too, so better to be safe). Let's undo all the DCHECK(history_service). Feel free to put a comment somewhere on top saying that the history service can go away at shutdown, so this code should never assume the service exists. https://codereview.chromium.org/2538763002/diff/100001/chrome/browser/predict... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): https://codereview.chromium.org/2538763002/diff/100001/chrome/browser/predict... chrome/browser/predictors/autocomplete_action_predictor.cc:338: if (!initialized_) nit: DCHECK here as well?
The CQ bit was checked by dullweber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2538763002/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2538763002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:1155: if (!history_service) On 2016/12/15 12:57:35, pasko wrote: > On 2016/12/13 15:47:35, dullweber wrote: > > On 2016/12/12 19:15:43, pasko wrote: > > > GetForProfile() with EXPLICIT_ACCESS never returns nullptr, right? Can we > use > > > DCHECK here and in similar cases of the other predictor? > > > > > > The codepaths with some initialization and !history_service look > > > privacy-sensitive. It would be better to make sure they are impossible. > > > > That's right. I removed the conditions and use a DCHECK instead. I had to fix > > the AutocompleteActionPredictorUnittest because the TestProfile needs to > > initialize the history service manually and it was done after the creation of > > the predictor. > > I was wrong here. Sorry. The history service may go away on shutdown, and if we > attempt to connect here after that time - boom, UAF (usual caveat: I might be > missing some guarantees, though others may be missing them too, so better to be > safe). > > Let's undo all the DCHECK(history_service). Feel free to put a comment somewhere > on top saying that the history service can go away at shutdown, so this code > should never assume the service exists. I reverted DCHECK(history_service) but added the DCHECK(initialized_) you mentioned in the other comment. I hope that is what was intended but I think DCHECK(initialized_) is safe :D The failed tests should be fixed due to this revert. https://codereview.chromium.org/2538763002/diff/100001/chrome/browser/predict... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): https://codereview.chromium.org/2538763002/diff/100001/chrome/browser/predict... chrome/browser/predictors/autocomplete_action_predictor.cc:338: if (!initialized_) On 2016/12/15 12:57:35, pasko wrote: > nit: DCHECK here as well? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, thank you! https://codereview.chromium.org/2538763002/diff/120001/chrome/browser/predict... File chrome/browser/predictors/autocomplete_action_predictor.cc (right): https://codereview.chromium.org/2538763002/diff/120001/chrome/browser/predict... chrome/browser/predictors/autocomplete_action_predictor.cc:444: if (history_service) nit: extra check for the pointer in TryDeleteOldEntries is fine, but every reader of these lines would have to go there and check whether it is done, which is an expensive seek :) this would look safer and more natural: if (history_service) { TryDeleteOldEntries(history_service); history_service_observer_.Add(history_service); }
The CQ bit was checked by dullweber@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org Link to the patchset: https://codereview.chromium.org/2538763002/#ps140001 (title: "move TryDeleteOldEntries in condition")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481878189655820,
"parent_rev": "0e42eac89a237fcb191f6e3c86b04cdb47df9fc0", "commit_rev":
"6cb8e0e31547df4bacc67e0dbc3ca244dd3cc6a1"}
Message was sent while issue was closed.
Description was changed from ========== Data from the autocomplete predictor wasn't deleted immediately when deleting browsing history. The cleanup method DeleteOldEntries() only deleted the data on next start because the corresponding history entries didn't exist anymore. The problem is that the AutocompleteActionPredictor unsubscribes from HistoryService events and doesn't get notified about history deletion. The deletion method was added in this cl: http://crrev.com/773103004 but it was never called as OnHistoryServiceLoaded() removed the subscription after initialization. The same issue exists and is fixed in the ResourcePrefetchPredictor code. BUG=668715 ========== to ========== Data from the autocomplete predictor wasn't deleted immediately when deleting browsing history. The cleanup method DeleteOldEntries() only deleted the data on next start because the corresponding history entries didn't exist anymore. The problem is that the AutocompleteActionPredictor unsubscribes from HistoryService events and doesn't get notified about history deletion. The deletion method was added in this cl: http://crrev.com/773103004 but it was never called as OnHistoryServiceLoaded() removed the subscription after initialization. The same issue exists and is fixed in the ResourcePrefetchPredictor code. BUG=668715 Review-Url: https://codereview.chromium.org/2538763002 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Data from the autocomplete predictor wasn't deleted immediately when deleting browsing history. The cleanup method DeleteOldEntries() only deleted the data on next start because the corresponding history entries didn't exist anymore. The problem is that the AutocompleteActionPredictor unsubscribes from HistoryService events and doesn't get notified about history deletion. The deletion method was added in this cl: http://crrev.com/773103004 but it was never called as OnHistoryServiceLoaded() removed the subscription after initialization. The same issue exists and is fixed in the ResourcePrefetchPredictor code. BUG=668715 Review-Url: https://codereview.chromium.org/2538763002 ========== to ========== Data from the autocomplete predictor wasn't deleted immediately when deleting browsing history. The cleanup method DeleteOldEntries() only deleted the data on next start because the corresponding history entries didn't exist anymore. The problem is that the AutocompleteActionPredictor unsubscribes from HistoryService events and doesn't get notified about history deletion. The deletion method was added in this cl: http://crrev.com/773103004 but it was never called as OnHistoryServiceLoaded() removed the subscription after initialization. The same issue exists and is fixed in the ResourcePrefetchPredictor code. BUG=668715 Committed: https://crrev.com/6a66fa4f05011ff93f2f4f5466662af9c7b16b9a Cr-Commit-Position: refs/heads/master@{#439077} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/6a66fa4f05011ff93f2f4f5466662af9c7b16b9a Cr-Commit-Position: refs/heads/master@{#439077} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
