MB-43028: [1/2] Make overhead tracking safe at VBucket destruction

Cherry-picks http://review.couchbase.org/c/kv_engine/+/141492

Note: This patch is cherry-picked as the issue it resolves prevents
clean forward-merging of a change _earlier_ in mad-hatter history.

Merging http://review.couchbase.org/c/kv_engine/+/136495 into master
uncovered santizer issues (mad-hatter CV runs an older Clang and did
not identify these issues).

This patch resolves one of these issues, before the above patch may
be merged to master.

WARNING: ThreadSanitizer: heap-use-after-free (pid=73551)
   Read of size 8 at 0x7b5800000310 by main thread:
     #0 operator() ../kv_engine/engines/ep/src/ephemeral_bucket.cc:301 (libep.so+0x00000034705f)
     #1 std::_Function_handler<void (long), EphemeralBucket::makeVBucket(...>::_M_invoke(std::_Any_data const&, long&&)
     #3 ~Checkpoint ../kv_engine/engines/ep/src/checkpoint.cc:228 (libep.so+0x00000019dc97)

     #10 ~CheckpointManager ../kv_engine/engines/ep/src/checkpoint_manager.h:73 (libep.so+0x00000042f782)
     ...
     #13 ~VBucket ../kv_engine/engines/ep/src/vbucket.cc:286 (libep.so+0x00000041689a)

During VBucket destruction, the CheckpointManager member, and any
remaining Checkpoints it holds are destroyed. This can trigger the
memOverheadChangedCallback, as destroying Checkpoints reduces the
memory overhead.

This was unsafe, as the state member had already been destroyed as it
is declared later in VBucket.

Move the CheckpointManager to be declared after state, so it is
destroyed first.

Change-Id: I0a34d3a11cd053f18f3168d6fbbb9dc974bbd2fc
Reviewed-on: http://review.couchbase.org/c/kv_engine/+/141492
Reviewed-by: Dave Rigby <[email protected]>
Tested-by: Build Bot <[email protected]>
Well-Formed: Build Bot <[email protected]>
Reviewed-on: http://review.couchbase.org/c/kv_engine/+/141498

Author: jameseh96

engines/ep/src/vbucket.cc

@@ -190,14 +190,6 @@ VBucket::VBucket(Vbid i,
                  const nlohmann::json* replTopology,
                  uint64_t maxVisibleSeqno)
     : ht(st, std::move(valFact), config.getHtSize(), config.getHtLocks()),
-      checkpointManager(std::make_unique<CheckpointManager>(st,
-                                                            i,
-                                                            chkConfig,
-                                                            lastSeqno,
-                                                            lastSnapStart,
-                                                            lastSnapEnd,
-                                                            maxVisibleSeqno,
-                                                            flusherCb)),
       failovers(std::move(table)),
       opsCreate(0),
       opsDelete(0),
@@ -220,6 +212,14 @@ VBucket::VBucket(Vbid i,
       id(i),
       state(newState),
       initialState(initState),
+      checkpointManager(std::make_unique<CheckpointManager>(st,
+                                                            i,
+                                                            chkConfig,
+                                                            lastSeqno,
+                                                            lastSnapStart,
+                                                            lastSnapEnd,
+                                                            maxVisibleSeqno,
+                                                            flusherCb)),
       replicationTopology(std::make_unique<nlohmann::json>()),
       purge_seqno(purgeSeqno),
       takeover_backed_up(false),

engines/ep/src/vbucket.h

@@ -837,9 +837,6 @@ class VBucket : public std::enable_shared_from_this<VBucket> {
 
     HashTable ht;
 
-    /// Manager of this vBucket's checkpoints. unique_ptr for pimpl.
-    std::unique_ptr<CheckpointManager> checkpointManager;
-
     /**
      * Searches for a 'valid' StoredValue in the VBucket.
      *
@@ -2442,6 +2439,15 @@ class VBucket : public std::enable_shared_from_this<VBucket> {
 
     vbucket_state_t                 initialState;
 
+public:
+    /**
+     * Manager of this vBucket's checkpoints. unique_ptr for pimpl.
+     * Declared after state as Checkpoint destruction may update stats
+     * based on the vbucket's current state.
+     */
+    std::unique_ptr<CheckpointManager> checkpointManager;
+
+private:
     /**
      * The replication topology, set as part of SET_VBUCKET_STATE.
      * It is encoded as nlohmann::json array of (max 2) replication chains.