Closed Bug 1194555 Opened 4 years ago Closed 4 years ago

Asynchronously invoke registered memory reporters

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: erahm, Assigned: erahm)

References

Details

Attachments

(8 files, 5 obsolete files)

8.94 KB, patch
erahm
: review+
Details | Diff | Splinter Review
2.73 KB, patch
njn
: review+
Details | Diff | Splinter Review
10.01 KB, patch
njn
: review+
Details | Diff | Splinter Review
15.97 KB, patch
njn
: review+
Details | Diff | Splinter Review
14.20 KB, patch
erahm
: review+
Details | Diff | Splinter Review
13.30 KB, patch
njn
: review+
Details | Diff | Splinter Review
16.19 KB, patch
njn
: review+
Details | Diff | Splinter Review
2.85 KB, patch
ted
: review+
Details | Diff | Splinter Review
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.
Move GetReportsState ctor to the impl so that mChildrenPending doesn't have
to be heap allocated.
Assignee: nobody → erahm
Status: NEW → ASSIGNED
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).
Attachment #8648519 - Flags: feedback?(n.nethercote)
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.
Attachment #8648520 - Flags: feedback?(n.nethercote)
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.
Attachment #8648519 - Flags: feedback?(n.nethercote) → feedback+
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?
Attachment #8648520 - Flags: feedback?(n.nethercote) → feedback+
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.
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.
Attachment #8667531 - Flags: review?(n.nethercote)
Attachment #8667531 - Flags: review?(jld)
Attachment #8648520 - Attachment is obsolete: true
Attachment #8648519 - Flags: review?(n.nethercote)
Attachment #8648518 - Flags: review?(n.nethercote)
(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 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?
Attachment #8648518 - Flags: review?(n.nethercote) → review+
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.
Attachment #8648519 - Flags: review?(n.nethercote) → review+
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.
Attachment #8667531 - Flags: review?(n.nethercote) → feedback+
BTW, it generally looks pretty good. Much neater than I was expecting :)
(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 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 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.
Attachment #8667531 - Flags: review?(jld) → review+
Updated to remove the GetReportState dtor as well.
Attachment #8648518 - Attachment is obsolete: true
RunReportersForThisProcess is no longer used and will no longer work once we have async reporters.
Attachment #8669159 - Flags: review?(n.nethercote)
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.
Attachment #8669161 - Flags: review?(n.nethercote)
|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|.
Attachment #8669162 - Flags: review?(n.nethercote)
Comment on attachment 8669157 [details] [diff] [review]
Part 0: Cleanup GetReportsState constructor

Carrying forward r+
Attachment #8669157 - Flags: review+
Updated to use the |GET_MEMORY_REPORTER_MANAGER| macro.
Attachment #8648519 - Attachment is obsolete: true
Comment on attachment 8669175 [details] [diff] [review]
Part 1: Add option for async memory reporters

Carrying forward r+
Attachment #8669175 - Flags: review+
(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.
Attachment #8669159 - Flags: review?(n.nethercote) → review+
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.
Attachment #8669161 - Flags: review?(n.nethercote) → review+
Attachment #8669162 - Flags: review?(n.nethercote) → review+
> 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?
(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).
Fair enough. I suggested "InterProcess" and "IntraProcess" in the names. "Parent" and "Child" might also work.
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.
Attachment #8670053 - Flags: review?(n.nethercote) → review+
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.
Attachment #8670055 - Flags: review?(n.nethercote)
Attachment #8667531 - Attachment is obsolete: true
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()/ ?
Attachment #8670055 - Flags: review?(n.nethercote) → review+
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.
(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
Flags: needinfo?(erahm)
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+.
Attachment #8670558 - Flags: review?(dmajor)
Attachment #8670558 - Flags: review?(dmajor) → review?(ted)
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!
Attachment #8671065 - Flags: review?(ted)
Attachment #8670558 - Attachment is obsolete: true
Attachment #8670558 - Flags: review?(ted)
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.
Flags: needinfo?(ted)
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?
Attachment #8671065 - Flags: review?(ted) → review+
Sorry, being bad about reviews right now.
Flags: needinfo?(ted)
(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.
You need to log in before you can comment on or make changes to this bug.