Closed Bug 910517 Opened 11 years ago Closed 11 years ago

Remove memory uni-reporters

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla26

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

(Keywords: addon-compat, Whiteboard: [MemShrink:P2])

Attachments

(3 files, 1 obsolete file)

Remove nsIMemoryReporter, and rename nsIMemoryMultiReporter as nsIMemoryReporter.
230.27 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
follow-up
6.23 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
screenshot
637.86 KB, image/png
Details 
We have two kinds of memory reporters:  uni-reporters (nsIMemoryReporter) and multi-reporters (nsIMemoryMultiReporter).  Multi-reporters subsume uni-reporters, and we only have uni-reporters because in the early days of about:memory we only measured a small number of things and didn't have multi-reporters.

I'm most of the way through a patch that will remove uni-reporters and rename nsIMemoryMultiReporter as nsIMemoryReporter.  The repurposing of "nsIMemoryReporter" is gauche, but it is too good a name to give up.  (jlebar didn't care for my suggested alternative "nsIReporter" :)

The patch is looking like a nice simplification.  Thanks to the MemoryReporterBase macro (and bug 831193's conversion of all uni-reporters to use it) the existing uni-reporters can be repackaged as multi-reporters (that happen to report one measurement) with minimal fuss.  And the various places that iterate through all the reporters become much simpler, having only one kind to deal with.

mccr8, heads up:  with jlebar's departure imminent, you'll be the lucky ducky who gets to review this :)
I assume whatever files you end up touching here are the ones I should review to familiarize myself with the memory reporting infrastructure?  I've looked at bits and pieces before, but I haven't tried to get an overview of the whole thing.
xpcom/base/nsIMemoryReporter.idl is the main one.  It defines nsIMemoryReporter and nsIMemoryReporterManager, which are the most important pieces.  Read that all the way through -- it's only 400 lines with the aforementioned patch applied -- and you'll have a pretty good idea.

And xpcom/base/nsMemoryReporterManager.cpp is the other big part;  it defines all of the nsIMemoryReporterManager.cpp methods, plus a bunch of basic reporters like "resident" and "vsize".

And then there's a bajillion reporters scattered all over the place, of course :)
While DownThemAll! 3.0 Beta might be the only add-on that we know of that implements own reporters, there are still a couple of add-ons querying the reporters: https://mxr.mozilla.org/addons/search?string=nsIMemoryReporter , such as apparently Firebug and the add-on SDK test harness.
Keywords: addon-compat
Total effect of this patch:

  66 files changed, 835 insertions(+), 1252 deletions(-)

Here are the basic changes.

- nsIMemoryReporter is gone.

- Things renamed:
  - nsIMemoryMultiReporter --> nsIMemoryReporter
  - nsIMemoryMultiReporterCallback --> nsIMemoryReporterCallback
  - WebGLMemoryMultiReporterWrapper --> WebGLMemoryReporterWrapper
    (and likewise for the filename)
  - Lots of other similar things.

- In various places that enumerate all reporters, we no longer need to worry
  about single reporters:
  - addon-sdk/source/lib/sdk/test/harness.js
  - ContentChild::RecvPMemoryReportRequestConstructor
  - aboutMemory.js:processMemoryReporters()
  - test_*.xul
  - DumpProcessMemoryReportsToGZFileWriter
  - nsMemoryReporterManager::GetExplicit
  - RunReporters

Here's some details on parts of the patch that are non-obvious.  Please ask if
you have any more questions.  (If you haven't yet read nsIMemoryReporter.idl,
please do so!)

- NumGhostsReporter, LowMemoryEvents{Virtual,Physical}Reporter,
  LowCommitSpaceEventsReporter.  The description strings had a %d in them, and
  that value could change at runtime.  This doesn't play well with
  MemoryUniReporter, which now can only take a description string only when the
  reporter is created.  I figured the usefulness of showing the value was
  sufficiently low that it wasn't worth rewriting these as from-the-ground-up
  nsIMemoryReporters (instead of using MemoryUniReporter), so I just got rid of
  it.

