1194555 - Asynchronously invoke registered memory reporters

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Eric Rahm [:erahm]]

When requesting a memory report it would be useful to have each reporter called asynchronously.

This will allow us to implement reporters that need to run on other threads (such as in media, audio, JS workers). It would also mean that measuring memory would not lock up the main thread (at least to the degree it currently does).

The proposed implementation would be similar to how our current multiprocess support works. We would dispatch memory report requests to the event loop, once all reporters have reported back or a timeout is reached we will invoke the memoryReportFinished callback.

Comment 1

[Eric Rahm [:erahm]]

Attachment: Part 0: Cleanup GetReportsState constructor

Move GetReportsState ctor to the impl so that mChildrenPending doesn't have
to be heap allocated.

Comment 2

[Eric Rahm [:erahm]]

Attachment: Part 1: Add option for async memory reporters

Comment 3

[Eric Rahm [:erahm]]

Attachment: Part 2: Run reporters asynchronously

Comment 4

[Eric Rahm [:erahm]]

Comment on attachment 8648519 [details] [diff] [review]
Part 1: Add option for async memory reporters

I took the approach of adding an async version of register(Weak|String)Reporter here.

I think there's merit to having two versions, primarily that we don't have to update all the existing reporters to be async. Nick, any thoughts on this approach (not looking for review of style/typos/etc, just design side).

Comment 5

[Eric Rahm [:erahm]]

Comment on attachment 8648520 [details] [diff] [review]
Part 2: Run reporters asynchronously

Here I add a new struct to track the internal state per-process (rather than the existing multi-process state tracker that only exists in the parent process). This actually works in non-e10s builds.

e10s is currently broken (we need to add some sort of "reporting done" callback). This also breaks the |getReportsForThisProcess| IDL interface (it will return immediately), but I think that's unavoidable if we're going to have async reporters. We *could* keep a version in place that just doesn't call async reporters, but I'm not sure that's a great idea.

Aside from e10s not working, do you have any thoughts on the overall design (again not looking for review of style/typos/etc, just design side). I just want to make sure this is going in the right direction.

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 8648519 [details] [diff] [review]
Part 1: Add option for async memory reporters

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

Sure, seems fine.

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 8648520 [details] [diff] [review]
Part 2: Run reporters asynchronously

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

This generally seems fine..

Changing IDL signatures is fine; make whatever changes are necessary.

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +1468,5 @@
> +                            mCollectReportsState->mHandleReportData,
> +                            mCollectReportsState->mAnonymize);
> +  if (!aIsAsync) {
> +    EndReport(aReporter);
> +  }

So...

- All reporters are now being run via runnables. Ok.

- EndReport() is called here for synchronous reporters. Presumably async reporters have to call EndReport() themselves? (Hmm, if a reporter calls EndReport() more than once that'll cause havoc. Not sure if it's worth worrying about that or not.)

@@ +1516,5 @@
>    return NS_OK;
>  }
>  
> +void
> +nsMemoryReporterManager::EndReport(nsIMemoryReporter* aReporter)

|aReporter| is unused. Just remove it?

Comment 8

[Nicholas Nethercote [inactive]]

BTW, I assume you're making the MP4 reporter async in order to test this. When the time comes to review, seeing this bug's patches and the updated MP4 reporter patch at the same time will make it easier to understand how all this works.

Comment 9

[Eric Rahm [:erahm]]

Attachment: Part 2: Run reporters asynchronously

