Postmortem report collection: validate internal state

components/browser_watcher/postmortem_report_extractor.cc

Index: components/browser_watcher/postmortem_report_extractor.cc
diff --git a/components/browser_watcher/postmortem_report_extractor.cc b/components/browser_watcher/postmortem_report_extractor.cc
index bd12a9a03b4b87f9892195c6a3806bd1d6fce629..3a150b44e0241c11fc49eafbfef73e900b25d3db 100644
--- a/components/browser_watcher/postmortem_report_extractor.cc
+++ b/components/browser_watcher/postmortem_report_extractor.cc
@@ -11,6 +11,8 @@
 
 #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"
@@ -213,6 +215,16 @@ CollectionStatus Extract(const base::FilePath& 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 =