Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2342673004:
Clear browsing data from ContentSuggestionService only if exists. (Closed)
DescriptionClear browsing data from ContentSuggestionService only if exists.
Currently, we instantiate the service in BrowsingDataRemover even when
it does not exist, yet. This means that we instantiate it on non-android
platforms, in unit-tests, etc.
This CL changes the logic so that the service is accessed only if it has
been created previously. This way, the BrowsingDataRemover respects the
start-up logic that decides whether to create the service or not.
BUG=647222
Committed: https://crrev.com/9d1e504520b774c0a6f24920f730748053c4b2b2
Cr-Commit-Position: refs/heads/master@{#419141}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Marc's comment #Patch Set 3 : Last comment of Bernhard #
Messages
Total messages: 21 (9 generated)
jkrcal@chromium.org changed reviewers: + bauerb@chromium.org
Hi Bernhard, this is another M54 merge candidate. PTAL! A bit more of explanation: If we flip the sign-in-not-required param to true by default (another M54 merge candidate), this means that the ContentSuggestionService is enabled by default, which means that BrowsingDataRemover creates all the providers of ContentSuggestionService when called from its unit-test which means that many unit-tests fail because the non-mocked providers try to do I/O on the unit-test thread :( As the next step, we should mock the service in browsing_data_remover_unittest.cc to ensure test coverage but this is nothing we need to merge to M54.
treib@chromium.org changed reviewers: + treib@chromium.org
https://codereview.chromium.org/2342673004/diff/1/chrome/browser/browsing_dat... File chrome/browser/browsing_data/browsing_data_remover.cc (left): https://codereview.chromium.org/2342673004/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:525: ContentSuggestionsServiceFactory::GetForProfile(profile_); GetForProfile is called in one more place in this file, you should update that too.
Thanks, Marc! Bernhard, PTAL. https://codereview.chromium.org/2342673004/diff/1/chrome/browser/browsing_dat... File chrome/browser/browsing_data/browsing_data_remover.cc (left): https://codereview.chromium.org/2342673004/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:525: ContentSuggestionsServiceFactory::GetForProfile(profile_); On 2016/09/15 12:45:09, Marc Treib wrote: > GetForProfile is called in one more place in this file, you should update that > too. Oh, thanks for spotting that!
On 2016/09/15 12:38:47, jkrcal wrote: > Hi Bernhard, this is another M54 merge candidate. PTAL! What's the impact on M54? Is it only unit tests? > A bit more of explanation: > If we flip the sign-in-not-required param to true by default (another M54 merge > candidate), this means that the ContentSuggestionService is enabled by default, > which means that BrowsingDataRemover creates all the providers of > ContentSuggestionService when called from its unit-test which means that many > unit-tests fail because the non-mocked providers try to do I/O on the unit-test > thread :( What is the unit-test thread? The bug alone is a bit sparse on context. > As the next step, we should mock the service in > browsing_data_remover_unittest.cc to ensure test coverage but this is nothing we > need to merge to M54. https://codereview.chromium.org/2342673004/diff/1/chrome/browser/browsing_dat... File chrome/browser/browsing_data/browsing_data_remover.cc (left): https://codereview.chromium.org/2342673004/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/browsing_data_remover.cc:525: ContentSuggestionsServiceFactory::GetForProfile(profile_); On 2016/09/15 12:53:17, jkrcal wrote: > On 2016/09/15 12:45:09, Marc Treib wrote: > > GetForProfile is called in one more place in this file, you should update that > > too. > > Oh, thanks for spotting that! Should we add a DCHECK that the feature is enabled when constructing the service so that we catch any regressions here?
On 2016/09/15 13:44:46, Bernhard Bauer wrote: > On 2016/09/15 12:38:47, jkrcal wrote: > > Hi Bernhard, this is another M54 merge candidate. PTAL! > > What's the impact on M54? Is it only unit tests? This CL is a prerequisite for CL 2343583002 to pass unit-tests. (we do not need this change on its own in M54, we would like to have the other CL in M54). As these are quite unrelated bugs, I wanted to land them in separate CLs. > > A bit more of explanation: > > If we flip the sign-in-not-required param to true by default (another M54 > merge > > candidate), this means that the ContentSuggestionService is enabled by > default, > > which means that BrowsingDataRemover creates all the providers of > > ContentSuggestionService when called from its unit-test which means that many > > unit-tests fail because the non-mocked providers try to do I/O on the > unit-test > > thread :( > > What is the unit-test thread? The bug alone is a bit sparse on context. I meant the single thread on which the unit-test runs. (Maybe I am wrong with the context, anyway a CHECK fails stating "MessageLoop::TYPE_IO == loop->type()" so I guess some I/O operations get executed on the wrong thread.) I updated the bug. > > As the next step, we should mock the service in > > browsing_data_remover_unittest.cc to ensure test coverage but this is nothing > we > > need to merge to M54. > > https://codereview.chromium.org/2342673004/diff/1/chrome/browser/browsing_dat... > File chrome/browser/browsing_data/browsing_data_remover.cc (left): > > https://codereview.chromium.org/2342673004/diff/1/chrome/browser/browsing_dat... > chrome/browser/browsing_data/browsing_data_remover.cc:525: > ContentSuggestionsServiceFactory::GetForProfile(profile_); > On 2016/09/15 12:53:17, jkrcal wrote: > > On 2016/09/15 12:45:09, Marc Treib wrote: > > > GetForProfile is called in one more place in this file, you should update > that > > > too. > > > > Oh, thanks for spotting that! > > Should we add a DCHECK that the feature is enabled when constructing the service > so that we catch any regressions here? Where do you mean to add the DCHECK? The ContentSuggestionService can get constructed when the feature is off, it is just disabled.
On 2016/09/15 14:30:11, jkrcal wrote: > On 2016/09/15 13:44:46, Bernhard Bauer wrote: > > On 2016/09/15 12:38:47, jkrcal wrote: > > > Hi Bernhard, this is another M54 merge candidate. PTAL! > > > > What's the impact on M54? Is it only unit tests? > > This CL is a prerequisite for CL 2343583002 to pass unit-tests. > (we do not need this change on its own in M54, we would like to have the other > CL in M54). > > As these are quite unrelated bugs, I wanted to land them in separate CLs. OK, but then we don't necessarily need to merge this CL? We don't really run many tests on branches. > > > A bit more of explanation: > > > If we flip the sign-in-not-required param to true by default (another M54 > > merge > > > candidate), this means that the ContentSuggestionService is enabled by > > default, > > > which means that BrowsingDataRemover creates all the providers of > > > ContentSuggestionService when called from its unit-test which means that > many > > > unit-tests fail because the non-mocked providers try to do I/O on the > > unit-test > > > thread :( > > > > What is the unit-test thread? The bug alone is a bit sparse on context. > > I meant the single thread on which the unit-test runs. > (Maybe I am wrong with the context, anyway a CHECK fails stating > "MessageLoop::TYPE_IO == loop->type()" so I guess some I/O operations get > executed on the wrong thread.) > > I updated the bug. OK, but what I was looking for was more like a link to the failing test, with a full stack trace. I've added a comment to the bug. So, TYPE_IO actually means _non-blocking_ I/O, i.e. IPC or network connections (file I/O actually is allowed in a unit test, but non-blocking I/O requires a special type of message loop). It would be interesting to find out what is actually trying to open a network connection here -- probably the SnippetsService trying to fetch from the server? > > > As the next step, we should mock the service in > > > browsing_data_remover_unittest.cc to ensure test coverage but this is > nothing > > we > > > need to merge to M54. > > > > > https://codereview.chromium.org/2342673004/diff/1/chrome/browser/browsing_dat... > > File chrome/browser/browsing_data/browsing_data_remover.cc (left): > > > > > https://codereview.chromium.org/2342673004/diff/1/chrome/browser/browsing_dat... > > chrome/browser/browsing_data/browsing_data_remover.cc:525: > > ContentSuggestionsServiceFactory::GetForProfile(profile_); > > On 2016/09/15 12:53:17, jkrcal wrote: > > > On 2016/09/15 12:45:09, Marc Treib wrote: > > > > GetForProfile is called in one more place in this file, you should update > > that > > > > too. > > > > > > Oh, thanks for spotting that! > > > > Should we add a DCHECK that the feature is enabled when constructing the > service > > so that we catch any regressions here? > > Where do you mean to add the DCHECK? The ContentSuggestionService can get > constructed when the feature is off, it is > just disabled. Except that's evidently not what happens without this CL -- the ContentSuggestionService is constructed, and some provider immediately tries to do a network request. We should define it so that either the ContentSuggestionService may not be constructed at all when the feature is off, or it can be constructed but does nothing.
On 2016/09/15 14:55:01, Bernhard Bauer wrote: > On 2016/09/15 14:30:11, jkrcal wrote: > > On 2016/09/15 13:44:46, Bernhard Bauer wrote: > > > On 2016/09/15 12:38:47, jkrcal wrote: > > > > Hi Bernhard, this is another M54 merge candidate. PTAL! > > > > > > What's the impact on M54? Is it only unit tests? > > > > This CL is a prerequisite for CL 2343583002 to pass unit-tests. > > (we do not need this change on its own in M54, we would like to have the > other > > CL in M54). > > > > As these are quite unrelated bugs, I wanted to land them in separate CLs. > > OK, but then we don't necessarily need to merge this CL? We don't really run > many tests on branches. I did not know that, great! Anyway, we need to land this to be able to land the other CL which we would like to merge. > > > > > A bit more of explanation: > > > > If we flip the sign-in-not-required param to true by default (another M54 > > > merge > > > > candidate), this means that the ContentSuggestionService is enabled by > > > default, > > > > which means that BrowsingDataRemover creates all the providers of > > > > ContentSuggestionService when called from its unit-test which means that > > many > > > > unit-tests fail because the non-mocked providers try to do I/O on the > > > unit-test > > > > thread :( > > > > > > What is the unit-test thread? The bug alone is a bit sparse on context. > > > > I meant the single thread on which the unit-test runs. > > (Maybe I am wrong with the context, anyway a CHECK fails stating > > "MessageLoop::TYPE_IO == loop->type()" so I guess some I/O operations get > > executed on the wrong thread.) > > > > I updated the bug. > > OK, but what I was looking for was more like a link to the failing test, with a > full stack trace. I've added a comment to the bug. Ok, thanks. > So, TYPE_IO actually means _non-blocking_ I/O, i.e. IPC or network connections > (file I/O actually is allowed in a unit test, but non-blocking I/O requires a > special type of message loop). It would be interesting to find out what is > actually trying to open a network connection here -- probably the > SnippetsService trying to fetch from the server? Thanks for explaining. Yes, I would guess it is the snippets service. Do you want me to find it out with more certainty? (at the same time, I would like to land the other CL in a reasonable time so that there are still some chances for merging) > > > > As the next step, we should mock the service in > > > > browsing_data_remover_unittest.cc to ensure test coverage but this is > > nothing > > > we > > > > need to merge to M54. > > > > > > > > > https://codereview.chromium.org/2342673004/diff/1/chrome/browser/browsing_dat... > > > File chrome/browser/browsing_data/browsing_data_remover.cc (left): > > > > > > > > > https://codereview.chromium.org/2342673004/diff/1/chrome/browser/browsing_dat... > > > chrome/browser/browsing_data/browsing_data_remover.cc:525: > > > ContentSuggestionsServiceFactory::GetForProfile(profile_); > > > On 2016/09/15 12:53:17, jkrcal wrote: > > > > On 2016/09/15 12:45:09, Marc Treib wrote: > > > > > GetForProfile is called in one more place in this file, you should > update > > > that > > > > > too. > > > > > > > > Oh, thanks for spotting that! > > > > > > Should we add a DCHECK that the feature is enabled when constructing the > > service > > > so that we catch any regressions here? > > > > Where do you mean to add the DCHECK? The ContentSuggestionService can get > > constructed when the feature is off, it is > > just disabled. > > Except that's evidently not what happens without this CL -- the > ContentSuggestionService is constructed, and some provider immediately tries to > do a network request. We should define it so that either the > ContentSuggestionService may not be constructed at all when the feature is off, > or it can be constructed but does nothing. Are you sure the features are off in unit-tests? Maybe I do not understand the test infrastructure. The features (such as NTPArticleSuggestions are on by default in components/ntp_snippets/features.cc).
On 2016/09/15 16:25:17, jkrcal wrote: > On 2016/09/15 14:55:01, Bernhard Bauer wrote: > > On 2016/09/15 14:30:11, jkrcal wrote: > > > On 2016/09/15 13:44:46, Bernhard Bauer wrote: > > > > On 2016/09/15 12:38:47, jkrcal wrote: > > > > > Hi Bernhard, this is another M54 merge candidate. PTAL! > > > > > > > > What's the impact on M54? Is it only unit tests? > > > > > > This CL is a prerequisite for CL 2343583002 to pass unit-tests. > > > (we do not need this change on its own in M54, we would like to have the > > other > > > CL in M54). > > > > > > As these are quite unrelated bugs, I wanted to land them in separate CLs. > > > > OK, but then we don't necessarily need to merge this CL? We don't really run > > many tests on branches. > > I did not know that, great! Anyway, we need to land this to be able to land the > other CL > which we would like to merge. > > > > > > > > A bit more of explanation: > > > > > If we flip the sign-in-not-required param to true by default (another > M54 > > > > merge > > > > > candidate), this means that the ContentSuggestionService is enabled by > > > > default, > > > > > which means that BrowsingDataRemover creates all the providers of > > > > > ContentSuggestionService when called from its unit-test which means that > > > many > > > > > unit-tests fail because the non-mocked providers try to do I/O on the > > > > unit-test > > > > > thread :( > > > > > > > > What is the unit-test thread? The bug alone is a bit sparse on context. > > > > > > I meant the single thread on which the unit-test runs. > > > (Maybe I am wrong with the context, anyway a CHECK fails stating > > > "MessageLoop::TYPE_IO == loop->type()" so I guess some I/O operations get > > > executed on the wrong thread.) > > > > > > I updated the bug. > > > > OK, but what I was looking for was more like a link to the failing test, with > a > > full stack trace. I've added a comment to the bug. > > Ok, thanks. > > > So, TYPE_IO actually means _non-blocking_ I/O, i.e. IPC or network connections > > (file I/O actually is allowed in a unit test, but non-blocking I/O requires a > > special type of message loop). It would be interesting to find out what is > > actually trying to open a network connection here -- probably the > > SnippetsService trying to fetch from the server? > > Thanks for explaining. Yes, I would guess it is the snippets service. Do you > want > me to find it out with more certainty? (at the same time, I would like to land > the other CL in a reasonable time so that there are still some chances for > merging) > > > > > > As the next step, we should mock the service in > > > > > browsing_data_remover_unittest.cc to ensure test coverage but this is > > > nothing > > > > we > > > > > need to merge to M54. > > > > > > > > > > > > > > https://codereview.chromium.org/2342673004/diff/1/chrome/browser/browsing_dat... > > > > File chrome/browser/browsing_data/browsing_data_remover.cc (left): > > > > > > > > > > > > > > https://codereview.chromium.org/2342673004/diff/1/chrome/browser/browsing_dat... > > > > chrome/browser/browsing_data/browsing_data_remover.cc:525: > > > > ContentSuggestionsServiceFactory::GetForProfile(profile_); > > > > On 2016/09/15 12:53:17, jkrcal wrote: > > > > > On 2016/09/15 12:45:09, Marc Treib wrote: > > > > > > GetForProfile is called in one more place in this file, you should > > update > > > > that > > > > > > too. > > > > > > > > > > Oh, thanks for spotting that! > > > > > > > > Should we add a DCHECK that the feature is enabled when constructing the > > > service > > > > so that we catch any regressions here? > > > > > > Where do you mean to add the DCHECK? The ContentSuggestionService can get > > > constructed when the feature is off, it is > > > just disabled. > > > > Except that's evidently not what happens without this CL -- the > > ContentSuggestionService is constructed, and some provider immediately tries > to > > do a network request. We should define it so that either the > > ContentSuggestionService may not be constructed at all when the feature is > off, > > or it can be constructed but does nothing. > > Are you sure the features are off in unit-tests? > Maybe I do not understand the test infrastructure. > The features (such as NTPArticleSuggestions are on by default in > components/ntp_snippets/features.cc). Sorry, I phrased this the wrong way. What I would like to do is this: 1) Investigate what (which provider, which method) initiated the network request (which isn't immediately obvious from the stack trace). 2) Add a DCHECK at that point so that if someone else instantiates a ContentSuggestionService by accident, they will get an error that immediately tells them they're Doing It Wrong. We can do these things after this CL has landed though, so add a TODO and it'll LGTM.
I've added the TODO. Thanks, Berhnard!
The CQ bit was checked by jkrcal@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: This issue passed the CQ dry run.
The CQ bit was checked by jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2342673004/#ps40001 (title: "Last comment of Bernhard")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Clear browsing data from ContentSuggestionService only if exists. Currently, we instantiate the service in BrowsingDataRemover even when it does not exist, yet. This means that we instantiate it on non-android platforms, in unit-tests, etc. This CL changes the logic so that the service is accessed only if it has been created previously. This way, the BrowsingDataRemover respects the start-up logic that decides whether to create the service or not. BUG=647222 ========== to ========== Clear browsing data from ContentSuggestionService only if exists. Currently, we instantiate the service in BrowsingDataRemover even when it does not exist, yet. This means that we instantiate it on non-android platforms, in unit-tests, etc. This CL changes the logic so that the service is accessed only if it has been created previously. This way, the BrowsingDataRemover respects the start-up logic that decides whether to create the service or not. BUG=647222 Committed: https://crrev.com/9d1e504520b774c0a6f24920f730748053c4b2b2 Cr-Commit-Position: refs/heads/master@{#419141} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9d1e504520b774c0a6f24920f730748053c4b2b2 Cr-Commit-Position: refs/heads/master@{#419141} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
