Closed
Bug 1194555
Opened 10 years ago
Closed 9 years ago
Asynchronously invoke registered memory reporters
Categories
(Core :: XPCOM, defect)
Core
XPCOM
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
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
10.01 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
15.97 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
14.20 KB,
patch
|
erahm
:
review+
|
Details | Diff | Splinter Review |
13.30 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
16.19 KB,
patch
|
n.nethercote
:
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.
Assignee | ||
Comment 1•10 years ago
|
||
Move GetReportsState ctor to the impl so that mChildrenPending doesn't have
to be heap allocated.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
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 6•10 years ago
|
||
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 7•10 years ago
|
||
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+
![]() |
||
Comment 8•10 years ago
|
||
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.
Assignee | ||
Comment 9•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8648520 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8648519 -
Flags: review?(n.nethercote)
Assignee | ||
Updated•9 years ago
|
Attachment #8648518 -
Flags: review?(n.nethercote)
Assignee | ||
Comment 10•9 years ago
|
||
Assignee | ||
Comment 11•9 years ago
|
||
(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•9 years ago
|
||
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 13•9 years ago
|
||
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 14•9 years ago
|
||
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+
![]() |
||
Comment 15•9 years ago
|
||
BTW, it generally looks pretty good. Much neater than I was expecting :)
Assignee | ||
Comment 16•9 years ago
|
||
(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.
Assignee | ||
Comment 17•9 years ago
|
||
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•9 years ago
|
||
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+
Assignee | ||
Comment 19•9 years ago
|
||
Assignee | ||
Comment 20•9 years ago
|
||
Updated to remove the GetReportState dtor as well.
Assignee | ||
Updated•9 years ago
|
Attachment #8648518 -
Attachment is obsolete: true
Assignee | ||
Comment 21•9 years ago
|
||
RunReportersForThisProcess is no longer used and will no longer work once we have async reporters.
Attachment #8669159 -
Flags: review?(n.nethercote)
Assignee | ||
Comment 22•9 years ago
|
||
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)
Assignee | ||
Comment 23•9 years ago
|
||
|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)
Assignee | ||
Comment 24•9 years ago
|
||
Comment on attachment 8669157 [details] [diff] [review]
Part 0: Cleanup GetReportsState constructor
Carrying forward r+
Attachment #8669157 -
Flags: review+
Assignee | ||
Comment 25•9 years ago
|
||
Updated to use the |GET_MEMORY_REPORTER_MANAGER| macro.
Assignee | ||
Updated•9 years ago
|
Attachment #8648519 -
Attachment is obsolete: true
Assignee | ||
Comment 26•9 years ago
|
||
Comment on attachment 8669175 [details] [diff] [review]
Part 1: Add option for async memory reporters
Carrying forward r+
Attachment #8669175 -
Flags: review+
Assignee | ||
Comment 27•9 years ago
|
||
(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.
![]() |
||
Updated•9 years ago
|
Attachment #8669159 -
Flags: review?(n.nethercote) → review+
![]() |
||
Comment 28•9 years ago
|
||
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+
![]() |
||
Updated•9 years ago
|
Attachment #8669162 -
Flags: review?(n.nethercote) → review+
![]() |
||
Comment 29•9 years ago
|
||
> 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?
Assignee | ||
Comment 30•9 years ago
|
||
(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•9 years ago
|
||
Fair enough. I suggested "InterProcess" and "IntraProcess" in the names. "Parent" and "Child" might also work.
Assignee | ||
Comment 32•9 years ago
|
||
Attachment #8670053 -
Flags: review?(n.nethercote)
![]() |
||
Comment 33•9 years ago
|
||
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+
Assignee | ||
Comment 34•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8667531 -
Attachment is obsolete: true
Assignee | ||
Comment 35•9 years ago
|
||
![]() |
||
Comment 36•9 years ago
|
||
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+
Assignee | ||
Comment 37•9 years ago
|
||
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.
Assignee | ||
Comment 38•9 years ago
|
||
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
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
Flags: needinfo?(erahm)
Assignee | ||
Comment 40•9 years ago
|
||
(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)
Assignee | ||
Comment 41•9 years ago
|
||
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)
Assignee | ||
Comment 42•9 years ago
|
||
Attachment #8670558 -
Flags: review?(dmajor) → review?(ted)
Assignee | ||
Comment 43•9 years ago
|
||
Assignee | ||
Comment 44•9 years ago
|
||
Assignee | ||
Comment 45•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8670558 -
Attachment is obsolete: true
Attachment #8670558 -
Flags: review?(ted)
Assignee | ||
Comment 46•9 years ago
|
||
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 47•9 years ago
|
||
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+
Assignee | ||
Comment 49•9 years ago
|
||
(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.
Assignee | ||
Comment 50•9 years ago
|
||
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•9 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in
before you can comment on or make changes to this bug.
Description
•