- ChildReporter.  Here's how the IPC stuff works... when a child communicates
  multiple reports to its parent, these are passed as an array of
  MemoryReports, which are just POD structs.
  
  Previously, on receipt of this array, the parent would register one
  (uni-)ChildReporter for each MemoryReport.  On receipt of a new array, all
  the previous (uni-)ChildReporters were unregistered, and new ones replaced
  them.
  
  In the new code, the parent installs a single (multi-)ChildReporter which
  contains a copy of the array and so can report all of the MemoryReports.  On
  receipt of a new array, the single (multi-)ChildReporter is replaced.

  (You may notice that both schemes only support getting reports reliably from
  a single child process!  This is because it dates back to when Fennec had two
  processes:  chrome and content.  In the new code, it could be extended to
  have a table of (multi-)ChildReporters indexed by PID, but really we need a
  completely new and better IPC design.)

- I changed the |name| of StorageSQLiteMultiReporter from "storage-sqlite" to
  "storage-sqlite-multi" so it doesn't collide with StorageSQLiteUniReporter.
  (Such a collision could confuse the telemetry code.)  Previously there was no
  collision because old-style uni-reporters didn't have a |name|.

- aboutMemory.js:  different parts of aboutMemory.js ignore some of the
  reporters.  Previously, it ignored some uni-reporters based on their *path*,
  and some multi-reporters based on the *name*.  Now, it ignores some reporters
  based on their *name*, and then among the non-ignored ones, ignores some of
  the reports based on their *path*.

  Complicating this is the fact that reports from child processes get installed
  under a different name to what you first expect.  E.g. in the new code the
  ChildReporter is called "child".  So both levels of checking are required.
  (Similarly when reading from file -- you only see report paths there, not
  reporter names.)

- In all the tests I had to convert all the fake uni-reporters to
  multi-reporters.  I also deleted some |explicitNonHeap| values that are no
  longer necessary and should have been removed in an earlier bug.

- TelemetryPing.js.  First, this file was already using NS_ASSERT(), which
  requires importing debug.js, but debug.js wasn't being imported.  This worked
  only because the failure-to-import only causes problems if NS_ASSERT() is
  actually called, and the sole existing call was on an impossible path.
  Still, worth fixing, esp. as I've added another call.

  Second, telemetry can only deal with uni-reporters.  That was easy to
  guarantee when uni-reporters were a separate type, but it's harder now.
  (This is about the only part of the code that is more complex as a result of
  this patch.)  Now I make the callback passed to collectReports() check that
  it's only called once, and NS_ASSERT() otherwise.  (BTW, I've never had to
  use bind() before, but I think I've used it appropriately here;  the code
  certainly doesn't work without it.)

  Finally, that |amount == -1| check in handleMemoryReport dates from ages ago,
  when we used to return -1 from memory uni-reporters that failed.  They now
  fail the usual way via nsresult, so this isn't needed.

- MemoryReporterBase, which handles the boilerplate for a uni-reporter, was
  changed to work as a multi-reporter that does a single report.  And I renamed
  it MemoryUniReporter.  (We don't yet have an equivalent MemoryMultiReporter
  class, but I plan to add one.)

- In RunReporters() I removed the HEAP/explicit checking added by bug 811018,
  which was a hack to avoid some false positive double reports in DMD's output.
  Subsequently, bug 820128's fix rendered it unnecessary (and is a much better
  approach), but the checking was never removed.  So I've removed it now.
