commit 6b5c1777dbacf6a78e8f84a845f4346a22f9129d
author Ambre Williams <ambre@google.com> Mon Jan 21 16:01:39 2019 +0100
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Wed Jan 23 13:31:00 2019 +0000
tree 754041eb268b2afce78581a414b956ae674a40ec
parent a4dca1268eff2fc7560df64d4de46001ed61196e

[ledger] Clean up state of PageManagerContainer

Fix a potential null pointer dereference in PageManagerContainer and add
comments documenting the different possible states.

TEST=ledger_tests

Change-Id: I43f12a0aead1225a524037702d20e5e78291ce77

bin/ledger/app/ledger_manager.cc

diff --git a/bin/ledger/app/ledger_manager.cc b/bin/ledger/app/ledger_manager.cc
index 8f4efa0..537d14f 100644
--- a/bin/ledger/app/ledger_manager.cc
+++ b/bin/ledger/app/ledger_manager.cc
@@ -228,9 +228,17 @@
   void CheckEmpty();
 
   const storage::PageId page_id_;
+
   std::unique_ptr<PageManager> page_manager_;
-  PageConnectionNotifier connection_notifier_;
+  // |status_| holds the status given to |SetPageManager|. If
+  // |page_manager_is_set_| is true, |status_| is |Status::OK| if and only if
+  // |page_manager_| is not null.
   Status status_ = Status::OK;
+  // |page_manager_is_set_| if |SetPageManager| has been called. |page_manager_|
+  // may still be null.
+  bool page_manager_is_set_ = false;
+
+  PageConnectionNotifier connection_notifier_;
   std::vector<std::pair<std::unique_ptr<PageDelayingFacade>,
                         fit::function<void(Status)>>>
       requests_;
@@ -239,7 +247,6 @@
       debug_requests_;
   std::vector<fit::function<void(Status, ExpiringToken, PageManager*)>>
       internal_request_callbacks_;
-  bool page_manager_is_set_ = false;
   fit::closure on_empty_callback_;
 
   // Must be the last member.
@@ -329,7 +336,7 @@
     Status status, std::unique_ptr<PageManager> page_manager) {
   TRACE_DURATION("ledger", "ledger_manager_set_page_manager");
 
-  FXL_DCHECK(!page_manager_);
+  FXL_DCHECK(!page_manager_is_set_);
   FXL_DCHECK((status != Status::OK) == !page_manager);
   status_ = status;
   page_manager_ = std::move(page_manager);
@@ -372,8 +379,8 @@
 }
 
 bool LedgerManager::PageManagerContainer::PageConnectionIsOpen() {
-  return (page_manager_is_set_ && !page_manager_->IsEmpty()) ||
-         !requests_.empty() || !debug_requests_.empty();
+  return (page_manager_ && !page_manager_->IsEmpty()) || !requests_.empty() ||
+         !debug_requests_.empty();
 }
 
 ExpiringToken LedgerManager::PageManagerContainer::NewExpiringToken() {
@@ -384,6 +391,8 @@
 }
 
 void LedgerManager::PageManagerContainer::CheckEmpty() {
+  // The PageManagerContainer is not considered empty until |SetPageManager| has
+  // been called.
   if (on_empty_callback_ && connection_notifier_.IsEmpty() &&
       page_manager_is_set_ && (!page_manager_ || page_manager_->IsEmpty())) {
     on_empty_callback_();