Hi, Nicolas, Stack frame shows something related to sync. Not sure if this is actually your area. Please help re-dispatch if it isn't. Thanks!

Looks like we're waking up off a timer, and trying to write out local data from |local_device_info_provider_|. In the past we've had problems doing this if we're in the process of a shutdown. We theoretically guard against this by stopping the timer if StopSyncing() is called. Something is clearly going wrong though, will investigate.

arenastring.h seems to be a protobuf library that we're crashing in the middle of. We pass a bunch of const string& into it and then it converts to raw pointer, then de-refs again, and gives it to new ::std::string(), where we seem to crash. So my working theory is one of these strings is bad somehow.

We get all of these strings from the DeviceInfo object, with suspects beign
* guid
* client_name
* chrome_version
* sync_user_agent
* signin_scopd_device_id

Bizarrely, signin_scopd_device_id isn't held onto as a const by DeviceInfo, but just a normal std::string, see https://cs.chromium.org/chromium/src/components/sync/device_info/device_info.h?l=89

Looking at my data, it actually looks like the signin_scopd_device_id field has been blanked since ~M57.

Looking at crashes, I see other arenastring stable signatures before M57, but none coming from DeviceInfoSyncService. This class didn't receive significant changes in M57, https://chromium.googlesource.com/chromium/src/+log/0dd660b9ba6e4699468c635874da841a25e135a7/components/sync/device_info/device_info_sync_service.cc , which makes me think it has to do with how the DeviceInfo[Provider] is exposing data.

Also interesting to note, these clients all have huge uptime, like hours. Which in my experience isn't very typical.

Well, disregard #7, initial hunch in #4 seems to be correct. If you change the pulse interval at https://cs.chromium.org/chromium/src/components/sync/device_info/device_info_util.cc?l=19 to TimeDelta::FromMilliseconds(1), and then sign out, you immediately hit this crash. As we had encountered earlier, the shutdown is not being called in time. Going to add a simple nullptr check.

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

commit babffd17e43935e30657dcfda02d962791adf460
Author: skym <skym@chromium.org>
Date: Tue Apr 11 22:47:21 2017

[Sync] Always check the device info provider for being null before using it.

During sign out, the DeviceInfoProviderImpl is reset before
DeviceInfoSyncService is told we're shutting down, as well as in a
separate task. This means if we get extremely unlucky, and a pulse is
happening at just the right time, we'll try to use the provider after
it's been destroyed. To fix this, we can just check that it's not
nullptr before using it. The other usages of the provider in
DeviceInfoSyncService have stronger guarantees from sync that we're not
in shutdown yet, and as such they DCHECK instead.

No unit tests were added as part of this change. It is non-trivial to
force an asyc pulse, and we currently do not have good seams to allow
this. Since this change needs to be merged back, I want this
modification to be as small as possible.

BUG= 709927 

Review-Url: https://codereview.chromium.org/2810873005
Cr-Commit-Position: refs/heads/master@{#463814}

[modify] https://crrev.com/babffd17e43935e30657dcfda02d962791adf460/components/sync/device_info/device_info_sync_service.cc

Adding merge request for M58. This is a race condition that's causing crashes to real users right now.

Was present in M57, we're seeing 10-30 crashes on iOS per day. Not sure why we haven't seen similar crashes on other platforms.

This bug requires manual review: Less than 9 days to go before AppStore submit on M58
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

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

I forgot to mention in #10, the risk here is low. Basically just wrapping existing logic with a null check, so we don't UAF.

can you mark this issue as fixed?

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

commit c9c3186627372f19063b9855a2176150567a5889
Author: Sky Malice <skym@chromium.org>
Date: Thu Apr 13 21:40:45 2017

[Sync] Always check the device info provider for being null before using it.

During sign out, the DeviceInfoProviderImpl is reset before
DeviceInfoSyncService is told we're shutting down, as well as in a
separate task. This means if we get extremely unlucky, and a pulse is
happening at just the right time, we'll try to use the provider after
it's been destroyed. To fix this, we can just check that it's not
nullptr before using it. The other usages of the provider in
DeviceInfoSyncService have stronger guarantees from sync that we're not
in shutdown yet, and as such they DCHECK instead.

No unit tests were added as part of this change. It is non-trivial to
force an asyc pulse, and we currently do not have good seams to allow
this. Since this change needs to be merged back, I want this
modification to be as small as possible.

BUG= 709927 

Review-Url: https://codereview.chromium.org/2810873005
Cr-Commit-Position: refs/heads/master@{#463814}
(cherry picked from commit babffd17e43935e30657dcfda02d962791adf460)

Review-Url: https://codereview.chromium.org/2818803003 .
Cr-Commit-Position: refs/branch-heads/3029@{#698}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/c9c3186627372f19063b9855a2176150567a5889/components/sync/device_info/device_info_sync_service.cc