Reproducible in the latest (M65) Canary.

Thank you for noticing and diagnosing this quickly, Jan!

Hi,

The root problem is sadly obvious in retrospect: the NTP Snippets code relies on receiving SigninManager::Observer callbacks, while IdentityManager also relies on these callbacks in order to update its internal state. Incrementally converting the NTP Snippets code to use IdentityManager had the nasty side effect that the state of IdentityManager was not guaranteed to reflect that of SigninManager when the snippets code goes through its flow in response to signin/signout.

In the short term the best plan is for me to put together a CL reverting the RemoteSuggestionsFetcherImpl conversion as well as its unittest conversion, land that on trunk, and merge it to 65. I'll then solve this ordering issue blocking incremental conversion and should be able to reland those CLs without change. I'll also look at adding a unittest of RemoteSuggestionsFetcherImpl to catch this case.

One question that I have is whether the ContextualSuggestionsFetcherImpl changes also need to be reverted: https://chromium-review.googlesource.com/c/chromium/src/+/870831. AFAICT, ContextualSuggestionsFetcherImpl::FetchContextualSuggestions() is *not* invoked via reception of GoogleSignedInSucceeded()/GoogleSignedOut() callbacks. Is that correct?

Reverting all sounds a bit harsh. What about having a mode that assumes you are signed in and you are willing to wait for the refresh token etc. It differs from the current kWaitUntilAvailable mode by _not_ waiting any more if the user is offline or becomes offline during the process.

Anyway, if you think reverting is the best way, you do not need to care about ContextualSuggestions as this is not launched.

Do you plan to revert the changes?

(This is relevant for me and for the fix&merge of  https://crbug.com/802179 ; I should not land anything before your changes.)

I am planning to revert the changes tomorrow (didn't manage to get there today due to the techtalk...).

Thanks! Please cc me on the CLs.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d8bd981fe9d407f8f6b5bd0071d7fdb4c1dd2690

commit d8bd981fe9d407f8f6b5bd0071d7fdb4c1dd2690
Author: Colin Blundell <blundell@chromium.org>
Date: Wed Jan 24 12:59:44 2018

[NTP Snippets] Revert RemoteSuggestionsFetcher usage of IdentityManager


This CL reverts the following two CLs:
- "[NTP Snippets] RemoteSuggestionsFetcherImpl test uses IdentityTestEnv"
  (fce90bed4910ce81836dca45bc923bda6220d278)
- "[NTP Snippets] Have RemoteSuggestionsFetcherImpl use IdentityManager"
  (683639a46be8c1a9c4ee278fe518bccd1a3ef0c5)

The reason is that the production change caused a regression in NTP
Snippets: on signout, an unauthenticated fetch of snippets is no longer
performed. The underlying cause is that IdentityManager being notified of
signout (and changing its internal state) currently races with NTP Snippets
itself being notified of signout (and starting its flow to refetch
snippets).

This is a straight revert with three minor exceptions:
- There was a minor amount of conflict resolution to do due to
  intervening changes of base::MakeUnique to std::make_unique
- The //services/identity API additions in fce90bed were *not* reverted
  as they are used by code that has subsequently landed in the tree.
- Similarly, the //components/ntp_snippets/BUILD.gn changes were not
  reverted as those dependencies are now used on trunk even without
  these changes.

This CL itself should be able to be reverted once I have fixed the above
race by ensuring that IdentityManager is notified of signin/signout events
before any observers of SigninManager. Note that in the long term this
won't be a problem as there will be no external observers of SigninManager.

To test this change, do the following:
- Sign in to Chrome on Android (in a chrome_apk build if building locally)
- Observe that there are personalized suggestions on the NTP
- Sign out
- Observe that there are different, generic suggestions on the NTP

TBR=gambard@chromium.org

Bug:  804410 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I114c3c4805d855b646bb748d4e2c5c5e9a966c3b
Reviewed-on: https://chromium-review.googlesource.com/883321
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531511}
[modify] https://crrev.com/d8bd981fe9d407f8f6b5bd0071d7fdb4c1dd2690/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
[modify] https://crrev.com/d8bd981fe9d407f8f6b5bd0071d7fdb4c1dd2690/components/ntp_snippets/remote/remote_suggestions_fetcher_impl.cc
[modify] https://crrev.com/d8bd981fe9d407f8f6b5bd0071d7fdb4c1dd2690/components/ntp_snippets/remote/remote_suggestions_fetcher_impl.h
[modify] https://crrev.com/d8bd981fe9d407f8f6b5bd0071d7fdb4c1dd2690/components/ntp_snippets/remote/remote_suggestions_fetcher_impl_unittest.cc
[modify] https://crrev.com/d8bd981fe9d407f8f6b5bd0071d7fdb4c1dd2690/ios/chrome/browser/ntp_snippets/BUILD.gn
[modify] https://crrev.com/d8bd981fe9d407f8f6b5bd0071d7fdb4c1dd2690/ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc
[modify] https://crrev.com/d8bd981fe9d407f8f6b5bd0071d7fdb4c1dd2690/ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory_util.cc

I verified on today's canary that if you sign out, you have suggestions on the NTP when you return. Jan, if you could do a sanity check on today's canary that the problematic behavior that you saw in the OP is gone, that would be helpful as well :).

