I think there is already a bug about this, but can't find it. thakis@ can you help?

I think the case in the bug description is already undefined behavior (dereferencing null), which UBSan can catch. In the bug in the postmortem the null pointer was passed to something else though, which could be more complicated for the compiler to catch.

I think issue 409318 is mostly the same bug, but the compiler won't find this if it crosses function boundaries.

In the case of the code that caused the crash:
  "field_trial_creator_(local_state, client.get(), ui_string_overrider),"

Perhaps instead of the compiler needing to reason about how it gets used, could it simply issue a warning "warning: client.get() will always be nullptr"?

If that was in a separate function and not preceded by std::move(client) in the same function, how would it know?

That line was in the ctor where the std::move() was on a preceding line in the initializer list.

That contradicts my reading of comment 2. Maybe you could the whole snippet containing the bug? :-) But yes, sounds like a dupe of issue 409318 so far.

https://docs.google.com/document/d/1JDJFFmMPhh44nTYkpCwxsd_ikpL7w37xtH5J9q8IJvE/edit

 VariationsService::VariationsService(
     std::unique_ptr<VariationsServiceClient> client,
     std::unique_ptr<web_resource::ResourceRequestAllowedNotifier> notifier,
     PrefService* local_state,
     metrics::MetricsStateManager* state_manager,
     const UIStringOverrider& ui_string_overrider)
     : client_(std::move(client)),
-      ui_string_overrider_(ui_string_overrider),
       local_state_(local_state),
       state_manager_(state_manager),
       policy_pref_service_(local_state),
-      seed_store_(local_state),
-      create_trials_from_seed_called_(false),
       initial_request_completed_(false),
       disable_deltas_for_next_request_(false),
       resource_request_allowed_notifier_(std::move(notifier)),
       request_count_(0),
+      field_trial_creator_(local_state, client.get(), ui_string_overrider),
       weak_ptr_factory_(this) {


To clarify, the actual crash happens much later. When field_trial_creator_ ends up trying to use the ptr it got in param 2.

My point is instead of trying to figure out the whole callgraph and see that a crash could happen later, the compiler could warn that an expression will always result in nullptr (and therefore if nullptr is what's desired, maybe the author should use nullptr explicitly).

However, not sure how many existing code this would uncover that does this currently.

Yes, this sounds like issue 409318.

I wonder if we can change unique_ptr so that it goes into "get() asserts" state after std::move(), and stays there until it's explicitly assigned anything.

> I wonder if we can change unique_ptr so that it goes into "get() asserts" state after std::move()

That would break the many legitimate instances "if(foo.get() == nullptr)", though, wouldn't it? (We could disallow that pattern, and tell everyone to use unique_ptr's built-in bool cast instead, although I expect such a proposal would be destined for epic bikeshedding.)

Bulk edit**

This bug has the label Postmortem-Followup but has not been updated in 3+ weeks. We are working on a new workflow to improve postmortem followthrough. Postmortems and postmortem bugs are very important in making sure we don't repeat prior mistakes and for making Chrome better for all.

We will be taking a closer look at these bugs in the coming weeks. Please take some time to work on this, reassign, or close if the issue has been fixed. Thank you.