Jed, can you take a look at the IPC changes (or more if you'd like)?

Nick, this is pretty similar to the original draft.

Comment 10

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f791e286ef79

Comment 11

[Eric Rahm [:erahm]]

(In reply to Eric Rahm [:erahm] from comment #10)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f791e286ef79

It would appear I need to update one of the about:memory tests (and add a null check).

Comment 12

[Nicholas Nethercote [inactive]]

Comment on attachment 8648518 [details] [diff] [review]
Part 0: Cleanup GetReportsState constructor

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

::: xpcom/base/nsMemoryReporterManager.h
@@ -213,5 @@
>      nsCOMPtr<nsITimer>                   mTimer;
> -    // This is a pointer to an nsTArray because otherwise C++ is
> -    // unhappy unless this header includes ContentParent.h, which not
> -    // everything that includes this header knows how to find.
> -    nsTArray<nsRefPtr<mozilla::dom::ContentParent>>* mChildrenPending;

I take it this problem isn't an issue any more?

Comment 13

[Nicholas Nethercote [inactive]]

Comment on attachment 8648519 [details] [diff] [review]
Part 1: Add option for async memory reporters

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

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +2380,5 @@
> +  nsCOMPtr<nsIMemoryReporterManager> mgr =
> +    do_GetService("@mozilla.org/memory-reporter-manager;1");
> +  if (!mgr) {
> +    return NS_ERROR_FAILURE;
> +  }

These five lines are repeated often enough in this file that it's probably worth creating a macro for them.

Comment 14

[Nicholas Nethercote [inactive]]

Comment on attachment 8667531 [details] [diff] [review]
Part 2: Run reporters asynchronously

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

We now have GetReportsState, which holds state for the inter-process asynchrony, and CollectReportsState, which holds state for the intra-process asynchrony. There's a lot of duplication between them.

I see you did it this way because you can enter this code via GetReportsExtended(), which uses both of them, or just via GetReportsForThisProcessExtended(), which just uses the latter. It might be hard to share the data instead of duplicating it, but maybe when you create the CollectReportsState you could assert that the relevant fields match the existing GetReportsState (if there is one).

Also the names GetReportsState and CollectReportsState aren't great. Maybe InterProcessState and IntraProcessState would be better? Also, a short comment at the top of each struct explaining their overlap, and why it's necessary, would be good.

::: xpcom/base/nsIMemoryReporter.idl
@@ +314,5 @@
> +  /*
> +   * Called by an asynchronous memory reporter upon completion.
> +   */
> +  [noscript] void
> +    endReport();

Nit: put the endReport() on the same line as the noscript. (getReportsForThisProcessExtended() follows a different pattern because it has such a long name plus long argument names.)

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +1513,5 @@
>  {
>    return GetReportsForThisProcessExtended(aHandleReport, aHandleReportData,
> +                                          aAnonymize, nullptr,
> +                                          nullptr, nullptr);
> +}

This is a bit lame. GetReportsForThisProcess should really get the extra arguments too. (Just as both GetReports() and GetReportsExtended() have the extra arguments.)

@@ +1556,5 @@
> +  mCollectReportsState =
> +      new CollectReportsState(aHandleReport, aHandleReportData,
> +                              aFinishReporting, aFinishReportingData,
> +                              aAnonymize, aDMDFile,
> +                              mGetReportsState ? mGetReportsState->mGeneration : 0);

There's no test if mCollectReportsState is non-null, i.e. we have a request already in flight. There probably should be one; mGetReportsState has one, and it just bails out in that case.

@@ +1590,5 @@
> +#ifdef MOZ_DMD
> +    if (mCollectReportsState->mDMDFile) {
> +      nsMemoryInfoDumper::DumpDMDToFile(mCollectReportsState->mDMDFile);
> +    }
> +#endif

Have you tested that DMD still does reasonable things? (DMD has tests but it wouldn't hurt to do a manual full-browser run as well.)

::: xpcom/base/nsMemoryReporterManager.h
@@ +229,5 @@
>                      nsIFinishReportingCallback* aFinishReporting,
>                      nsISupports* aFinishReportingData,
>                      const nsAString& aDMDDumpIdent);
>  
>      ~GetReportsState();

Might as well remove the destructor now that it's empty. Then it'll match CollectReportsState, which doesn't have one.

Comment 15

[Nicholas Nethercote [inactive]]

BTW, it generally looks pretty good. Much neater than I was expecting :)

Comment 16

[Eric Rahm [:erahm]]

(In reply to Nicholas Nethercote [:njn] from comment #12)
> Comment on attachment 8648518 [details] [diff] [review]
> Part 0: Cleanup GetReportsState constructor
> 
> Review of attachment 8648518 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/base/nsMemoryReporterManager.h
> @@ -213,5 @@
> >      nsCOMPtr<nsITimer>                   mTimer;
> > -    // This is a pointer to an nsTArray because otherwise C++ is
> > -    // unhappy unless this header includes ContentParent.h, which not
> > -    // everything that includes this header knows how to find.
> > -    nsTArray<nsRefPtr<mozilla::dom::ContentParent>>* mChildrenPending;
> 
> I take it this problem isn't an issue any more?

Correct, by moving the ctor into the impl we can avoid the issue.

Comment 17

[Eric Rahm [:erahm]]

Comment on attachment 8648519 [details] [diff] [review]
Part 1: Add option for async memory reporters

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

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +2380,5 @@
> +  nsCOMPtr<nsIMemoryReporterManager> mgr =
> +    do_GetService("@mozilla.org/memory-reporter-manager;1");
> +  if (!mgr) {
> +    return NS_ERROR_FAILURE;
> +  }

It looks like there's already a macro [1] for that, I'll move it up and use it.

[1] https://dxr.mozilla.org/mozilla-central/rev/891ee0d0ba3ec42b6484cf0205b3c95e21c58f74/xpcom/base/nsMemoryReporterManager.cpp?offset=0#2424-2429

Comment 18

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8667531 [details] [diff] [review]
Part 2: Run reporters asynchronously

The IPC part looks good.

I've read the rest of the patch, and nothing jumps out at me, but I'd need to spend more time with the rest of the patches to be sure I understand everything that's going on here.

Comment 19

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8a19db325ce

Comment 20

[Eric Rahm [:erahm]]

Attachment: Part 0: Cleanup GetReportsState constructor

Updated to remove the GetReportState dtor as well.

Comment 21

[Eric Rahm [:erahm]]

Attachment: Part 0-1: Remove RunReportersForThisProcess

RunReportersForThisProcess is no longer used and will no longer work once we have async reporters.

Comment 22

[Eric Rahm [:erahm]]

Attachment: Part 0-2: Remove |explicit| attribute from nsIMemoryReporterManager

The calculation of |explicit| relies on the synchronous |getReportsForThisProcess|, once we have asynchronous reporters this will no longer work. As it is currently referenced in the about::memory tests we can just remove it.

Comment 23

[Eric Rahm [:erahm]]

Attachment: Part 0-3: Remove |getReportsForThisProcess| from the nsIMemoryReporterManager interface

|getReportsForThisProcess| differs from |getReports| in that it is limited to current process and is synchronous. When asynchronous memory reporters are added the function will no longer be able tobe synchronous. There isn't much utility in only measuring the current process, so we can remove the function and switch existing users to |getReports|.

Comment 24

[Eric Rahm [:erahm]]

Comment on attachment 8669157 [details] [diff] [review]
Part 0: Cleanup GetReportsState constructor

Carrying forward r+

Comment 25

[Eric Rahm [:erahm]]

Attachment: Part 1: Add option for async memory reporters

Updated to use the |GET_MEMORY_REPORTER_MANAGER| macro.

Comment 26

[Eric Rahm [:erahm]]

Comment on attachment 8669175 [details] [diff] [review]
Part 1: Add option for async memory reporters

Carrying forward r+

Comment 27

[Eric Rahm [:erahm]]

(In reply to Nicholas Nethercote [:njn] from comment #14)
> Comment on attachment 8667531 [details] [diff] [review]
> Part 2: Run reporters asynchronously
> 
> Review of attachment 8667531 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We now have GetReportsState, which holds state for the inter-process
> asynchrony, and CollectReportsState, which holds state for the intra-process
> asynchrony. There's a lot of duplication between them.
> 
> I see you did it this way because you can enter this code via
> GetReportsExtended(), which uses both of them, or just via
> GetReportsForThisProcessExtended(), which just uses the latter. It might be
> hard to share the data instead of duplicating it, but maybe when you create
> the CollectReportsState you could assert that the relevant fields match the
> existing GetReportsState (if there is one).
> 
> Also the names GetReportsState and CollectReportsState aren't great. Maybe
> InterProcessState and IntraProcessState would be better? Also, a short
> comment at the top of each struct explaining their overlap, and why it's
> necessary, would be good.

Renaming is probably the most straightforward solution at this point (and adding some more comments / extra assertions). I'll do the |GetReportsState| rename as another cleanup patch and update Part 2 to use a different name for |CollectReportsState|.
 
> ::: xpcom/base/nsIMemoryReporter.idl
> @@ +314,5 @@
> > +  /*
> > +   * Called by an asynchronous memory reporter upon completion.
> > +   */
> > +  [noscript] void
> > +    endReport();
> 
> Nit: put the endReport() on the same line as the noscript.
> (getReportsForThisProcessExtended() follows a different pattern because it
> has such a long name plus long argument names.)

Will update.

> 
> ::: xpcom/base/nsMemoryReporterManager.cpp
> @@ +1513,5 @@
> >  {
> >    return GetReportsForThisProcessExtended(aHandleReport, aHandleReportData,
> > +                                          aAnonymize, nullptr,
> > +                                          nullptr, nullptr);
> > +}
> 
> This is a bit lame. GetReportsForThisProcess should really get the extra
> arguments too. (Just as both GetReports() and GetReportsExtended() have the
> extra arguments.)

|GetReportsForThisProcess| has been removed in Part 0-3.

> 
> @@ +1556,5 @@
> > +  mCollectReportsState =
> > +      new CollectReportsState(aHandleReport, aHandleReportData,
> > +                              aFinishReporting, aFinishReportingData,
> > +                              aAnonymize, aDMDFile,
> > +                              mGetReportsState ? mGetReportsState->mGeneration : 0);
> 
> There's no test if mCollectReportsState is non-null, i.e. we have a request
> already in flight. There probably should be one; mGetReportsState has one,
> and it just bails out in that case.

Will add the check.

> @@ +1590,5 @@
> > +#ifdef MOZ_DMD
> > +    if (mCollectReportsState->mDMDFile) {
> > +      nsMemoryInfoDumper::DumpDMDToFile(mCollectReportsState->mDMDFile);
> > +    }
> > +#endif
> 
> Have you tested that DMD still does reasonable things? (DMD has tests but it
> wouldn't hurt to do a manual full-browser run as well.)

It still works :)

> ::: xpcom/base/nsMemoryReporterManager.h
> @@ +229,5 @@
> >                      nsIFinishReportingCallback* aFinishReporting,
> >                      nsISupports* aFinishReportingData,
> >                      const nsAString& aDMDDumpIdent);
> >  
> >      ~GetReportsState();
> 
> Might as well remove the destructor now that it's empty. Then it'll match
> CollectReportsState, which doesn't have one.

Removed in update to Part 0.

Comment 28

[Nicholas Nethercote [inactive]]

Comment on attachment 8669161 [details] [diff] [review]
Part 0-2: Remove |explicit| attribute from nsIMemoryReporterManager

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

IIRC the |explicit| property used to be used by endurance tests, which were a precursor to AWSY. But nothing significant that I know of uses them any more.

Comment 29

[Nicholas Nethercote [inactive]]

> Renaming is probably the most straightforward solution at this point (and
> adding some more comments / extra assertions).

Can't you merge the two state structs now that getReportsForThisProcess is no longer an entry point?

Comment 30

[Eric Rahm [:erahm]]

(In reply to Nicholas Nethercote [:njn] from comment #29)
> > Renaming is probably the most straightforward solution at this point (and
> > adding some more comments / extra assertions).
> 
> Can't you merge the two state structs now that getReportsForThisProcess is
> no longer an entry point?

Unfortunately the offender is |getReportsForThisProcessExtended| which is still present and is also used via IPC to start reporting w/in the child processes (which do no have GetReportsState).

Comment 31

[Nicholas Nethercote [inactive]]

Fair enough. I suggested "InterProcess" and "IntraProcess" in the names. "Parent" and "Child" might also work.

Comment 32

[Eric Rahm [:erahm]]

Attachment: Part 0-4: Rename GetReportsState PendingProcessesState

Comment 33

[Nicholas Nethercote [inactive]]

Comment on attachment 8670053 [details] [diff] [review]
Part 0-4: Rename GetReportsState PendingProcessesState

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

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +1433,3 @@
>                                           aAnonymize,
>                                           aMinimize,
>                                           concurrency,

Nit: fix indentation here.

Comment 34

[Eric Rahm [:erahm]]

Attachment: Part 2: Run reporters asynchronously

This incorporates updates from the previous review, no need to review the IPC
part (:jld has taken care of that).

Primary changes:
  - CollectReportsState is renamed PendingReportsState
  - The amount of shared variables b/w PendingProcessesState and
    PendingReportsState has been reduced by capturing them in the
    runnable dispatched to the main thread.
  - A few misbehaving tests have been fixed. They were copying the text of
    a target frame to the clipboard, then waiting for expected text to show
    up in the clipboard. The expected text used to show up before checking the
    clipboard, but that is no longer the case.

Comment 35

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=933c5e996115

Comment 36

[Nicholas Nethercote [inactive]]

Comment on attachment 8670055 [details] [diff] [review]
Part 2: Run reporters asynchronously

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

BTW, it's probably worth renumbering all the patches here before landing.

::: toolkit/components/aboutmemory/tests/test_aboutmemory.xul
@@ +555,5 @@
> +          let rslt = aActual.trim() === aExpected.trim();
> +          if (!rslt) {
> +            // Try copying again.
> +            synthesizeKey("A", {accelKey: true});
> +            synthesizeKey("C", {accelKey: true});

Dodgy, but ok...

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +1518,5 @@
> +  // Grab refs to everything used in the lambda function.
> +  nsRefPtr<nsMemoryReporterManager> self = this;
> +  nsCOMPtr<nsIMemoryReporter> reporter = aReporter;
> +  nsCOMPtr<nsIHandleReportCallback> handleReport = aHandleReport;
> +  nsCOMPtr<nsISupports> handleReportData = aHandleReportData;

So the point of this is to ensure these things aren't collected so long as the lambda is alive? Ok. Did you see this technique used elsewhere?

::: xpcom/base/nsMemoryReporterManager.h
@@ +268,5 @@
>    // new/delete for this because its lifetime doesn't match block scope or
>    // anything like that.
>    PendingProcessesState* mPendingProcessesState;
>  
> +  // This is reinitialized each time a call to get reports is initiated.

s/get reports/GetReports()/ ?

Comment 37

[Eric Rahm [:erahm]]

Thanks for the quick review! Try looks happy so we should be good to land.

(In reply to Nicholas Nethercote [:njn] from comment #36)
> Comment on attachment 8670055 [details] [diff] [review]
> Part 2: Run reporters asynchronously
> 
> Review of attachment 8670055 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> BTW, it's probably worth renumbering all the patches here before landing.

Good point, I'll be sure to rename them.

> 
> ::: toolkit/components/aboutmemory/tests/test_aboutmemory.xul
> @@ +555,5 @@
> > +          let rslt = aActual.trim() === aExpected.trim();
> > +          if (!rslt) {
> > +            // Try copying again.
> > +            synthesizeKey("A", {accelKey: true});
> > +            synthesizeKey("C", {accelKey: true});
> 
> Dodgy, but ok...

Yeah...it was already dodgy, it's slightly less dodgy now :)

> ::: xpcom/base/nsMemoryReporterManager.cpp
> @@ +1518,5 @@
> > +  // Grab refs to everything used in the lambda function.
> > +  nsRefPtr<nsMemoryReporterManager> self = this;
> > +  nsCOMPtr<nsIMemoryReporter> reporter = aReporter;
> > +  nsCOMPtr<nsIHandleReportCallback> handleReport = aHandleReport;
> > +  nsCOMPtr<nsISupports> handleReportData = aHandleReportData;
> 
> So the point of this is to ensure these things aren't collected so long as
> the lambda is alive? Ok. Did you see this technique used elsewhere?

Yes this is required (I believe we have static analysis to enforce 'no raw pointers to ref counted things' in lambdas). The pattern is somewhat prevalent in media (look for various |self| references) and has been discussed on dev.platform (one such example [1]).

[1] https://groups.google.com/d/msg/mozilla.dev.platform/EgwT_1A3SjY/q_-udzONVQgJ

> ::: xpcom/base/nsMemoryReporterManager.h
> @@ +268,5 @@
> >    // new/delete for this because its lifetime doesn't match block scope or
> >    // anything like that.
> >    PendingProcessesState* mPendingProcessesState;
> >  
> > +  // This is reinitialized each time a call to get reports is initiated.
> 
> s/get reports/GetReports()/ ?

Will update.

Comment 38

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fcbfd1379fcd3bbe55d6b1feff5610eb6475f36b
Bug 1194555 - Part 0: Cleanup GetReportsState constructor. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbaac05a29345a9793802decc27d67f51954c3da
Bug 1194555 - Part 1: Remove RunReportersForThisProcess. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/748bfebe81e7f6abfbbacab8268f1be7fc19b8ad
Bug 1194555 - Part 2: Remove |explicit| attribute from nsIMemoryReporterManager. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/70a8ed3b6a450fdf309e13bbbfd4409456bd8d66
Bug 1194555 - Part 3: Remove |getReportsForThisProcess| from the nsIMemoryReporterManager interface. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/99b27aa952c57741df91fc047c2d973437e9c285
Bug 1194555 - Part 4: Rename GetReportsState PendingProcessesState. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/11951462a37c71976aac60a9094a9fa3e6cef11c
Bug 1194555 - Part 5: Add option for async memory reporters. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/e77be333f4fb32b1556c2e01308ebef44acc2d16
Bug 1194555 - Part 6: Run reporters asynchronously. r=njn,jld

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/dc6be2cf0c5d for xpcshell failures:

https://treeherder.mozilla.org/logviewer.html#?job_id=15255681&repo=mozilla-inbound

Comment 40

[Eric Rahm [:erahm]]

(In reply to Wes Kocher (:KWierso) from comment #39)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/dc6be2cf0c5d for
> xpcshell failures:
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=15255681&repo=mozilla-
> inbound

Best I can tell we've uncovered a latent bug, that test calls |crashReporter.saveMemoryReport| [1] which happens to asynchronously create a memory report (but returns immediately) [2], it has a callback that sets an attribute indicating there's a memory report [3].

Presumably that callback isn't hit before the crash is triggered so no memory report is found. I'll see if I can repro locally.

[1] https://hg.mozilla.org/mozilla-central/annotate/89732fcdb0baca70e8b7a25a2725117113f0db80/toolkit/crashreporter/test/unit/test_crash_with_memory_report.js#l46
[2] https://hg.mozilla.org/mozilla-central/annotate/89732fcdb0baca70e8b7a25a2725117113f0db80/toolkit/xre/nsAppRunner.cpp#l1414
[3] https://hg.mozilla.org/mozilla-central/annotate/89732fcdb0baca70e8b7a25a2725117113f0db80/toolkit/xre/nsAppRunner.cpp#l1422

Comment 41

[Eric Rahm [:erahm]]

Attachment: Part 2-1: Let the event loop process before crashing in xpcshell crash tests

So this works, that's about all I'll say about it. Short of reinventing the
xpcshell crashtest harness this is probably as good as it gets.

I'll fold this into Part 2 once we get an r+.

Comment 42

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4dfdf8155db9

Comment 43

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b60163f763e7

Comment 44

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=192cb9f3041d

Comment 45

[Eric Rahm [:erahm]]

Attachment: Part 2-1: Let the event loop process before crashing in xpcshell crash tests

For the case where we're generating a memory report we let the event loop spin
a bit before crashing. See comment 40 to get an idea for what this is working
around.

This version even has a clean try run!

Comment 46

[Eric Rahm [:erahm]]

Ted, are you able to review attachment 8671065 [details] [diff] [review] or redirect to a more appropriate reviewer? Its blocking landing this bug, which in turn is blocking landing bug 1190592.

Comment 47

[(not currently active) Ted Mielczarek]

Comment on attachment 8671065 [details] [diff] [review]
Part 2-1: Let the event loop process before crashing in xpcshell crash tests

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

::: toolkit/crashreporter/test/unit/crasher_subprocess_tail.js
@@ +2,5 @@
> +if (shouldDelay) {
> +  let shouldCrashNow = false;
> +  let thr = Components.classes["@mozilla.org/thread-manager;1"]
> +                          .getService().currentThread;
> +  thr.dispatch({ run: () => { shouldCrashNow = true; } },

This is sufficient to allow everything to complete? This would fail if one of the memory reporters were to queue further events, right?

Would it be better to have an explicit observer service notification or something for "memory reporting complete" and wait for that?

Comment 48

[(not currently active) Ted Mielczarek]

Sorry, being bad about reviews right now.

Comment 49

[Eric Rahm [:erahm]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #47)
> Comment on attachment 8671065 [details] [diff] [review]
> Part 2-1: Let the event loop process before crashing in xpcshell crash tests
> 
> Review of attachment 8671065 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/crashreporter/test/unit/crasher_subprocess_tail.js
> @@ +2,5 @@
> > +if (shouldDelay) {
> > +  let shouldCrashNow = false;
> > +  let thr = Components.classes["@mozilla.org/thread-manager;1"]
> > +                          .getService().currentThread;
> > +  thr.dispatch({ run: () => { shouldCrashNow = true; } },
> 
> This is sufficient to allow everything to complete? This would fail if one
> of the memory reporters were to queue further events, right?
> 
> Would it be better to have an explicit observer service notification or
> something for "memory reporting complete" and wait for that?

This is sufficient for now, I'll file a follow up for making xpcshell crashtests properly handle async requests.

Comment 50

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e093cc10d7ca892d8bab175c3bdfcb8fb74787e8
Bug 1194555 - Part 0: Cleanup GetReportsState constructor. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/647ea5d7f72675182fc31340feb4eade8f5cde70
Bug 1194555 - Part 1: Remove RunReportersForThisProcess. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/61a127fdd89b7cff2bbeca51b38b1c6b1063c9c8
Bug 1194555 - Part 2: Remove |explicit| attribute from nsIMemoryReporterManager. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/c08f47894887fc3d6777842d041f4cc176f234a6
Bug 1194555 - Part 3: Remove |getReportsForThisProcess| from the nsIMemoryReporterManager interface. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/c3a95dcbc56b273686e9d613cbf907afb600b96d
Bug 1194555 - Part 4: Rename GetReportsState PendingProcessesState. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/8db37a722f8326f39042dd2f38d0814fc20ad552
Bug 1194555 - Part 5: Add option for async memory reporters. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/dcee00c2b89dbb1c26720228d28672aa10264126
Bug 1194555 - Part 6: Run reporters asynchronously. r=njn,jld,ted

Comment 51

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e093cc10d7ca
https://hg.mozilla.org/mozilla-central/rev/647ea5d7f726
https://hg.mozilla.org/mozilla-central/rev/61a127fdd89b
https://hg.mozilla.org/mozilla-central/rev/c08f47894887
https://hg.mozilla.org/mozilla-central/rev/c3a95dcbc56b
https://hg.mozilla.org/mozilla-central/rev/8db37a722f83
https://hg.mozilla.org/mozilla-central/rev/dcee00c2b89d