Attachment #797659 - Flags: review?(continuation)
(In reply to Nils Maier [:nmaier] from comment #3)
> While DownThemAll! 3.0 Beta might be the only add-on that we know of that
> implements own reporters, there are still a couple of add-ons querying the
> reporters: https://mxr.mozilla.org/addons/search?string=nsIMemoryReporter ,
> such as apparently Firebug and the add-on SDK test harness.

I have removed usage of nsIMemoryReporter from Firebug source tree (it's been obsolete code anyway). The change is tracked here:
http://code.google.com/p/fbug/issues/detail?id=6726

Honza
There's a US holiday on Monday, so I'll block out Tuesday to review this.
Thanks!  This isn't urgent.  Please ask as many questions as necessary;  I've thrown you in the deep end on this one.
Depends on: 911639
Blocks: 911639
No longer depends on: 911639
The memory reporting MDN page ( https://wiki.mozilla.org/Memory_Reporting ) needs to be updated in light of this (and previous) changes.  For instance, the macro NS_MEMORY_REPORTER_IMPLEMENT_HELPER doesn't seem to exist any more.

Also, in "please ask nnethercote" at the top of the MDN page, nnethercote should probably be changed to something else, either your name spelled out, or :njn or n.nethercote or something, as typing in nnethercote into the reviewer slot in bugzilla will not produce a useful result. :)
Keywords: dev-doc-needed
And the MDN pages on nsIMemoryReporter, nsIMemoryMultiReporter, and nsIMemoryReporterManager will also need updating.
Mossop: do you know about addon-sdk/source/lib/sdk/test/harness.js?  I just noticed it has a function reportMemoryUsage() that does memory reporter stuff, but the code is so out-of-date that it can't have worked correctly for something like two years, due to XPCOM interface changes.  It appears to only be called if |profileOptions| is true, and that appears to be determined by the |options| parameter to runTests(), and runTests() appears to be exported somehow.

If you could shed some light, or needinfo someone else who can, that'd be great.  Thanks!
Flags: needinfo?(dtownsend+bugmail)
Attached patch follow-up — — Splinter Review 
This patch applies on top of the previous one, though I'll fold them before
landing.  It addresses some minor things I found.

- I added a comment to harness.js about the existing code being utterly broken.

- I fixed test_bug601470.html, which I had missed.

- In aboutMemory.js:
  - I removed an out-of-date comment in processMemoryReporters().
  - I hoisted the handleReport() closure out of a loop, on the off chance that
    this is more efficient.
  - I now ignore "resident-fast" in both ignoreReporter() and ignoreReport().
    Previously, I was only doing the latter, which wasn't a big deal but meant
    that it was running unnecessarily.  (Nb: we still need the check in
    ignoreReport() to handle the case of "resident-fast" reports from child
    processes.)
Attachment #799037 - Flags: review?(continuation)
Comment on attachment 797659 [details] [diff] [review]
Remove nsIMemoryReporter, and rename nsIMemoryMultiReporter as nsIMemoryReporter.

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

Nice cleanup!

(I haven't looked at the new patch yet so feel free to ignore anything you've addressed in there.)

::: dom/base/nsWindowMemoryReporter.h
@@ +138,5 @@
>      nsRefPtr<nsWindowMemoryReporter> mWindowReporter;
>    };
>  
>    /**
>     * nsGhostWindowReporter generates the "ghost-windows" single-report, which

nit: "uni-report" rather than "single-report"?

::: dom/ipc/ContentParent.cpp
@@ +180,2 @@
>      {
> +        name.AssignLiteral("child");

Is this too generic?  Maybe something more like contentChild?

::: dom/ipc/ContentParent.h
@@ +142,5 @@
>  
>      bool IsAlive();
>      bool IsForApp();
>  
> +    void SetChildMemoryReporters(const InfallibleTArray<MemoryReport>&

Should this be called "SetChildMemoryReports"?

::: dom/workers/WorkerPrivate.h
@@ +33,5 @@
>  
>  class JSAutoStructuredCloneBuffer;
>  class nsIChannel;
>  class nsIDocument;
> +class nsIMemoryReporter;

Looks like this forward declaration can just be removed if you aren't using it in the header anywhere?

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1571,5 @@
>                 js::gc::ChunkSize;
>      }
>  };
>  
> +// Nb: js-compartments/system + js-compartments/user could be different to the

"different from" rather than "different to"?  Though maybe that's a Commonwealthism. ;)

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +582,5 @@
>    updateMainAndFooter("", SHOW_FOOTER);
>  
>    try {
>      // Process the reports from the memory reporters.
> +    let process = function(aIgnoreReporter, aIgnoreReport, aHandleReport) {

What is the point of this eta expansion?  ie why don't you just pass in processMemoryReports directly?

::: toolkit/components/aboutmemory/tests/test_aboutcompartments.xul
@@ +46,2 @@
>  
>    function f(aProcess, aPath, aKind, aUnits, aAmount) {

The function f can be removed.

::: toolkit/components/aboutmemory/tests/test_aboutmemory.xul
@@ +51,2 @@
>  
>    function f2(aProcess, aPath, aKind, aUnits, aAmount) {

Looks like you can delete the functions f2 and f2.

@@ +183,5 @@
> +      collectReports: function(aCbObj, aClosure) {
> +        function f(aP1, aP2, aK, aU, aA) {
> +          aCbObj.callback(aP1, aP2, aK, aU, aA, "Desc.", aClosure);
> +        }
> +        f("2nd", "heap-allocated",  OTHER,   BYTES,1000 * MB);

I'm not sure what sort of alignment you are going for in the last column ("1000 * MB" etc) but it isn't really working...

@@ +196,5 @@
> +        f("2nd", "other0",          OTHER,   BYTES,666 * MB);
> +        f("2nd", "other1",          OTHER,   BYTES,111 * MB);
> +        // If the "smaps" reporter is in a child process it'll be passed to
> +        // the main process under a different name.  The fact that we skip
> +        // the "smaps" reporter in the main process won't cause these '

nit: extra ' in this comment

::: xpcom/base/nsIMemoryReporter.idl
@@ +28,5 @@
> +   * reporter initially has "" in this field, indicating that it applies to the
> +   * current process.  (This is true even for reporters in a child process.)
> +   * When a reporter from a child process is copied into the main process, the
> +   * copy has its 'process' field set appropriately.
> +   * 

multi-nit: trailing whitespace.  There are 5 or so more instances of it that I see in this file from looking at splinter.

@@ +116,5 @@
> +   * |amount|  The numeric value reported by this memory reporter.  Accesses
> +   * can fail if something goes wrong when getting the amount.
> +   *
> +   *
> +   * |description|  A human-readable description of this memory usage report.

Do you want to put the bit in here about how this should be a sentence?

@@ +124,5 @@
>                  in AUTF8String description, in nsISupports closure);
>  };
>  
>  /*
> + * A nsIMemoryReporter reports one or more memory measurements via a

nit: maybe this and below be "An nsIblahblah"?

@@ +140,5 @@
> + *   OS-level allocation (e.g. mmap/VirtualAlloc/vm_allocate) or a
> + *   heap-level allocation (e.g. malloc/calloc/operator new).  Reporters
> + *   in this tree must have kind HEAP or NONHEAP, units BYTES, and a
> + *   description that is a sentence (i.e. starts with a capital letter and
> + *   ends with a period, or similar).

This sentence on constraints mostly, but not entirely, overlaps with the constraints described in the callback, which seems a little odd to me, but I guess it doesn't matter.  A sentence referring back to the callback description would be about as long as this bit, and it is nice to have it all together in one place.

@@ +327,5 @@
> +// memory uni-reporters.  You just need to provide the following.
> +// - The constant values: nameAndPath (which serves as both the reporters name,
> +//   and the path in its single report), kind, units, and description.  They
> +//   are passed to the MemoryUniReporter constructor.
> +// - A (private) Amount() or GetAmount() method.  It can use the MallocSizeOf

protected, not private?

::: xpcom/base/nsMemoryInfoDumper.cpp
@@ +815,4 @@
>    DUMP(aWriter, ",\n");
>    DUMP(aWriter, "  \"reports\": ");
>  
>    // Process single reporters.

nit: This should be "Process reporters." or something.

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +956,4 @@
>          {
> +            Int64Wrapper* wrappedInt64 =
> +                static_cast<Int64Wrapper*>(aWrappedExplicit);
> +            wrappedInt64->mValue += aAmount;

Should you assert that aAmount != -1 (which this used to test for)?

::: xpcom/ds/nsObserverService.h
@@ +46,5 @@
>    void RegisterReporter();
>  
>    bool mShuttingDown;
>    nsTHashtable<nsObserverList> mObserverTopicTable;
> +  nsCOMPtr<nsIMemoryReporter> mReporter;

This is is a little odd.  IIRC, forward declarations usually aren't enough for nsCOMPtr, so I bet you can remove the forward declaration because you are bootlegging it in somehow. ;)
Attachment #797659 - Flags: review?(continuation) → review+
Attachment #799037 - Flags: review?(continuation) → review+
Whiteboard: [MemShrink] → [MemShrink:P2]
Thanks for the speedy review!  I've addressed all comments except for the ones mentioned below.


> > +        name.AssignLiteral("child");
> 
> Is this too generic?  Maybe something more like contentChild?

I chose "content-child", which matches the standard reporter name form.

> "different from" rather than "different to"?  Though maybe that's a
> Commonwealthism. ;)

Yeah, it's a British-ism, the way "different than" is an American-ism :)

> What is the point of this eta expansion?  ie why don't you just pass in
> processMemoryReports directly?

Heh.  I wonder if it's always been like that, or if it used to be necessary and then something got simplified.

> ::: xpcom/base/nsIMemoryReporter.idl
> 
> multi-nit: trailing whitespace.  There are 5 or so more instances of it that
> I see in this file from looking at splinter.

/me modifies his .vimrc to add .idl files to those that are checked for trailing whitespace.

> > +   * |description|  A human-readable description of this memory usage report.
> 
> Do you want to put the bit in here about how this should be a sentence?

No.  Not all descriptions need to be sentences;  just those that about:memory processes.  Which is most but not all of them :)

> @@ +140,5 @@
> > + *   OS-level allocation (e.g. mmap/VirtualAlloc/vm_allocate) or a
> > + *   heap-level allocation (e.g. malloc/calloc/operator new).  Reporters
> > + *   in this tree must have kind HEAP or NONHEAP, units BYTES, and a
> > + *   description that is a sentence (i.e. starts with a capital letter and
> > + *   ends with a period, or similar).
> 
> This sentence on constraints mostly, but not entirely, overlaps with the
> constraints described in the callback, which seems a little odd to me, but I
> guess it doesn't matter.  A sentence referring back to the callback
> description would be about as long as this bit, and it is nice to have it
> all together in one place.

Ditto -- about:memory's constraints are a little tighter than the basic ones.

> > +// - A (private) Amount() or GetAmount() method.  It can use the MallocSizeOf
> 
> protected, not private?

Amount() can be private.  GetAmount() must be public;  I've clarified that.

> ::: xpcom/base/nsMemoryReporterManager.cpp
> @@ +956,4 @@
> >          {
> > +            Int64Wrapper* wrappedInt64 =
> > +                static_cast<Int64Wrapper*>(aWrappedExplicit);
> > +            wrappedInt64->mValue += aAmount;
> 
> Should you assert that aAmount != -1 (which this used to test for)?

No.  Although none currently do, it's feasible that a reporter might legitmately report -1 for an amount, e.g. if it measured a difference since last time it was measured.

> > +  nsCOMPtr<nsIMemoryReporter> mReporter;
> 
> This is is a little odd.  IIRC, forward declarations usually aren't enough
> for nsCOMPtr, so I bet you can remove the forward declaration because you
> are bootlegging it in somehow. ;)

IIRC, forward declarations suffice so long as the relevant constructor is in a .cpp file :)
> While DownThemAll! 3.0 Beta might be the only add-on that we know of that
> implements own reporters, there are still a couple of add-ons querying the
> reporters: https://mxr.mozilla.org/addons/search?string=nsIMemoryReporter ,
> such as apparently Firebug and the add-on SDK test harness.

Let's look at the interesting ones.

- Firebug's ok, as per comment 5.

- I've updated the in-repo version of the addon SDK test harness.

- https://mxr.mozilla.org/addons/source/200730/chrome/content/util.js#988 is already busted -- it still refers to nsIMemoryReporter.memoryUsed, which was changed to |amount| 2 years ago.

- https://mxr.mozilla.org/addons/source/249361/chrome/content/memoryrestart.js#143:  ditto.

- https://mxr.mozilla.org/addons/source/282244/resources/jid0-rpvfv5hsovtkmfhwdnyqpd1qc9k-at-jetpack-memory-meter-lib/main.js#35:  ditto.

- r2d2b2g:  this seems to just have copied huge chunks of the code.  Weird.

- https://mxr.mozilla.org/addons/source/424070/about.js#251:  this currently handle uni-reporters and multi-reporters correctly, and will be broken by the change.  The fix is very simple.

So, only one case of bustage that we know about.  I think I'm clear to land;  I'll do the doc updates afterwards.  Nils, do you agree?
Flags: needinfo?(maierman)
Comment on attachment 797659 [details] [diff] [review]
Remove nsIMemoryReporter, and rename nsIMemoryMultiReporter as nsIMemoryReporter.

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

::: xpcom/base/nsIMemoryReporter.idl
@@ +189,3 @@
>  };
>  
>  [scriptable, builtinclass, uuid(70b0e608-8dbf-4dc7-b88f-f1c745c1b48c)]

Shouldn't you change the uuid here?
> - r2d2b2g:  this seems to just have copied huge chunks of the code.  Weird.
That's the boot2gecko/Firefox OS simulator add-on that essentially ships a whole b2g build. ;)

> - https://mxr.mozilla.org/addons/source/424070/about.js#251
That's just my about:addons-memory ;)

> I think I'm clear to land;  I'll do the doc updates afterwards.  Nils, do you agree?
Yup. Except for the nsIMemoryReporterManager uuid remark.
Flags: needinfo?(maierman)
(In reply to Nicholas Nethercote [:njn] from comment #10)
> Mossop: do you know about addon-sdk/source/lib/sdk/test/harness.js?  I just
> noticed it has a function reportMemoryUsage() that does memory reporter
> stuff, but the code is so out-of-date that it can't have worked correctly
> for something like two years, due to XPCOM interface changes.  It appears to
> only be called if |profileOptions| is true, and that appears to be
> determined by the |options| parameter to runTests(), and runTests() appears
> to be exported somehow.
> 
> If you could shed some light, or needinfo someone else who can, that'd be
> great.  Thanks!

Looks like ancient code. I'd just remove it or ignore it, we clearly aren't using it
Flags: needinfo?(dtownsend+bugmail)
> >  [scriptable, builtinclass, uuid(70b0e608-8dbf-4dc7-b88f-f1c745c1b48c)]
> 
> Shouldn't you change the uuid here?

Yes!  Good catch, thanks.

> Looks like ancient code. I'd just remove it or ignore it, we clearly aren't
> using it

I added a comment explaining that it's busted.
I have updated https://wiki.mozilla.org/Platform/Memory_Reporting#Memory_Reporters

sheppy, the patch that just landed did the following:
- Removed nsIMemoryReporter.
- Renamed nsIMemoryMultiReporter as nsIMemoryReporter.

Those two classes are documented here:

https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIMemoryReporter
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIMemoryMultiReporter

and there are two associated classes:

https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIMemoryMultiReporterCallback
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIMemoryReporterManager

How should I document the change?  The easiest thing would be document things as they are now -- e.g. delete the nsIMemoryMultiReporter page -- but I think you'll want to keep some info about how things used to be around?  Let me know.  Thanks!
Flags: needinfo?(eshepherd)
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/676322e0166c because surprise! talos was depending on it and broke.
Turns out this broke Talos, because Talos uses memory reporters to get RSS:
http://mxr.mozilla.org/build/source/talos/talos/pageloader/chrome/memory.js#72

I know how to fix the code;  it's pretty easy.  But I don't know how to test and land a Talos patch.  jmaher, jhammel, can you tell me?
Attached patch Improve how Talos measures RSS. (obsolete) — — Splinter Review 
This fixes the problem.  It only gives the RSS for the main process, but then,
that's also true of the old code, despite the presence of the procName.split()
call.
Attachment #799981 - Flags: review?(jmaher)
Depends on: 912863
Comment on attachment 799981 [details] [diff] [review]
Improve how Talos measures RSS.

I moved the Talos patch into bug 912863.
Attachment #799981 - Attachment is obsolete: true
Attachment #799981 - Flags: review?(jmaher)
Blocks: 911641
Also appears to have caused timeouts in mochitest-1 on Ubuntu 32:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=ubuntu.*mochitest-1&tochange=676322e0166c&fromchange=e46601eb7279
(further retriggers pending, will confirm either way)

eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=27415002&tree=Mozilla-Inbound

Bizarre!
> Also appears to have caused timeouts in mochitest-1 on Ubuntu 32:

I was going to say "no way in hell it could cause that" but now I see that in the try run I did:
https://tbpl.mozilla.org/?tree=Try&rev=a9357bc4d46f.  I hadn't even looked at that because it seemed so unlikely.  This patch is *absolutely* unrelated to those tests.

Having said that, at the time of writing 3 of those retriggers have finished, and one of them completed.
> Having said that, at the time of writing 3 of those retriggers have
> finished, and one of them completed.

With some more time passing, by my count, while the patch was on m-i there were 24 runs of mochitest-1 on Ubuntu-32 and 20 of them timed out.  Argh.
Heh.

That's mozilla-central, as in, your regression range is between this backout which happened to be the tip of one merge and the tip of the prior merge, 87 changesets wide. You'll want to look at mozilla-inbound instead (but beware, then you'll get the bustage-fix push which was the first time we were able to build for half a dozen pushes).
Bug 912863's changes to Talos have landed, so that issue is fixed.

I'm still getting the baffling Linux opt32 mochitest-1 time-outs:  https://tbpl.mozilla.org/?tree=Try&rev=a2474ee792fd.  Why would we get time-outs for opt32 builds but not debug32 builds?  I wonder if I've hit a compiler bug :(
All of the failing tests have this:

  INFO -  106504 INFO Error: Unable to restore focus, expect failures and timeouts.

DXR tells me we have two existing tests that take specific actions in finish() to avoid this error --  content/tests/chrome/test_bug624329.xul and layout/forms/test/test_bug665540.html -- though they both only do it on Windows.

karlt, you've dealt with that error before (in bug 670053).  Any idea what might be going on here?  I'm  getting it about 90% of the time on opt32 Linux builds on TBPL -- no other platforms, not even debug32 Linux -- with a patch that seems entirely unrelated to anything that might cause that :(
Flags: needinfo?(karlt)
Attached image screenshot — 
I suspect this assertion failure is the cause.
Flags: needinfo?(karlt)
> I suspect this assertion failure is the cause.

Brilliant!  Thanks very much for this;  as soon as I saw that I was able to determine the problem immediately.  Did you just run this yourself on 32-bit Linux?  I tried and failed to do a 32-bit build today :/
I've filed bug 914092 to make debug.js NS_ASSERTs fail the test run in a more obvious way (we shouldn't be having to resort to local builds to find these! :-)).
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c9f3fb14995

Turns out the assertion failure was because I had both a uni-reporter and a multi-reporter named "ghost-windows".  When the multi-reporter was (unintentionally) invoked by telemetry, it would assert because it reported more than one measurement.  I don't know why this only caused focus problems on 32-bit Linux opt builds, though.  No matter, it's fixed now.
Dammit!

jmaher, are the Android test machines running a different version of talos?  https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=5c9f3fb14995 is my commit which had to be backed out because RSS wasn't being measured correctly on Android.
Flags: needinfo?(jmaher)
Flags: needinfo?(jhammel)
(In reply to Nicholas Nethercote [:njn] from comment #33)
> Did you just run this yourself on 32-bit Linux?

I didn't need to.  Mochitest logs on at least some tier-1 platforms have screenshots as data:image/png;base64 urls on the first timeout :)
> I didn't need to.  Mochitest logs on at least some tier-1 platforms have
> screenshots as data:image/png;base64 urls on the first timeout :)

Ah... and I see you can just copy and paste that into the address bar to view the image.  I did not know that.  Very useful!
Depends on: 914464
These are outlined on the main talos wiki (updated early last week):
https://wiki.mozilla.org/Buildbot/Talos/Misc#Steps_to_add_a_test_to_production

You just need to create a talos.zip file, then file a bug to have releng upload that to production, then make a change to talos.json.
Flags: needinfo?(jmaher)
Flags: needinfo?(jhammel)
https://hg.mozilla.org/mozilla-central/rev/3fe9649ebd84
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Seems this broke c-c:

/home/buildslave/comm-central-i386/build/mailnews/db/msgdb/src/nsMsgDatabase.cpp: In constructor 'nsMsgDatabase::nsMsgDatabase()':
/home/buildslave/comm-central-i386/build/mailnews/db/msgdb/src/nsMsgDatabase.cpp:1156:59: error: cannot allocate an object of abstract type 'mozilla::mailnews::MsgDBReporter'
/home/buildslave/comm-central-i386/build/mailnews/db/msgdb/src/nsMsgDatabase.cpp:1050:7: note:   because the following virtual functions are pure within 'mozilla::mailnews::MsgDBReporter':
../../../../mozilla/dist/include/nsIMemoryReporter.h:113:14: note: 	virtual nsresult nsIMemoryReporter::GetName(nsACString_internal&)
../../../../mozilla/dist/include/nsIMemoryReporter.h:116:14: note: 	virtual nsresult nsIMemoryReporter::CollectReports(nsIMemoryReporterCallback*, nsISupports*)

http://tbpl-dev.callek.net/php/getParsedLog.php?id=20805721&tree=SeaMonkey#error0
http://buildbot.rhaalovely.net/builders/comm-central-i386/builds/409/steps/build/logs/stdio
Also seen in Thunderbird:
https://tbpl.mozilla.org/php/getParsedLog.php?id=27755719&tree=Thunderbird-Trunk#error0

../../../../../mailnews/db/msgdb/src/nsMsgDatabase.cpp: In member function 'virtual nsrefcnt nsMsgDBService::Release()':
../../../../../mailnews/db/msgdb/src/nsMsgDatabase.cpp:78:769: warning: deleting object of polymorphic class type 'nsMsgDBService' which has non-virtual destructor might cause undefined behaviour [-Wdelete-non-virtual-dtor]
../../../../../mailnews/db/msgdb/src/nsMsgDatabase.cpp: In constructor 'nsMsgDatabase::nsMsgDatabase()':
../../../../../mailnews/db/msgdb/src/nsMsgDatabase.cpp:1156:59: error: cannot allocate an object of abstract type 'mozilla::mailnews::MsgDBReporter'
../../../../../mailnews/db/msgdb/src/nsMsgDatabase.cpp:1050:7: note:   because the following virtual functions are pure within 'mozilla::mailnews::MsgDBReporter':
In file included from ../../../../../mailnews/db/msgdb/src/nsMsgDatabase.cpp:50:0:
../../../../mozilla/dist/include/nsIMemoryReporter.h:113:60: note: 	virtual nsresult nsIMemoryReporter::GetName(nsACString_internal&)
../../../../mozilla/dist/include/nsIMemoryReporter.h:116:60: note: 	virtual nsresult nsIMemoryReporter::CollectReports(nsIMemoryReporterCallback*, nsISupports*)
make[9]: *** [nsMsgDatabase.o] Error 1
Apologies for the c-c bustage, and thanks for the fix!
(In reply to Nicholas Nethercote [:njn] from comment #18)
> > Looks like ancient code. I'd just remove it or ignore it, we clearly aren't
> > using it
> 
> I added a comment explaining that it's busted.

I filed bug 916501.
Depends on: 918882
(In reply to Nicholas Nethercote [:njn] from comment #20)
> I have updated
> https://wiki.mozilla.org/Platform/Memory_Reporting#Memory_Reporters
> 
> sheppy, the patch that just landed did the following:
> - Removed nsIMemoryReporter.
> - Renamed nsIMemoryMultiReporter as nsIMemoryReporter.
> 
> Those two classes are documented here:
> 
> https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/
> nsIMemoryReporter
> https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/
> nsIMemoryMultiReporter
> 
> and there are two associated classes:
> 
> https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/
> nsIMemoryMultiReporterCallback
> https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/
> nsIMemoryReporterManager
> 
> How should I document the change?  The easiest thing would be document
> things as they are now -- e.g. delete the nsIMemoryMultiReporter page -- but
> I think you'll want to keep some info about how things used to be around? 
> Let me know.  Thanks!

Sorry to take so long to reply on this! Yes, we need to delete nsIMemoryMultiReporter, but add an {{obsolete}} box at the top, with a note to use the new technique instead.
Flags: needinfo?(eshepherd)
Nicholas, I got a report from the AMO validation service about MemChaser. I actually missed that in the last couple of months, but wonder if we have to update something or not. Can you please have a look?

https://github.com/mozilla/memchaser/blob/master/extension/lib/memory.js#L18
Flags: needinfo?(n.nethercote)
Henrik, AFAICT the only way you use nsIMemoryReporterManager is to read its |resident| property, which hasn't changed. So that seems fine. Is MemChaser still working as expected? What did the validation report say?
Flags: needinfo?(n.nethercote)
(In reply to Nicholas Nethercote [:njn] from comment #51)
> Henrik, AFAICT the only way you use nsIMemoryReporterManager is to read its
> |resident| property, which hasn't changed. So that seems fine. Is MemChaser
> still working as expected? What did the validation report say?

So far I haven't seen a problem with it. All seems to work as usual. The wording for that for Firefox 26.0 is:

Some memory reporting interfaces have been changed.
Error: The memory reporting interfaces have changed to remove uni-reporters.
See bug https://bugzilla.mozilla.org/show_bug.cgi?id=910517 for more information.

I assume I don't have to change anything. Thanks.
The amo-validator is not clever enough to determine the exact usage in an add-on and if it is affected... It is just a regex test for r"nsIMemory(Multi)?Reporter(Callback|Manager)?".
https://github.com/mozilla/amo-validator/blob/631efe1790c54eabbc3032a86916624a89da0a63/validator/testcases/regex.py#L1223

I submitted two memchaser issues, https://github.com/mozilla/memchaser/issues/192 (resident vs. residentFast) and https://github.com/mozilla/memchaser/issues/193 (minimizeMemory implementation).
Thanks Nils! All that makes sense. We will try to get those updates into the 0.6 release.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: