Postmortem report collection: validate internal state

components/browser_watcher/postmortem_report_extractor.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/browser_watcher/postmortem_report_extractor.h"
 
 #include <memory>
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "base/debug/activity_analyzer.h"
 #include "base/logging.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/metrics/persistent_memory_allocator.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/browser_watcher/stability_data_names.h"
 #include "components/variations/active_field_trials.h"
 #include "third_party/crashpad/crashpad/util/misc/uuid.h"
 
 namespace browser_watcher {
 
@@ skipping 182 matching lines @@
 CollectionStatus Extract(const base::FilePath& stability_file,
                          StabilityReport* report) {
   DCHECK(report);
 
   // Create a global analyzer.
   std::unique_ptr<GlobalActivityAnalyzer> global_analyzer =
       GlobalActivityAnalyzer::CreateWithFile(stability_file);
   if (!global_analyzer)
     return ANALYZER_CREATION_FAILED;
 
+  uint8_t allocator_state = global_analyzer->allocator().GetMemoryState();

[bcwhite, 2017/03/23 11:32:44]

I think it would be better to add a GetMemoryState() method to
GlobalActivityAnalyzer.  It could then be smart about what it returns,
especially in the face of multiple files under analysis.

The analyzer's version could include other values:
MEMORY_FULL = 99,
MEMORY_CORRUPT = 98,
etc.

[bcwhite, 2017/03/23 11:35:40]

Make that...

MEMORY_FULL = 100,    // MEMORY_USER_DEFINED + 0
MEMORY_CORRUPT = 101, // MEMORY_USER_DEFINED + 1

[manzagop (departed), 2017/03/23 19:18:54]

Acknowledged.

[manzagop (departed), 2017/03/23 19:18:54]

Hm, maybe it's best to decouple the two. Tell me what you think.

[bcwhite, 2017/03/23 21:10:43]

The memory state was written expecting that it would be extended if a
higher-level knew more detail so I think it's fine and better than creating
something new that simply copies most of it and extends.

[manzagop (departed), 2017/03/23 21:25:02]

I'm not sure I follow. Here are the main things:
- full/corrupt are properties of the allocator - why should they be in the user
defined range?
- if corrupt/full are valid memory states, should GetMemoryState return these?
Should IsCorrupt/IsFull become private?
- If we plan on having multiple files to analyze, then I think it makes sense to
have a different enum to represent the overall state.

[bcwhite, 2017/03/24 12:49:04]

If you ignore the implementation details, you can simply think of it as the
tracker's state.

More accurately, full & corrupt are "flags" but since you're not reporting them
that way upstream, I don't think it's important to differentiate them as such
here, especially when there is a fairly clear priority order: full means nothing
if corrupt, full and corrupt mean nothing if deleted, etc.
Yes.
They allocator is private so those methods are inaccessible outside the
analyzer.  They're important to the user of the allocator (the analyzer in this
case) but shouldn't be important to the user of the analyzer.
Eventually I think it will go that way but what you'd really want is state per
process.  I'd hold off on that until it proved important in the field, however,
as its likely to be rare enough to not warrant the effort.

Accessing the allocator for state information in order to do analysis shouldn't
be necessary since this is the "analyzer" class.  This class should do the
interpretation of the underlying data and report it in a meaningful manner to
the caller.

The only reason to access the allocator or any of its methods should be if
non-tracker information was also being stored there for convenience purposes.

+  if (allocator_state == base::PersistentMemoryAllocator::MEMORY_UNINITIALIZED)
+    return ALLOCATOR_UNINITIALIZED;
+  if (allocator_state == base::PersistentMemoryAllocator::MEMORY_DELETED)
+    return ALLOCATOR_DELETED;
+  if (global_analyzer->allocator().IsCorrupt())
+    return ALLOCATOR_CORRUPT;
+  UMA_HISTOGRAM_BOOLEAN("ActivityTracker.Collect.IsAllocatorFull",

[bcwhite, 2017/03/23 11:32:44]

I think it would be better to just enable the internal histograms of the
allocator behind the tracker.  I'll add that to the tracker.

This way you'll know how much of the space is being used, rather than just
after-the-fact that it became full.

[manzagop (departed), 2017/03/23 19:18:54]

Removed the fullness histogram. Will the histograms be specific to activity
tracking?

[bcwhite, 2017/03/23 21:10:43]

They're specific to the allocator's name:

UMA.PersistentAllocator.NAME.UsedPct

[manzagop (departed), 2017/03/23 21:25:02]

Great!

+                        global_analyzer->allocator().IsFull());
+
   // Early exit if there is no data.
   std::vector<std::string> log_messages = global_analyzer->GetLogMessages();
   ActivityUserData::Snapshot global_data_snapshot =
       global_analyzer->GetGlobalUserDataSnapshot();
   ThreadActivityAnalyzer* thread_analyzer = global_analyzer->GetFirstAnalyzer();
   if (log_messages.empty() && global_data_snapshot.empty() &&
       !thread_analyzer) {
     return DEBUG_FILE_NO_DATA;
   }
 
@@ skipping 27 matching lines @@
     CollectThread(thread_analyzer->activity_snapshot(), thread_state);
   }
 
   // Collect module information.
   CollectModuleInformation(global_analyzer->GetModules(), process_state);
 
   return SUCCESS;
 }
 
 }  // namespace browser_watcher