Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(405)

Issue 1413373012: Sanitize access to services in HistoryCounter. (Closed)

Created:
5 years, 1 month ago by msramek
Modified:
5 years, 1 month ago
Reviewers:
engedy, Mike West
CC:
chromium-reviews, markusheintz_
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Sanitize access to services in HistoryCounter. HistoryService: - Request it with EXPLICIT_ACCESS. This is valid, because the counters only count data and do not modify them (and they're also only used in the chrome://settings/ menu which cannot be opened in the incognito mode). - Otherwise, the counter would (crash and) fail to count history if prefs::kSavingBrowserHistoryDisabled == true, but deletion would still work, since BrowsingDataRemover also asks for EXPLICIT_ACCESS. ProfileSyncService: - Do not DCHECK if it does not exist, since it is not essential for the counter to run. If the Sync service is not present, we do not get any callbacks from it, and the counter behaves the same way as if history sync was simply disabled. - Otherwise, this will crash if Sync service does not exist, which is a valid circumstance (e.g. when switches::kDisableSync flag is present). BUG=510028, 553640 Committed: https://crrev.com/e42d24457084d609a882d04799fea1a7194ebc4a Cr-Commit-Position: refs/heads/master@{#358819}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+6 lines, -5 lines) Patch
M chrome/browser/browsing_data/history_counter.cc View 2 chunks +6 lines, -5 lines 0 comments Download

Messages

Total messages: 11 (3 generated)
msramek
Hi Balázs, can you please have a look? It turns out I was bold in ...
5 years, 1 month ago (2015-11-10 10:03:48 UTC) #2
engedy
LGTM.
5 years, 1 month ago (2015-11-10 10:36:06 UTC) #3
msramek
Thanks! Mike, can you please review as the owner?
5 years, 1 month ago (2015-11-10 10:45:59 UTC) #5
Mike West
lgtm
5 years, 1 month ago (2015-11-10 10:51:05 UTC) #6
msramek
Thank you! Landing this now.
5 years, 1 month ago (2015-11-10 10:52:43 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413373012/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413373012/1
5 years, 1 month ago (2015-11-10 10:52:52 UTC) #9
commit-bot: I haz the power
Committed patchset #1 (id:1)
5 years, 1 month ago (2015-11-10 11:24:02 UTC) #10
commit-bot: I haz the power
5 years, 1 month ago (2015-11-10 11:24:55 UTC) #11
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/e42d24457084d609a882d04799fea1a7194ebc4a
Cr-Commit-Position: refs/heads/master@{#358819}

Powered by Google App Engine
This is Rietveld 408576698