964636 - (about service) implement about:service framework core

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Alphan Chen [:alchen]]

It is the bug for "[User Story] About Service".
In this bug, we focus on gecko implementation.

Comment 1

[Alphan Chen [:alchen]]

Attachment: Implement "about:service" framework core

Comment 2

[Alphan Chen [:alchen]]

Attachment: Test sample for about service

Comment 3

[Alphan Chen [:alchen]]

About this feature, I reference the implementation of memory reporter.
With this feature, people can add a "status reporter" and dump the status easily.

There is a docs for this patch.
https://docs.google.com/presentation/d/15-aq5G4xcZZK6uaFwA0kY3bSHCz-ZnY_9lXiXXPXTG4/edit?usp=sharing

bug-964636-fix.patch :
It is the patch I'm working on.

bug-964636-test-sample.patch :
It is a test sample.
In this sample, I added a status reporter in AdjustSystemClock() (@hal/gonk/GonkHal.cpp)

Test procedure:
1. Adjust time in setting.
2. Use adb shell to trigger the dump.
   adb shell "echo 'status report' > /data/local/status_reporter_trigger"
3. The dumps are in /data/local/tmp/status-reports/.
   status-reports-8365-1.json   => the first time
   status-reports-8365-2.json   => the second time
4. Output
(status-reports-8365-1.json)
  [Sysdump Report Start]: 
    {"Process": "PID 8365", "Reporter name": "StatusReporter State", "Status Description": "Running:
There are 2 reporters"}
    {"Process": "PID 8365", "Reporter name": "The state of adjust time", "Status Description": "Finish
"}

Comment 4

[[:fabrice] Fabrice Desré]

How is that different from what we have for memory reports and telemetry?

Comment 5

[Alphan Chen [:alchen]]

Hi Fabrice,
this feature is similar to "dumpsys" command on Android.
This command dumps interesting information about the status of system services.
So we think it would be helpful if we have this functionality.
For example, if we got some BT problems, we can check the dump to see the status of BT module at that time.

Comment 6

[Alphan Chen [:alchen]]

Besides, module owner need to add status reporter to report useful information just like memory reporter.
This bug just made a easy way to add a reporter and collect the reports.

Comment 7

[Alphan Chen [:alchen]]

Comment on attachment 8366474 [details] [diff] [review]
Implement "about:service" framework core

Hi Dave,
could you give some feedback about this feature and this patch?

Alan said that you two had a discussion for this feature before.
At that time, the conclusion is the feature can be accomplished by similar implementation as memory reporter.

In this patch, we have a StatusReporterManager and several macro to simplify the effort of add reporters and collect reports.
I also upload a sample for adding a reporter.

Comment 8

[Marco Chen [:mchen]]

(In reply to Fabrice Desré [:fabrice] from comment #4)
> How is that different from what we have for memory reports and telemetry?

This is not related to memory usage (memory reports) and not used to collect performance data.
The general purpose is used to dump current state of services.

ex: 
1. Partner may meet a bug about audio routing is wrong and he didn't record the log at that moment.
   Without to reproduce it again, the "about:service" will dump current output devices status and SCO/A2DP status from AudioManager. Then we can know whether Gecko didn't handle it or audio hal didn't handle well.

2. When partner reported audio competing issues, he can dump the status from AudioChannelService to show each monitored media element's info. ex: visibility, audio channel type and play status. Then we can judge the root cause.

3. PowerManagerService can dump the wakelock status so when we met power issue we can make sure which wakelock name caused this issue.

Comment 9

[Dave Hylands [:dhylands]]

Comment on attachment 8366474 [details] [diff] [review]
Implement "about:service" framework core

Review of attachment 8366474 [details] [diff] [review]:
-----------------------------------------------------------------

I think that a huge portion of this is common with the memory status reporting and that this should be factored out and shared.

::: xpcom/base/nsIStatusReporter.idl
@@ +32,5 @@
> +
> +  /*
> +   * Return an enumerator of nsIStatusReporters that are currently registered.
> +   */
> +  nsISimpleEnumerator enumerateReporters ();

nit: no space before opening parenthesis (applies to rest of this file as well)

::: xpcom/base/nsStatusReporterManager.cpp
@@ +6,5 @@
> +#include "nsCOMPtr.h"
> +#include "nsDirectoryServiceDefs.h"
> +#include "nsDirectoryServiceUtils.h"
> +#include "nsServiceManagerUtils.h"
> +#include "nsStatusReporterManager.h"

nit: #include nsStatusReporterManager.h as the very first #include. This will ensure that nsStatusReporterManager.h #includes all of the files needed for it to compile.

@@ +90,5 @@
> +}
> +
> +NS_STATUS_REPORTER_IMPLEMENT(StatusReporter, "StatusReporter State", getStatus)
> +
> +class FifoWatcher : public MessageLoopForIO::Watcher

So since we're using the exact same mechanisms for trigger memory reports as these, so we should factor all of that code so that the entire triggering mechanism is shared between memorey reports and this module.

@@ +91,5 @@
> +
> +NS_STATUS_REPORTER_IMPLEMENT(StatusReporter, "StatusReporter State", getStatus)
> +
> +class FifoWatcher : public MessageLoopForIO::Watcher
> +		, public nsIObserver

nit: indentation

@@ +145,5 @@
> +      return -1;
> +
> +    if (unlink(path.get())) {
> +      LOG("FifoWatcher::OpenFifo unlink failed; errno=%d.  "
> +          "Continuing despite error.", errno);

It seems to me that the normal case should be that the unlink will fail (due to the file not existing) so we shouldn't be reporting here.

@@ +316,5 @@
> +    } while (0)
> +
> +static nsresult
> +DumpReport(nsIFileOutputStream *aOStream, const nsACString &aProcess,
> +           const nsACString &aName, const nsACString &aDescription)

nit: move & and * left. i.e. use "nsIFileOutputStream* aOStream" not "nsIFileOutputStream *aOStream"

This applies to all of the code

@@ +319,5 @@
> +DumpReport(nsIFileOutputStream *aOStream, const nsACString &aProcess,
> +           const nsACString &aName, const nsACString &aDescription)
> +{
> +  unsigned pid;
> +  if (!aProcess.IsEmpty()) {

Please use poisitive logic. Use: if (aProcess.IsEmpty()) and swap the if/else blocks.

::: xpcom/base/nsStatusReporterManager.h
@@ +40,5 @@
> +};
> +
> +#define NS_STATUS_REPORTER_MANAGER_CID \
> +{ 0xe8eb4e7e, 0xf2cf, 0x45e5, \
> +{ 0xb8, 0xa4, 0x6a, 0x0f, 0x50, 0x18, 0x84, 0x63 } }

I'd be inclined to put this in the .idl file. Otherwise people will update the .idl file and forget to update this.

Comment 10

[Alphan Chen [:alchen]]

Attachment: part1. Move some common classes and functions out of nsMemoryInfoDump.cpp

Hi Nicholas and Dave,
The goal of this patch is try to factored out common code into nsDumpUtils.h/cpp.
We can use this common code in different place.
Please give me some feedback about this patch.
Thanks a lot.


[nsDumpUtils.h/cpp]
1. Add a class called nsDumpUtils. 
There is a static function (OpenTempFile) in it. 
I add a foldername parameter (default as EmptyCString) in the implementation of this function.

2. For FifoWatcher class, the major part is the same.
There is a new private static member variable.(static nsRefPtr<FifoWatcher> fw)
I add a checking in MaybeCreate(). 
With this modification, we will only new the class once.

-----------------modification-------------------------------
  // The FifoWatcher is held alive by the observer service.
  if (!FifoWatcher::fw) {
    FifoWatcher::fw = new FifoWatcher();
    FifoWatcher::fw->Init();
  }
-------------------------------------------------------------

3. For FdWatcher and SignalPipeWatcher, they are the same as before.

4. For "GCAndCCLogDumpRunnable" and "DumpStatusInfoToTempDirRunnable", they are the same as before. Since this two runnable are used by nsMemoryInfoDumper.cpp, I put these two runnable in headerfile.

[nsMemoryInfoDumper.cpp]
1. Include "nsDumpUtils.h"
2. Factored out common code.
3. Replace "nsMemoryInfoDumper::OpenTempFil" into "nsDumpUtils::OpenTempFile" and add one more parameter.

[nsCycleCollector.cpp]
1. Include "nsDumpUtils.h"
2. Replace "nsMemoryInfoDumper::OpenTempFil" into "nsDumpUtils::OpenTempFile" and add one more parameter.

Comment 11

[Alphan Chen [:alchen]]

Attachment: part2. Implement "about:service" framework

Hi Dave,
This patch is base on " 8376077: part1. Move some common classes and functions out of nsMemoryInfoDump.cpp". 
Status-reporter can use common code and trigger mechanism with memory reporter.
1. adb shell "echo 'status reporter' > debug_info_trigger"
2. the output file(status-reports-$pid-$num.json) will exist in /data/local/tmp/status-reports.

In "nsStatusReporterManager::Init()", in order to create Fifowatcher we will call "FifoWatcher::MaybeCreate();". With the first patch, we will only new the class once.

Comment 12

[Alphan Chen [:alchen]]

Comment on attachment 8366475 [details] [diff] [review]
Test sample for about service

We can still use this patch as a test sample.

Comment 13

[Nicholas Nethercote [inactive]]

Comment on attachment 8376081 [details] [diff] [review]
part2. Implement "about:service" framework

Review of attachment 8376081 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/base/nsIStatusReporter.idl
@@ +20,5 @@
> +   * has "" in this field, indicating that it applies to the current process.
> +   */
> +  readonly attribute ACString process;
> +  /*
> +   * A human-readable staus description.

typo: staus should be status

::: xpcom/base/nsStatusReporterManager.cpp
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 50; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Use the proper modelines: https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Mode_Line

::: xpcom/base/nsStatusReporterManager.h
@@ +34,5 @@
> +  nsStatusReporterManager();
> +  virtual ~nsStatusReporterManager();
> +
> +private:
> +  nsCOMArray<nsIStatusReporter>      sReporters;

This should be called |mReporters|, because it's not static.

Comment 14

[Nicholas Nethercote [inactive]]

> It is the bug for "[User Story] About Service".

What is this user story? Is it documented somewhere? I've skimmed the patches but don't understand the high-level idea.

Comment 15

[Alphan Chen [:alchen]]

> > It is the bug for "[User Story] About Service".
> 
> What is this user story? Is it documented somewhere? I've skimmed the
> patches but don't understand the high-level idea.


Hi Nicholas,
there is no document yet.
The main idea is from "dumpsys" command on Android.

In Comment 8,
The general purpose is used to dump current state of services.

ex: 
1. Partner may meet a bug about audio routing is wrong and he didn't record the log at that moment.
   Without to reproduce it again, the "about:service" will dump current output devices status and SCO/A2DP status from AudioManager. Then we can know whether Gecko didn't handle it or audio hal didn't handle well.

2. When partner reported audio competing issues, he can dump the status from AudioChannelService to show each monitored media element's info. ex: visibility, audio channel type and play status. Then we can judge the root cause.

3. PowerManagerService can dump the wakelock status so when we met power issue we can make sure which wakelock name caused this issue.(In reply to Nicholas Nethercote [:njn] from comment #14)

Comment 16

[Dave Hylands [:dhylands]]

Comment on attachment 8376077 [details] [diff] [review]
part1. Move some common classes and functions out of nsMemoryInfoDump.cpp

Review of attachment 8376077 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/base/nsDumpUtils.cpp
@@ +2,5 @@
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

This is a huge step in the right direction.

To be truly generic, this code shouldn't know about the memory dumper/services.

Instead, they should register callbacks with this code to indicate that a given signal/command was received.

Factoring this code to be truly generic might be over-engineering, since there are only 2 consumers of this information, so I'm happy with this the way it is. I just thought I'd bring it up.

::: xpcom/base/nsDumpUtils.h
@@ +111,5 @@
> +
> +  virtual void OnFileCanReadWithoutBlocking(int aFd);
> +
> +private:
> +  static nsRefPtr<FifoWatcher> fw;

since this is static you should probably use StaticRefPtr, and call the member varable sFw;

@@ +178,5 @@
> +  const bool mMinimizeMemoryUsage;
> +  const bool mDumpChildProcesses;
> +};
> +
> +class GCAndCCLogDumpRunnable : public nsRunnable

This particular class is specific to the memory dumper right? In which case, it should stay in the nsMemoryInfoDumper.cpp file (if you decide to make DUmpUtils generic)

Comment 17

[Dave Hylands [:dhylands]]

Comment on attachment 8376081 [details] [diff] [review]
part2. Implement "about:service" framework

Review of attachment 8376081 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/base/nsStatusReporterManager.cpp
@@ +197,5 @@
> +  rv = ostream->Close();
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // Rename the status reports file
> +  nsCOMPtr<nsIFile> mrFinalFile;

Why is this called mrFinalFile? Doe mr mean memory report? This for service info right?

::: xpcom/base/nsStatusReporterManager.h
@@ +15,5 @@
> +
> +  nsStatusReporter(nsACString& process,
> +                   nsACString& desc);
> +
> +  ~nsStatusReporter();

add virtual (since this is a virtual destructor)

Comment 18

[Nicholas Nethercote [inactive]]

Comment on attachment 8376077 [details] [diff] [review]
part1. Move some common classes and functions out of nsMemoryInfoDump.cpp

I think dhylands' f+ is sufficient here. I had a quick look through and it seems reasonable.

Comment 19

[Alphan Chen [:alchen]]

Attachment: part1. Refactor nsMemoryInfoDumper (move common classes and functions into nsDumpUtils)

In this patch, I made nsDumpUtils more generic.

1.
For this purpose, I introduced "RegisterCallback" function call into FifoWatcher and SignalPipeWatcher.
Consumers(memoryInfoDump and statusreporter for now) need to register their callbacks right after they create the watcher.

@FifoWatcher
  static void RegisterCallback(const nsCString& aCommand,
                               FifoCallback aCallback);
@SignalPipeWatcher
  static void RegisterCallback(const uint8_t aSignal, 
                               PipeCallback aCallback);

2. 
I modify the return type of FifoWatcher::MaybeCreate() (from void to bool)
By the return value, user can know whether the fifo is created sucessfully.
If fifo does not exist, we don't need to register the callback.

3.
By the original design, we need to register signal handler of the signal we want to monitor as DumpAboutMemorySignalHandler.
I add a function "RegisterSignalHandler" into class SignalPipeWatcher to fit the design now.

Usage: 
       RegisterSignalHandler(const uint8_t aSignal)
       aSignal is set 0 by default. 
         (register all signals in "static nsTArray<uint8_t> sSignals")
       Other, register given aSignal only.

For now, we will call RegisterSignalHandler() (register all signal) when doing OpenFd().
Besides, we will call RegisterSignalHandler(aSignal) (register aSignal) in SignalPipeWatcher::RegisterCallback as well.

Comment 20

[Alphan Chen [:alchen]]

Attachment: part2. Implement the "about:service" framework core based on the latest nsDumpUtils design

The usage is still the same as before.

Status-reporter can use common code and trigger mechanism with memory reporter.
1. adb shell "echo 'status reporter' > debug_info_trigger"
2. the output file(status-reports-$pid-$num.json) will exist in /data/local/tmp/status-reports.

In "nsStatusReporterManager::Init()", in order to create Fifowatcher we will call "FifoWatcher::MaybeCreate();".
If FifoWatcher exists, we will register command and callback for dumping reports.

@
nsStatusReporterManager::Init()
{
      ....
  if (FifoWatcher::MaybeCreate()) {
    FifoWatcher::RegisterCallback(NS_LITERAL_CSTRING("status report"),
                                  doStatusReport);
  }
      ....
}

Comment 21

[Dave Hylands [:dhylands]]

Comment on attachment 8382837 [details] [diff] [review]
part1. Refactor nsMemoryInfoDumper (move common classes and functions into nsDumpUtils)

Review of attachment 8382837 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay on the review, I had a couple of high priority bugs come up last week.

Overall this looks really good. A few minor things that need to be fixed up.

::: xpcom/base/nsDumpUtils.cpp
@@ +18,5 @@
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +
> +/*
> + * The following code supports dumping about:memory upon receiving a signal.

Since this is genericized now, it should probably talk about triggering a registered callback (and perhaps mention about:memory as an example).

@@ +36,5 @@
> + */
> +
> +// This is the write-end of a pipe that we use to notice when a
> +// dump-about-memory signal occurs.
> +static Atomic<int> sDumpAboutMemoryPipeWriteFd(-1);

Should be renamed since this is no longer specific to about::memory

Perhaps just sDumpPipeWriteFd?

@@ +39,5 @@
> +// dump-about-memory signal occurs.
> +static Atomic<int> sDumpAboutMemoryPipeWriteFd(-1);
> +
> +void
> +DumpAboutMemorySignalHandler(int aSignum)

Should be renamed since it might not just be used for about::memory.

Perhaps just DumpSignalHandler. It should probably also be declated static.

::: xpcom/base/nsDumpUtils.h
@@ +109,5 @@
> +
> +private:
> +  static StaticRefPtr<FifoWatcher> sFw;
> +  static nsTArray<nsCString> sCommands;
> +  static nsTArray<FifoCallback> sCallbacks;

Similar to my comment I made a bit later, since the elements of sCommands and sCallbacks are tightly coupled, you should create a struct and have an array of structs.

@@ +140,5 @@
> +  virtual void OnFileCanReadWithoutBlocking(int aFd);
> +
> +private:
> +  static nsTArray<uint8_t> sSignals;
> +  static nsTArray<PipeCallback> sCallbacks;

Since sSignals and sCallbacks are tightly coupled, I prefer to see a struct containing the 2 members, and have a single array of structs, rather than having 2 arrays.

struct SignalInfo {
  uint8_t mSignal;
  PipeCallback mCallback;
};
nsTArray<SignalInfo> sSignalInfo;

Then you use sSignalInfo[i].mSignal and sSignalInfo[i].mCallback;

::: xpcom/base/nsMemoryInfoDumper.cpp
@@ -122,5 @@
> - *  - SIGRTMIN:     Dump our memory reporters (and those of our child
> - *                  processes),
> - *  - SIGRTMIN + 1: Dump our memory reporters (and those of our child
> - *                  processes) after minimizing memory usage, and
> - *  - SIGRTMIN + 2: Dump the GC and CC logs in this and our child processes.

This seems like useful documentation (What the various signals do). I think it should be preserved.

Comment 22

[Dave Hylands [:dhylands]]

Comment on attachment 8382840 [details] [diff] [review]
part2. Implement the "about:service" framework core based on the latest nsDumpUtils design

Review of attachment 8382840 [details] [diff] [review]:
-----------------------------------------------------------------

Just some nits.

Otherwise, this looks good.

::: xpcom/base/nsStatusReporterManager.cpp
@@ +20,5 @@
> +#else
> +#include <unistd.h>
> +#endif
> +
> +#if defined(XP_LINUX) || defined(__FreeBSD__) // {

nit: I prefer to see "feature" based #if's

So do something like this instead:

#if defined(XP_LINUX) || defined(__FreeBSD__)
#define DO_STATUS_REPORT 1
#else
#define DO_STATUS_REPORT 0
#endif

And then use

#if DO_STATUS_REPORT

rather than some complicated #if involving OS's etc. It makes the code easier to read, and easier to maintain down the road.

@@ +47,5 @@
> +  NS_DispatchToMainThread(runnable);
> +}
> +
> +} //anonymous namespace
> +#endif // XP_LINUX }

nit: The comment doesn't match the #if (it's missing __FreeBSD__ )