This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Colin, the last Canary for Android is from Jan 22 so I cannot verify it yet :|

I verified it in my own build of Chrome if it helps.

Hi Jan,

Thanks for your assistance!

When I downloaded Canary on Android earlier today, I got 66.0.3331.0 at #531794. This CL landed as #531511. Are you seeing canary at a different rev?

Ping! Can we get a merge review here? Thanks!

Verified in:

App Version: 66.0.3335.0 canary
Devices: iPhone 7 Plus, iPhone 8
iOS Versions: 10.3.3, 11.2.5

Followed the steps mentioned in comment #8 to verify this issue, 
[1] When user sign in to chrome, personalized suggestions are displayed on NTP 
[2] When user sign out of chrome, generic suggestions are displayed on NTP

Current Behaviour:
https://drive.google.com/open?id=1rl1PbuyC15AbhNALWYVAHbSQgPWh2xtR

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bc68b5aeaccfdbe7c94f7b33d6e7e42d41f88eae

commit bc68b5aeaccfdbe7c94f7b33d6e7e42d41f88eae
Author: Colin Blundell <blundell@chromium.org>
Date: Fri Feb 02 13:01:38 2018

[NTP Snippets] Revert RemoteSuggestionsFetcher usage of IdentityManager

This CL reverts the following two CLs:
- "[NTP Snippets] RemoteSuggestionsFetcherImpl test uses IdentityTestEnv"
  (fce90bed4910ce81836dca45bc923bda6220d278)
- "[NTP Snippets] Have RemoteSuggestionsFetcherImpl use IdentityManager"
  (683639a46be8c1a9c4ee278fe518bccd1a3ef0c5)

The reason is that the production change caused a regression in NTP
Snippets: on signout, an unauthenticated fetch of snippets is no longer
performed. The underlying cause is that IdentityManager being notified of
signout (and changing its internal state) currently races with NTP Snippets
itself being notified of signout (and starting its flow to refetch
snippets).

This is a straight revert with three minor exceptions:
- There was a minor amount of conflict resolution to do due to
  intervening changes of base::MakeUnique to std::make_unique
- The //services/identity API additions in fce90bed were *not* reverted
  as they are used by code that has subsequently landed in the tree.
- Similarly, the //components/ntp_snippets/BUILD.gn changes were not
  reverted as those dependencies are now used on trunk even without
  these changes.

This CL itself should be able to be reverted once I have fixed the above
race by ensuring that IdentityManager is notified of signin/signout events
before any observers of SigninManager. Note that in the long term this
won't be a problem as there will be no external observers of SigninManager.

To test this change, do the following:
- Sign in to Chrome on Android (in a chrome_apk build if building locally)
- Observe that there are personalized suggestions on the NTP
- Sign out
- Observe that there are different, generic suggestions on the NTP

TBR=blundell@chromium.org, gambard@chromium.org

(cherry picked from commit d8bd981fe9d407f8f6b5bd0071d7fdb4c1dd2690)

Bug:  804410 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I114c3c4805d855b646bb748d4e2c5c5e9a966c3b
Reviewed-on: https://chromium-review.googlesource.com/883321
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#531511}
Reviewed-on: https://chromium-review.googlesource.com/899083
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/branch-heads/3325@{#257}
Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369}
[modify] https://crrev.com/bc68b5aeaccfdbe7c94f7b33d6e7e42d41f88eae/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
[modify] https://crrev.com/bc68b5aeaccfdbe7c94f7b33d6e7e42d41f88eae/components/ntp_snippets/remote/remote_suggestions_fetcher_impl.cc
[modify] https://crrev.com/bc68b5aeaccfdbe7c94f7b33d6e7e42d41f88eae/components/ntp_snippets/remote/remote_suggestions_fetcher_impl.h
[modify] https://crrev.com/bc68b5aeaccfdbe7c94f7b33d6e7e42d41f88eae/components/ntp_snippets/remote/remote_suggestions_fetcher_impl_unittest.cc
[modify] https://crrev.com/bc68b5aeaccfdbe7c94f7b33d6e7e42d41f88eae/ios/chrome/browser/ntp_snippets/BUILD.gn
[modify] https://crrev.com/bc68b5aeaccfdbe7c94f7b33d6e7e42d41f88eae/ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc
[modify] https://crrev.com/bc68b5aeaccfdbe7c94f7b33d6e7e42d41f88eae/ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory_util.cc

Verified the issue on 65.0.3325.53 beta tested on iPhone6,iPhone7Plus,iPad Pro and iPhonex(10.3.3,11.2.2).
Content suggestions area displayed in signing out of chrome,works fine.