@@ +50,5 @@
> +} //anonymous namespace
> +#endif // XP_LINUX }
> +
> +static bool status_report_progress = 0;
> +static int num_reporters = 0;

nit: Since these are globals, they should start with g

Also camel case seems to be prefered over underscores (gStatusReportProgress and gNumReporters)

https://developer.mozilla.org/en/docs/Developer_Guide/Coding_Style#Prefixes

@@ +76,5 @@
> +    } while (0)
> +
> +static nsresult
> +DumpReport(nsIFileOutputStream* aOStream, const nsACString& aProcess,
> +           const nsACString& aName, const nsACString& aDescription)

nit: Since the caller already has aProcess, aName, and aDescription as nsCStrings I think it would be better to pass these as const nsCString & and then you can call .get() and not have to make copies inside the function.

@@ +78,5 @@
> +static nsresult
> +DumpReport(nsIFileOutputStream* aOStream, const nsACString& aProcess,
> +           const nsACString& aName, const nsACString& aDescription)
> +{
> +  unsigned pid;

nit: getpid returns pid_t not unsigned.

@@ +119,5 @@
> +NS_IMETHODIMP
> +nsStatusReporterManager::Init()
> +{
> +#define REGISTER(_x)  RegisterReporter(new NS_STATUS_REPORTER_NAME(_x))
> +  REGISTER(StatusReporter);

nit: This seems to be used exactly once. Why bother with the REGISTER macro?

Comment 23

[Alphan Chen [:alchen]]

Attachment: part1. Refactor nsMemoryInfoDumper (move common classes and functions into nsDumpUtils)

Hi Dave,
I refined the patch.

The biggest modification is I add two structures called SignalInfo and FifoInfo.
struct SignalInfo {
  uint8_t mSignal;
  PipeCallback mCallback;
};

struct FifoInfo {
  nsCString mCommand;
  FifoCallback mCallback;
};

Besides, registercallback function use them as parameter.
FifoWatcher::RegisterCallback(const FifoInfo &aFifoInfo);
SignalPipeWatcher::RegisterCallback(const SignalInfo &aSignalInfo);

There are other modification based on your comment.
Thanks.

Comment 24

[Alphan Chen [:alchen]]

Attachment: part2. Implement the "about:service" framework core based on the 0311 nsDumpUtils design

Made some modification based on the latest nsDumpUtils design and comment 22.

Comment 25

[Alphan Chen [:alchen]]

Hi Dave,
Since Attachment #8389090 [details] [diff] depends on Attachment #8389089 [details] [diff], I think it will be great that you have double check it.
Thanks.

Comment 26

[Dave Hylands [:dhylands]]

Comment on attachment 8389089 [details] [diff] [review]
part1. Refactor nsMemoryInfoDumper (move common classes and functions into nsDumpUtils)

Review of attachment 8389089 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the B2G_DEBUG=1 build tested

::: xpcom/base/nsDumpUtils.h
@@ +135,5 @@
> +  static void RegisterSignalHandler(const uint8_t aSignal = 0);
> +
> +  virtual ~SignalPipeWatcher()
> +  {
> +    MOZ_ASSERT(sDumpAboutMemoryPipeWriteFd == -1);

This failed to compile for me, even after renaming.

This suggests to me that this wasn't ever tested while B2G_DEBUG=1.

I highly recommend that you always build and test with B2G_DEBUG=1

You need to move the destructor into the .cpp file.

Comment 27

[Alphan Chen [:alchen]]

Attachment: part1(ver0318). Refactor nsMemoryInfoDumper (move common classes and functions into nsDumpUtils)

Hi Dave,
I refined the patch again.
The following is the change list of this patch compared to previous patch.

1. Add the following #ifndef/#define/#endif into nsDumpUtils.h to prevent double include case.
#ifndef mozilla_nsDumpUtils_h
#define mozilla_nsDumpUtils_h
#endif

2. Let FifoWatcher and SignalPipeWatcher be Singleton.

3. Let FifoWatcher::mFifoInfo and SignalPipeWatcher::mSignalInfo be prviate. (They are static in previous patch).

4. Move the destructor into the .cpp file

Comment 28

[Alphan Chen [:alchen]]

Attachment: part2(ver0318). Implement the "about:service" framework core based on the 0311 nsDumpUtils design

Base on attachment 8393283 [details] [diff] [review], here is the corresponding patch.

Try server result for these two patches:
https://tbpl.mozilla.org/?tree=Try&rev=9376ed560b3d

Comment 29

[Dave Hylands [:dhylands]]

Comment on attachment 8393284 [details] [diff] [review]
part2(ver0318). Implement the "about:service" framework core based on the 0311 nsDumpUtils design

Review of attachment 8393284 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with issues addressed.

::: xpcom/base/nsStatusReporterManager.cpp
@@ +170,5 @@
> +  DUMP(ostream, "  [Sysdump Report Start]: ");
> +
> +  nsCOMPtr<nsISimpleEnumerator> e;
> +  bool more;
> +  EnumerateReporters(getter_AddRefs(e));

nit: EnumerateReporters can fail. Please check return code.

@@ +178,5 @@
> +    nsCOMPtr<nsIStatusReporter> r = do_QueryInterface(supports);
> +
> +    nsCString process;
> +    rv = r->GetProcess(process);
> +    NS_ENSURE_SUCCESS(rv, rv);

nit: This particular macro has been deprecated. See bug 672843 and
https://groups.google.com/forum/#!topic/mozilla.dev.platform/1clMLuuhtWQ

Comment 30

[Dave Hylands [:dhylands]]

Comment on attachment 8393283 [details] [diff] [review]
part1(ver0318). Refactor nsMemoryInfoDumper (move common classes and functions into nsDumpUtils)

Review of attachment 8393283 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with issues addressed.

I'd also like to see a green try run (with debug builds included), and I'd like you to verify that tools/get_about_memory.py works the same before and after this patch is applied (I verified this myself with the previous patch, I just want to make sure that you do this before requesting checking-needed.

::: xpcom/base/nsDumpUtils.cpp
@@ +119,5 @@
> +
> +/* static */ void
> +SignalPipeWatcher::RegisterCallback(const SignalInfo &aSignalInfo)
> +{
> +  for (PRUint32 i = 0 ; i < SignalPipeWatcher::mSignalInfo.Length(); i++) {

nit: I believe that we shouldn't be using PRUint32 in new code. See bug 579517

And even better, is that nsTArray<SignalInfo>::index_type and nsTArray<SignalInfo>::size_type would be preferred.

I normally create a typedef like:

typedef nsTArray<SignalInfo> SignalInfoArray;

and then using SignalInfoArray to declare the array, and SignalInfoArray::index_type and SignalInfoArray::size_type as appropriate.

@@ +153,5 @@
> +}
> +
> +SignalPipeWatcher::~SignalPipeWatcher()
> +{
> +  MOZ_ASSERT(sDumpPipeWriteFd == -1);

Since you've made this be a singleton with ClearOnShutdown, it seems to me that scenarios might exist where this assert would fire.

Wouldn't it just be better to close the file descriptor if its open?

Even better, would be that this file descriptor should really be something like a ScopedFd. I suspect that because its an Atomic<int> and not just an int, that you'll need to create something like: ScopedCloseAtomic.
See http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/FileUtils.h#38 for details.

@@ +154,5 @@
> +
> +SignalPipeWatcher::~SignalPipeWatcher()
> +{
> +  MOZ_ASSERT(sDumpPipeWriteFd == -1);
> +  SignalPipeWatcher::mSignalInfo.Clear();

nit: This line isn't required (nsTArray does this as part of its destructor)

@@ +268,5 @@
> +}
> +
> +FifoWatcher::~FifoWatcher()
> +{
> +  FifoWatcher::mFifoInfo.Clear();

nit: This line isn't required (nsTArray already does this in its own destructor).

@@ +269,5 @@
> +
> +FifoWatcher::~FifoWatcher()
> +{
> +  FifoWatcher::mFifoInfo.Clear();
> +  FifoWatcher::sSingleton = 0;

nit: This line isn't needed (ClearOnShutdown clears sSingleton).

If you were going to keep it, then assigning nullptr would be better than 0, since sSingleton is a pointer and not an int.

::: xpcom/base/nsMemoryInfoDumper.cpp
@@ +188,5 @@
> +
> +  // Dump memory reporters (and those of our child processes)
> +  sDumpAboutMemorySignum = SIGRTMIN;
> +  sw->RegisterCallback( { sDumpAboutMemorySignum,
> +                          doMemoryReport } );

nit: This looks weird to me. I understand what you're doing, I just don't think it buys you anything.

I think having RegisterCallback take 2 arguments would look "normal".

Comment 31

[Dave Hylands [:dhylands]]

Alphan - you should take a look at bug 985736

We will need to address that issue here, if it isn't already being dealt with.

Comment 32

[Dave Hylands [:dhylands]]

Looks like bug 980419 has a patch. So you should probably incorporate that patch into your work.

Comment 33

[Alphan Chen [:alchen]]

Attachment: part1(ver0324). Refactor nsMemoryInfoDumper (move common classes and functions into nsDumpUtils), r=dhylands

This modification patch is based on comment 30.

For "sDumpPipeWriteFd", I keep original design.
It will be closed if its open in "~SignalPipeWatcher".

Comment 34

[Alphan Chen [:alchen]]

Attachment: part2(ver0324). Implement the "about:service" framework core, r=dhylands

The patch is based on comment 29 and Attachment #8396149 [details] [diff].

Comment 35

[Alphan Chen [:alchen]]

Comment on attachment 8393284 [details] [diff] [review]
part2(ver0318). Implement the "about:service" framework core based on the 0311 nsDumpUtils design

Use attachment 8396150 [details] [diff] [review] to replace this one.

Comment 36

[Alphan Chen [:alchen]]

These two patches are based on the latest code base.
(including the patch of bug 980419)

The following is the try server result.
https://tbpl.mozilla.org/?tree=Try&rev=2804a404f815
There are some intermittent errors.
However, they are not caused by this patch.

Comment 37

[Alphan Chen [:alchen]]

(In reply to Dave Hylands [:dhylands] from comment #30)
> 
> Review of attachment 8393283 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with issues addressed.
> 
> I'd also like to see a green try run (with debug builds included), and I'd
> like you to verify that tools/get_about_memory.py works the same before and
> after this patch is applied (I verified this myself with the previous patch,
> I just want to make sure that you do this before requesting checking-needed.
> 

I have tested all the way to get reports.
1. get_about_memory.py
2. killer SIGNUM B2G_PID
3. echo "command" > debug_info_trigger
   (command including: i.   memory report
                       ii.  minimize memory report
                       iii. gc log
                       iv.  abbreviated gc log
                       v.   status report

Comment 38

[Dave Hylands [:dhylands]]

Excellent work

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/5643caf4797f
https://hg.mozilla.org/integration/b2g-inbound/rev/5e337eb07938

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5643caf4797f
https://hg.mozilla.org/mozilla-central/rev/5e337eb07938