Last Comment Bug 687724 - Per-tab reporting in about:memory
: Per-tab reporting in about:memory
Status: RESOLVED FIXED
[MemShrink:P1]
: meta, relnote
Product: Toolkit
Classification: Components
Component: about:memory (show other bugs)
: unspecified
: All All
: -- normal with 13 votes (vote)
: mozilla16
Assigned To: Nicholas Nethercote [:njn]
:
:
Mentors:
: 670580 (view as bug list)
Depends on: cpg 713799 755186 772615 774534
Blocks: 400120 704623 711248
  Show dependency treegraph
 
Reported: 2011-09-19 16:39 PDT by Luke Wagner [:luke]
Modified: 2012-11-22 15:38 PST (History)
55 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Per-tab reporting in about:memory. (10.22 KB, patch)
2012-06-25 23:17 PDT, Nicholas Nethercote [:njn]
no flags Details | Diff | Splinter Review
patch, v2 (13.10 KB, patch)
2012-06-26 21:56 PDT, Nicholas Nethercote [:njn]
no flags Details | Diff | Splinter Review
(part 1) - Convert JSCompartment::gcRunning to gcState. (71.86 KB, patch)
2012-06-29 00:05 PDT, Nicholas Nethercote [:njn]
wmccloskey: review+
Details | Diff | Splinter Review
(part 2) - Make JSCompartment::global() fallible. (3.93 KB, patch)
2012-06-29 00:06 PDT, Nicholas Nethercote [:njn]
luke: review+
Details | Diff | Splinter Review
(part 3) - Report JS memory consumption for compartments that are associated with |window| objects under "window-objects". (25.47 KB, patch)
2012-06-29 00:07 PDT, Nicholas Nethercote [:njn]
justin.lebar+bug: review+
luke: review+
bobbyholley: review+
Details | Diff | Splinter Review
(part 4) - Re-indent JSMemoryMultiReporter's methods. (10.96 KB, patch)
2012-06-29 00:10 PDT, Nicholas Nethercote [:njn]
bobbyholley: review+
Details | Diff | Splinter Review
(part 1) - Convert JSCompartment::gcRunning to gcState, v2 (109.39 KB, patch)
2012-07-03 17:15 PDT, Nicholas Nethercote [:njn]
wmccloskey: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2011-09-19 16:39:59 PDT
With compartment-per-global (bug 650353), there will be many many more compartments than there are now and about:memory will get swamped.  This coincides nicely with a feature request (from Asa, among others) that memory be reported per-tab.  With compartment-per-domain, we aren't able to do this easily, but since every tab contains at least one global, this should be easy.

Of course, we should be able to drilldown into per-tab usage (via ?verbose or interactive clicking).  The first level of subdivision could partition globals by bfcache entry.  This would help to avoid the current confusion the bfcache causes in about:memory (bug 685758).  Below that, we could partition globals by the principals->codebase.

For the system compartment, there are also >100 globals.  This also coincides nicely with the feature request of reporting per-addon data.  Thus, this bug should try to partition system globals into addons and any other meaningful front-end sources of globals.
Comment 1 Nicholas Nethercote [:njn] 2011-09-27 16:40:19 PDT
FWIW, if you have multiple reporters with the same path they'll be aggregated into a single entry in about:memory, with a number in square brackets (e.g. "[4]") indicating how many same-named reporters were aggregated.  So the swamping may be less than feared.

(Note that for system compartments we currently deliberately include the compartment address to avoid this aggregation, and there are efforts such as bug 681556 to include more detailed info for system compartments.)

But yes, per-tab reporters would be cool, esp. because it's what users want.  Getting the exact tree hierarchy right is a little tricky -- you don't want it to get too deep, for example -- as is deciding on the presentation (such as what is collapsible).
Comment 2 Nicholas Nethercote [:njn] 2011-11-15 11:52:29 PST
Luke, will it be possible to auto-mark zombie compartments?  That would be fantastically helpful, esp. for add-on authors.
Comment 3 Luke Wagner [:luke] 2011-11-15 15:15:47 PST
(In reply to Nicholas Nethercote [:njn] from comment #2)
Great question.  I'm not the best person to ask but I think a first stab would be to detect content compartments (i.e., compartments whose global is some content window) that are not reachable via the standard content compartment "roots" such as being visible or in the bfcache.

Maybe bz has a better idea?
Comment 4 Nicholas Nethercote [:njn] 2011-11-15 15:19:49 PST
If we can put them in groups "visible", "bfcache", "other" and it's clear that ones in "other" might be zombies, that would help.
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2011-11-15 20:26:37 PST
Once we have compartment per global, we can absolutely keep track, for each global, whether it's in bfcache, visible, or "zombie".
Comment 6 Nicholas Nethercote [:njn] 2011-11-29 18:30:27 PST
One complicating factor is that I suspect with CPG we'll have to move various things out of compartments to avoid memory blow-ups.  For example, we recently moved regexp code out of compartments into threads, which means you can't blame regexp code usage on individual compartments any more.  If we have to do that for other things (e.g. mjit code) that'll make this per-tab reporting harder :(
Comment 7 Luke Wagner [:luke] 2011-11-29 20:32:30 PST
(In reply to Nicholas Nethercote [:njn] from comment #6)
Hopefully we can attribute most of the memory in these shared allocators/pools to specific compartments and just attribute the leftover slop to the runtime.  With a shared/cached resource (like RegExpPrivate) we run into the same problem as the os with shared pages (do it proportionally?), but other things (like mjit code) should still have a simple 1:1 association with compartments.
Comment 8 Nicholas Nethercote [:njn] 2012-01-22 02:16:09 PST
*** Bug 670580 has been marked as a duplicate of this bug. ***
Comment 9 Nicholas Nethercote [:njn] 2012-05-14 21:44:45 PDT
There are two obvious ways to approach this:

1. Iterate over all the windows.

2. Iterate over all the compartments.

Option 2 is what we currently use in the JS memory reporter.  But there's no way to get the nsGlobalWindow of a JSCompartment, AFAIK.

Option 1 would work, because AFAICT you can get the JSCompartment of an nsGlobalWindow (via nsGlobalWindow::mJSObject).  But I'm worried that then we'd miss zombie compartments -- could we have compartments that don't have an nsGlobalWindow?

Going back to option 2, maybe I could put a pointer to the nsGlobalWindow in the CompartmentPrivate?

Any guidance on the relationship between nsGlobalWindows and JSCompartments would be welcome!
Comment 10 Andrew McCreight [:mccr8] 2012-05-14 22:09:42 PDT
Well, if an object has a pointer to a global window, you can certainly dredge it out while you are traversing over the entire heap, though it isn't particularly pretty.  You would basically look for objects that have the PRIVATE_IS_NSISUPPORTS flag or whatnot, then attempt to QI it to nsGlobalWindow.  I'm not sure if that's actually useful.
Comment 11 Nicholas Nethercote [:njn] 2012-05-14 22:14:48 PDT
(This is the result of discussion with bholley on IRC.)

Every global has a compartment:  DOM globals (a.k.a. windows), components, 
JS modules, workers, etc.  nsGlobalWindow implements the C++ side of DOM 
globals.

So I should iterate over all JSCompartments.  For each one, I need to
determine if it is actually a window object.  To do that:

- I use JSCompartment::globalObject (which Luke will add in bug 755186).

- Specifically, I do |GetNativeOfWrapper(compartment->globalObject)|, which
  gives me an |nsISupports *|.  
  
- I can then QI that;  if it's an instance of NSPIDOMWindow then it's an 
  nsGlobalWindow, and I can report it under the appropriate "window-objects"
  entry in about:memory.
Comment 12 Johnny Stenback (:jst, jst@mozilla.com) 2012-05-15 09:17:25 PDT
But note that there can be window objects in memory that have no compartment (i.e. all JS stuff is gone, but it's leaked by C++ code), and there can be compartments for which there are no window objects (sandboxes etc). So I think we ultimately need to enumerate both and report the intersection of them as tabs and the outliers as either a compartment and a window.
Comment 13 Nicholas Nethercote [:njn] 2012-05-15 17:35:20 PDT
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #12)
> But note that there can be window objects in memory that have no compartment
> (i.e. all JS stuff is gone, but it's leaked by C++ code), and there can be
> compartments for which there are no window objects (sandboxes etc). So I
> think we ultimately need to enumerate both and report the intersection of
> them as tabs and the outliers as either a compartment and a window.

That's what I'm planning.  (But comment 11 didn't make it clear that I intend to keep the current window enumeration.)  Thanks for the clarification!
Comment 14 Nicholas Nethercote [:njn] 2012-06-25 23:17:20 PDT
Created attachment 636606 [details] [diff] [review]
Per-tab reporting in about:memory.

This is a very rough draft patch.  It incorporates Bobby's patch from
767942.  Basic cases work, viz:

│  ├───5,514,760 B (06.21%) -- top(http://www.mozilla.org/en-US/, id=6)
│  │   ├──4,963,504 B (05.59%) -- active/window(http://www.mozilla.org/en-US/)
│  │   │  ├──3,809,744 B (04.29%) ++ js/compartment(http://www.mozilla.org/en-US/)
│  │   │  ├────642,400 B (00.72%) ++ layout
│  │   │  ├────299,072 B (00.34%) ++ dom
│  │   │  └────212,288 B (00.24%) ── style-sheets

Note that the "js" sub-tree is beneath "window-objects".

But I'm hitting some problems.

- The JS memory reporter uses IterateCompartmentsArenasCells().  One of the
  first things that function does is set rt->gcRunning, via an
  AutoHeapSession object.  But I need to call JS_GetGlobalForCompartment()
  while iterating, and it has a AssertNoGC() call.  I've just commented out
  the guts of AssertNoGC() for now, but obviously that needs addressing.
  Any suggestions?  
  
  (I tried reinstating the assertion and just removing the AutoHeapSession
  object from IterateCompartmentsArenasCells().  It seems to work...)

- JS_GetCompartmentForGlobal() is sometimes returning dead objects.  (I know
  this because I sometimes get crashes involving 0xdadadada values.)  Is
  this expected?

- JSCompartment::global() is sometimes asserting:
  
    js::GlobalObject &global() const {
        JS_ASSERT(global_->compartment() == this);  // asserts
        return *global_; 
    }

  I have no idea why, but it's not encouraging.

- JSCompartment::global() returns a reference rather than a pointer, which
  suggests that all compartment have a global.  But that's not true -- the
  atoms compartment doesn't have a global.  So if you call
  JSCompartment::global() on the atoms compartment you get an assertion
  failure (debug builds) or a crash (opt builds).

  This, combined with the above two issues, suggest to me that
  JSCompartment::global() still has some kinks to iron out...

- In the reporter I have a JSRuntime but no JSContext.  But
  JS_GetGlobalForCompartment() and GetNativeOfWrapper() both need a
  JSContext, so I just build a new one and destroy it again immediately
  afterwards.  Pretty gross -- especially since I do it once per
  compartment! -- but it works for now.  A better alternative would be
  nicer.  It feels like some of our APIs could take either a JSContext
  or a JSRuntime and still work, but there's a lack of consistency which can
  create impedance mismatches like this one.
Comment 15 Nicholas Nethercote [:njn] 2012-06-25 23:19:00 PDT
> │ 
> ├──ÃÃ
> ‚¢Ã‚”€5,514,760 B (06.21%) --
> top(http://www.mozilla.org/en-US/, id=6)

Wow, |hg bzexport| doesn't handle non-ASCII chars well.  That's meant to look like this:

│  ├───5,514,760 B (06.21%) -- top(http://www.mozilla.org/en-US/, id=6)
│  │   ├──4,963,504 B (05.59%) -- active/window(http://www.mozilla.org/en-US/)
│  │   │  ├──3,809,744 B (04.29%) ++ js/compartment(http://www.mozilla.org/en-US/)
│  │   │  ├────642,400 B (00.72%) ++ layout
│  │   │  ├────299,072 B (00.34%) ++ dom
│  │   │  └────212,288 B (00.24%) ── style-sheets
Comment 16 Ted Mielczarek [:ted.mielczarek] 2012-06-26 08:42:56 PDT
Yuck, file a bzexport bug on that? (Probably gets mangled coming in from the external editor.)
Comment 17 Bill McCloskey (:billm) 2012-06-26 11:56:40 PDT
The AssertNoGC in JS_GetGlobalForCompartment can be removed. It's there to ensure we don't try to run JS scripts during GC, but it's not that important.

For the other stuff, I'll try to pretend it's not my fault until Luke can prove otherwise.
Comment 18 Luke Wagner [:luke] 2012-06-26 12:02:10 PDT
For the other issues, file a bug with a testcase and we can find out what is happening.
Comment 19 Nicholas Nethercote [:njn] 2012-06-26 21:26:16 PDT
> The AssertNoGC in JS_GetGlobalForCompartment can be removed. It's there to
> ensure we don't try to run JS scripts during GC, but it's not that important.

Can I remove the AssertNoGC() in JS_SaveFrameChain() and JS_RestoreFrameChain()
as well?  I hit the former with the following stack trace, and the latter with
a very similar one:

#5  0x00007fae54195dc0 in AssertNoGC (rt=<optimised out>)
    at /home/njn/moz/mi3/js/src/jsapi.cpp:222
#6  0x00007fae54195dde in AssertNoGC (cx=<optimised out>)
    at /home/njn/moz/mi3/js/src/jsapi.cpp:228
#7  0x00007fae541a54a1 in JS_SaveFrameChain (cx=0x7fae388aaa80)
    at /home/njn/moz/mi3/js/src/jsapi.cpp:5595
#8  0x00007fae536258c8 in XPCJSContextStack::Push (this=0x7fae555c6790,
    cx=0x7fae2fcd78b0)
    at /home/njn/moz/mi3/js/xpconnect/src/XPCJSContextStack.cpp:104
#9  0x00007fae535ee29f in XPCCallContext::Init (this=0x7fff10cdb530,
    callerLanguage=<optimised out>, callBeginRequest=1, obj=0x0, funobj=0x0,
    wrapperInitOptions=XPCCallContext::INIT_SHOULD_LOOKUP_WRAPPER, name=...,
    argc=4294967295, argv=0x0, rval=0x0)
    at /home/njn/moz/mi3/js/xpconnect/src/XPCCallContext.cpp:101
#10 0x00007fae535ee734 in XPCCallContext::XPCCallContext (
    this=<optimised out>, callerLanguage=XPCContext::LANG_NATIVE,
    cx=0x7fae2fcd78b0, obj=0x0, funobj=0x0, name=..., argc=4294967295,
    argv=0x0, rval=0x0)
    at /home/njn/moz/mi3/js/xpconnect/src/XPCCallContext.cpp:32
#11 0x00007fae535e5d42 in nsXPConnect::GetNativeOfWrapper (
    this=<optimised out>, aJSContext=0x7fae2fcd78b0, aJSObj=0x7fae3f203060)
    at /home/njn/moz/mi3/js/xpconnect/src/nsXPConnect.cpp:1428
Comment 20 Nicholas Nethercote [:njn] 2012-06-26 21:56:48 PDT
Created attachment 636996 [details] [diff] [review]
patch, v2

> For the other issues, file a bug with a testcase and we can find out what is
> happening.

I'll just do it here, because you'll need this patch to reproduce.

Steps to reproduce:

- Apply the attached patch.

- Do a debug browser build.

- Start browser, open techcrunch.com, wait for it to mostly or entirely load.

- In a second tab, open about:memory.  Reload it repeatedly until one of the two
  assertion failures hit;  it usually takes me 5--10 reloads, but occasionally
  more.

I'll put details about the two assertion failures in the subsequent comments.
Comment 21 Nicholas Nethercote [:njn] 2012-06-26 22:00:27 PDT
Asertion failure 1.  Sometimes you'll get this:

<signal handler called>
#5  0x00007f799cd3f141 in global (this=0x7f797aa10000)
    at /home/njn/moz/mi3/js/src/jscompartment.h:123
#6  JS_GetGlobalForCompartment (cx=<optimised out>, c=0x7f797aa10000)
    at /home/njn/moz/mi3/js/src/jsapi.cpp:2227
#7  0x00007f799c1bf4c4 in XPCJSRuntimeStats::initExtraCompartmentStats (
    this=<optimised out>, c=0x7f797aa10000, cstats=0x7f7986f27200)
    at /home/njn/moz/mi3/js/xpconnect/src/XPCJSRuntime.cpp:1701
#8  0x00007f799cfb2a7c in JS::StatsCompartmentCallback (rt=<optimised out>,
    data=0x7fffc2861850, compartment=0x7f797aa10000)
    at /home/njn/moz/mi3/js/src/MemoryMetrics.cpp:62
#9  0x00007f799cdceddf in js::IterateCompartmentsArenasCells (
    rt=0x7f7989423000, data=0x7fffc2861850,
    compartmentCallback=0x7f799cfb27ab <JS::StatsCompartmentCallback(JSRuntime*,
 void*, JSCompartment*)>,
    arenaCallback=0x7f799cfb1969 <JS::StatsArenaCallback(JSRuntime*, void*, js::
gc::Arena*, JSGCTraceKind, size_t)>,
    cellCallback=0x7f799cfb1a00 <JS::StatsCellCallback(JSRuntime*, void*, void*,
 JSGCTraceKind, size_t)>) at /home/njn/moz/mi3/js/src/jsgc.cpp:4020
#10 0x00007f799cfb2542 in JS::CollectRuntimeStats (rt=0x7f7989423000,
    rtStats=0x7fffc2861850) at /home/njn/moz/mi3/js/src/MemoryMetrics.cpp:197
#11 0x00007f799c1bd850 in JSMemoryMultiReporter::CollectReports (
    this=<optimised out>, cb=0x7f798718e400, closure=0x7f798718efc0)
    at /home/njn/moz/mi3/js/xpconnect/src/XPCJSRuntime.cpp:1743
#12 0x00007f799c97ab8d in nsMemoryReporterManager::GetExplicit (
    this=<optimised out>, aExplicit=0x7fffc2861f30)
    at /home/njn/moz/mi3/xpcom/base/nsMemoryReporterManager.cpp:807
#13 0x00007f799c97a126 in GetExplicit (n=0x7fffc2861f30)
    at /home/njn/moz/mi3/xpcom/base/nsMemoryReporterManager.cpp:504
#14 MemoryReporter_Explicit::GetAmount (this=<optimised out>,
    amount=<optimised out>)
    at /home/njn/moz/mi3/xpcom/base/nsMemoryReporterManager.cpp:507
#15 0x00007f799c97ccc2 in NS_InvokeByIndex_P (that=<optimised out>,
    methodIndex=7, paramCount=<optimised out>, params=<optimised out>)
    at /home/njn/moz/mi3/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_uni
x.cpp:163
#16 0x00007f799c1df2aa in Invoke (this=0x7fffc2861ef0)
    at /home/njn/moz/mi3/js/xpconnect/src/XPCWrappedNative.cpp:3071
#17 CallMethodHelper::Call (this=0x7fffc2861ef0)

which is the assertion in JSCompartment::global() failing:

    js::GlobalObject &global() const {
        JS_ASSERT(global_->compartment() == this);
        return *global_;
    }

When I first reproduced this just now, global_->compartment() was NULL and global_'s fields were all 0x0.

But then I reproduced again and both were non-NULL, but not equal, whereupon I saw this:

  (gdb) p this->global_
  $11 = (js::GlobalObject *) 0x7f5ff982d060

  (gdb) p this->global_->compartment()->global_
  $12 = (js::GlobalObject *) 0x7f5ff980c060

  (gdb) p this->global_->compartment()->global_->compartment()->global_
  $13 = (js::GlobalObject *) 0x7f5ff980c060

and this:

  (gdb) p this
  $16 = (const JSCompartment * const) 0x7f5ff3a5e800

  (gdb) p this->global_->compartment()
  $17 = (JSCompartment *) 0x7f5fe7307800

  (gdb) p this->global_->compartment()->global_->compartment()
  $18 = (JSCompartment *) 0x7f5fe7307800

Looks like the global<->compartment pointers are inconsistent.
Comment 22 Nicholas Nethercote [:njn] 2012-06-26 22:03:10 PDT
Asertion failure 2.  Sometimes you'll get this:

#4  <signal handler called>
#5  0x00007fb6df33661c in GetObjectClass (obj=0x7fb6b650d060)
    at ../../../dist/include/jsfriendapi.h:326
#6  XPCWrappedNative::GetWrappedNativeOfJSObject (cx=0x7fb6b2da3a80,
    obj=<optimised out>, funobj=<optimised out>, pobj2=0x7fffcefe0718,
    pTearOff=0x0)
    at /home/njn/moz/mi3/js/xpconnect/src/XPCWrappedNative.cpp:1807
#7  0x00007fb6df2e5d9c in nsXPConnect::GetNativeOfWrapper (
    this=<optimised out>, aJSContext=0x7fb6b2da3a80, aJSObj=0x7fb6b650d060)
    at /home/njn/moz/mi3/js/xpconnect/src/nsXPConnect.cpp:1437
#8  0x00007fb6df31e4db in XPCJSRuntimeStats::initExtraCompartmentStats (
    this=<optimised out>, c=<optimised out>, cstats=0x7fb6bb4ccf20)
    at /home/njn/moz/mi3/js/xpconnect/src/XPCJSRuntime.cpp:1702
#9  0x00007fb6e0111a7c in JS::StatsCompartmentCallback (rt=<optimised out>,
    data=0x7fffcefe0a30, compartment=0x7fb6b7704800)
    at /home/njn/moz/mi3/js/src/MemoryMetrics.cpp:62
#10 0x00007fb6dff2dddf in js::IterateCompartmentsArenasCells (
    rt=0x7fb6cc523000, data=0x7fffcefe0a30,
    compartmentCallback=0x7fb6e01117ab <JS::StatsCompartmentCallback(JSRuntime*,
 void*, JSCompartment*)>,
    arenaCallback=0x7fb6e0110969 <JS::StatsArenaCallback(JSRuntime*, void*, js::
gc::Arena*, JSGCTraceKind, size_t)>,
    cellCallback=0x7fb6e0110a00 <JS::StatsCellCallback(JSRuntime*, void*, void*,
 JSGCTraceKind, size_t)>) at /home/njn/moz/mi3/js/src/jsgc.cpp:4020
#11 0x00007fb6e0111542 in JS::CollectRuntimeStats (rt=0x7fb6cc523000,
    rtStats=0x7fffcefe0a30) at /home/njn/moz/mi3/js/src/MemoryMetrics.cpp:197
#12 0x00007fb6df31c850 in JSMemoryMultiReporter::CollectReports (
    this=<optimised out>, cb=0x7fb6b3160180, closure=0x7fb6b3160380)
    at /home/njn/moz/mi3/js/xpconnect/src/XPCJSRuntime.cpp:1743
#13 0x00007fb6dfad9b8d in nsMemoryReporterManager::GetExplicit (
    this=<optimised out>, aExplicit=0x7fffcefe1110)
    at /home/njn/moz/mi3/xpcom/base/nsMemoryReporterManager.cpp:807
#14 0x00007fb6dfad9126 in GetExplicit (n=0x7fffcefe1110)
    at /home/njn/moz/mi3/xpcom/base/nsMemoryReporterManager.cpp:504
#15 MemoryReporter_Explicit::GetAmount (this=<optimised out>,
    amount=<optimised out>)
    at /home/njn/moz/mi3/xpcom/base/nsMemoryReporterManager.cpp:507
#16 0x00007fb6dfadbcc2 in NS_InvokeByIndex_P (that=<optimised out>,
    methodIndex=7, paramCount=<optimised out>, params=<optimised out>)
    at /home/njn/moz/mi3/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_uni
x.cpp:163
#17 0x00007fb6df33e2aa in Invoke (this=0x7fffcefe10d0)
    at /home/njn/moz/mi3/js/xpconnect/src/XPCWrappedNative.cpp:3071

That's in this code:

    inline js::Class *
    GetObjectClass(const JSObject *obj)
    {
        return reinterpret_cast<const shadow::Object*>(obj)->shape->base->clasp;
    }

|reinterpret_cast<const shadow::Object*>(obj)->shape| is 0xdadadadadadadada,
which indicates that |obj| is dead.  This particular |obj| is the return value from the JS_GetGlobalForCompartment() call in
XPCJSRuntimeStats::initExtraCompartmentStats().

What's preventing JSCompartment::global_ from being collected once all other references to it have disappeared?  JSCompartment doesn't have a trace() method, AFAICT.
Comment 23 Nicholas Nethercote [:njn] 2012-06-27 00:20:58 PDT
> Looks like the global<->compartment pointers are inconsistent.

I used some diagnostic printfs and concluded that global->compartment() is changing for some globals.  Consider the code:

  JSCompartment *
  Cell::compartment() const
  {
      return arenaHeader()->compartment;
  }

arenaHeader() can't change, because it's just |global| with the lower bits masked.  So arenaHeader()->compartment must have changed, and my diagnostic printfs confirm this.

So, why would ArenaHeader::compartment change for an in-use arena?  Perhaps if the GC didn't realize the arena was in-use.  Hmm, that reminds me of this:

> What's preventing JSCompartment::global_ from being collected once all other
> references to it have disappeared?  JSCompartment doesn't have a trace()
> method, AFAICT.

Maybe the global is the only live thing in the arena, but the GC doesn't know about the JSCompartment::global reference, so it collects the object, frees the arena, and then later starts using the arena again for a different compartment?
Comment 24 Nicholas Nethercote [:njn] 2012-06-27 00:35:14 PDT
> What's preventing JSCompartment::global_ from being collected once all other
> references to it have disappeared?  JSCompartment doesn't have a trace()
> method, AFAICT.

Thinking some more:  JSCompartment::global_ must be a weak reference, otherwise the compartment could never be empty and never be freed.

But a compartment's global must be collected before the compartment can be destroyed, so there must be a period in each compartment's life when it lacks a global.

So, I think JSCompartment::global() is too optimistic -- it must be able to fail in the case where the global has been collected.  (And also in the case where the compartment never had a global, i.e. the atoms compartment.)  I think this accounts for both of the assertion failures I'm seeing.
Comment 25 Nicholas Nethercote [:njn] 2012-06-27 20:36:45 PDT
> So, I think JSCompartment::global() is too optimistic -- it must be able to
> fail in the case where the global has been collected.  (And also in the case
> where the compartment never had a global, i.e. the atoms compartment.)  I
> think this accounts for both of the assertion failures I'm seeing.

With Bill's help I have a patch that sets JSCompartment::global_ to NULL when the global object is collected, and I've added JSCompartment::maybeGlobal(), which can return NULL.  With those two changes, I'm no longer seeing either assertion failure.
Comment 26 Nicholas Nethercote [:njn] 2012-06-27 23:45:46 PDT
Another interesting problem with this bug:  the JS multi-reporter must create paths like "explicit/window-objects/top(<URL>)/active/window(<URL>)/js/...".  But creating that stuff involves all sorts of Gecko-ish code, and the JS multi-reporter lives in XPConnect.  Paths involving ghost windows are particularly tricky, and difficult to do outside of nsWindowMemoryReporter.

I think the easiest thing will be to effectively merge JSMemoryMultiReporter into  nsWindowMemoryReporter.  JSMemoryMultiReporter's code will still live in XPConnect, but it won't be a standalone reporter, and that code will be invoked by nsWindowMemoryReporter.  The advantage of this is that nsWindowMemoryReporter can generate the paths for all the window objects and pass in a table to the JS reporter code, which can just look them up.
Comment 27 Andrew McCreight [:mccr8] 2012-06-28 08:07:08 PDT
I was having similar problems with my orphan node patch, which uses JS engine hooks to find orphan nodes, but you want to give URL information about the orphans.
Comment 28 Luke Wagner [:luke] 2012-06-28 11:56:08 PDT
So, am I correct in my understanding that the problem is we are in a state, outside of normal GC, where a compartment contains live objects but the global of the compartment has been finalized?

I was talking w/ billm about this earlier and I think it is possible b/c some objects (compiler created functions are one, strings are another, I think scripts are a third) that have a NULL parent (or no parent, for strings) and thus do not root their global.  Instead of adding JSCompartment::maybeGlobal (which will confuse all callers and may limit its utility), could we instead change ScanBaseShape with:

   if (JSObject *parent = base->getObjectParent())
       PushMarkStack(gcmarker, parent);
 + else
 +     PushMarkStack(base->compartment()->global());

and tweak scripts/strings to do similarly?  Bill was already saying that, when we remove 'parent', this is probably what we'd want to do (unconditionally, of course).

With this, we'll have a pretty useful invariant.  To check it really well, it seems like we could note when we finalize a global object and then assert that the compartment is swept in the same GC.

Sound reasonable Bill?
Comment 29 Bill McCloskey (:billm) 2012-06-28 12:19:30 PDT
I talked this over with Luke. I think we have a reasonable solution.

For objects and scripts, it seems useful to have an infallible global() method. To support that, we should ensure that objects and scripts always mark their global (using the technique Luke mentioned in comment 28).

For other stuff (strings, for example), we don't want to have to scan the global every time. So the global() method on compartment will have to be fallible.
Comment 30 Luke Wagner [:luke] 2012-06-28 14:12:42 PDT
Also, JSContext::global() can be infallible.  Probably others too; in theory, most paths shows have the "infallible" invariant.
Comment 31 Nicholas Nethercote [:njn] 2012-06-29 00:05:35 PDT
Created attachment 637795 [details] [diff] [review]
(part 1) - Convert JSCompartment::gcRunning to gcState.

This patch:

- Changes JSRuntime::gcRunning into JSRuntime::gcState, a tri-valued field
  that is one of |Idle|, |GC|, or |Iterator|.  It's necessary to avoid
  various AssertNoGC() failures when doing things while iterating.

- Gives AutoHeapSession's constructor an additional parameter that indicates
  if rt->gcState should be set to |GC| or |Iterator|.

- Converts all instances of |rt->gcRunning| to |rt->gcHeapBusy()|, which
  succeeds if |gcState| is |GC| or |Iterator|.  This preserves the existing
  behaviour by treating |GC| and |Iterator| the same.  Later patches will
  distinguish the two as necessary.

- Renames AssertNoGC() as AssertIdle() in jsapi.cpp.
Comment 32 Nicholas Nethercote [:njn] 2012-06-29 00:06:30 PDT
Created attachment 637796 [details] [diff] [review]
(part 2) - Make JSCompartment::global() fallible.

This patch makes JSCompartment::global() fallible, renaming it maybeGlobal()
in the process to distinguish it from infallible global() methods in
JSObject and other classes.

I made the suggested change to ScanBaseShape (with a minor modification),
but I couldn't work out what the equivalent changes for objects and scripts
were (as suggested in comment 29).
Comment 33 Nicholas Nethercote [:njn] 2012-06-29 00:07:00 PDT
Created attachment 637797 [details] [diff] [review]
(part 3) - Report JS memory consumption for compartments that are associated with |window| objects under "window-objects".
Comment 34 Nicholas Nethercote [:njn] 2012-06-29 00:09:15 PDT
Comment on attachment 637797 [details] [diff] [review]
(part 3) - Report JS memory consumption for compartments that are associated with |window| objects under "window-objects".

> (part 3) - Report JS memory consumption for compartments that are associated
> with |window| objects under "window-objects".

This patch moves reports for JS compartments that are associated with
|window| objects under "window-objects".  An example:

|  +---4,055,688 B (06.03%) -- top(http://www.mozilla.org/en-US/, id=6)
|  |   +--3,240,320 B (04.82%) --
active/window(http://www.mozilla.org/en-US/)
|  |   |  +--1,933,624 B (02.88%) --
js/compartment(http://www.mozilla.org/en-US/)
|  |   |  |  +--1,028,096 B (01.53%) ++ gc-heap
|  |   |  |  +----282,848 B (00.42%) -- script-data
|  |   |  |  +----240,672 B (00.36%) ++ type-inference
|  |   |  |  +----224,336 B (00.33%) ++ shapes-extra
|  |   |  |  +-----98,080 B (00.15%) -- analysis-temporary
|  |   |  |  +-----52,480 B (00.08%) -- objects/slots
|  |   |  |  +------7,112 B (00.01%) -- other-sundries
|  |   |  +----685,408 B (01.02%) ++ layout
|  |   |  +----318,448 B (00.47%) -- style-sheets
|  |   |  +----302,840 B (00.45%) ++ dom

This effectively gives per-tab reports, but some follow-up re-organisation
(in a subsequent patch) will make it easier to read.

Lots of different reviewers for this...

- jlebar: can you review the small changes in nsWindowMemoryReporter.cpp?
  It now invokes the JS multi-reporter, passing in the window paths.

  Can you also review the test_memoryReporter.xul change?

- bent: can you review the WorkerPrivate.cpp change?  Worker stats are now
  reported under "explicit/workers" instead of "explicit/dom" -- the "dom"
  sub-tree has been gone for a while.  Also, there's minor tweaks to how the
  path is constructed.

  It would be nice to report worker
  memory consumption under the "window-objects" tree as well, but that's
  hard, and workers aren't used much so I punted.

- luke: can you review the js/src/* changes?  I've added
  JS_GetGlobalForCompartment(), which is not the JS_GetCurrentGlobal() that
  you wanted, but JS_GetCurrentGlobal() would require that I enter every
  compartment that I iterate over, which is a pain.

  I also loosened the assertion in JS_{Save,Restore}FrameChain, because I
  need them during compartment iteration.

- bholley: can you review the js/xpconnect/* changes?  I've changed how the
  compartment paths are constructed (that's the whole point of the patch),
  the important part is in initExtraCompartmentStats().

  To do this I had to change things so that JSMemoryMultiReporter is invoked
  from nsWindowMemoryReporter, instead of being registered as a standalone
  memory reporter;  this was so that nsWindowMemoryReporter could pass in
  information about paths associated with |window| objects.  This required
  adding the new public header XPCJSMemoryReporter.h.

Thanks, everyone!
Comment 35 Nicholas Nethercote [:njn] 2012-06-29 00:10:38 PDT
Created attachment 637798 [details] [diff] [review]
(part 4) - Re-indent JSMemoryMultiReporter's methods.

This patch just reindents JSMemoryMultiReporter's methods.  I did it in a
separate patch to make the previous patch easier to review.
Comment 36 Bobby Holley (:bholley) (busy with Stylo) 2012-06-29 02:16:47 PDT
Comment on attachment 637798 [details] [diff] [review]
(part 4) - Re-indent JSMemoryMultiReporter's methods.

rs=bholley
Comment 37 Bobby Holley (:bholley) (busy with Stylo) 2012-06-29 02:17:36 PDT
Comment on attachment 637797 [details] [diff] [review]
(part 3) - Report JS memory consumption for compartments that are associated with |window| objects under "window-objects".

Bouncing my review here to mccr8, who I think has a better understanding of the memory reporting stuff.
Comment 38 Luke Wagner [:luke] 2012-06-29 11:01:48 PDT
Comment on attachment 637796 [details] [diff] [review]
(part 2) - Make JSCompartment::global() fallible.

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

::: js/src/gc/Marking.cpp
@@ +620,5 @@
>          PushMarkStack(gcmarker, parent);
> +    } else {
> +        GlobalObject *global = base->compartment()->maybeGlobal();
> +        if (global)
> +            PushMarkStack(gcmarker, global);

if (GlobalObject *global = ...)
   PushMarkStack(gcmarker, global);

::: js/src/jscompartment.h
@@ +119,5 @@
>    private:
>      js::GlobalObject             *global_;
>    public:
> +    // Nb: global_ might be NULL, if (a) it's the atoms compartment, or (b) the
> +    // compartment's global has been collected.

Could you explain the latter a bit, like how it can happen.  Could you also explain that marking a JSObject always marks its global and thus JSObject::global() is infallible?  Lastly, could you put a "TODO: add infallible JSScript::global and JSContext::global".
Comment 39 Luke Wagner [:luke] 2012-06-29 11:04:21 PDT
Comment on attachment 637797 [details] [diff] [review]
(part 3) - Report JS memory consumption for compartments that are associated with |window| objects under "window-objects".

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

::: js/src/jsapi.cpp
@@ +229,4 @@
>  }
>  
>  static void
> +AssertIdleOrIterating(JSRuntime *rt)

To be a little more clear what "idle" means, how about AssertGCIdleOrIterating?

@@ +2233,4 @@
>  }
>  
>  JS_PUBLIC_API(JSObject *)
> +JS_GetGlobalForCompartment(JSContext *cx, JSCompartment *c)

I'd be explicit (and avoid 4 trivial null crashes over the next few years) and say JS_GetGlobalForCompartmentOrNull.
Comment 40 Bill McCloskey (:billm) 2012-06-29 12:16:17 PDT
Comment on attachment 637795 [details] [diff] [review]
(part 1) - Convert JSCompartment::gcRunning to gcState.

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

::: js/src/jsapi.cpp
@@ +217,4 @@
>  }
>  
>  static void
> +AssertIdle(JSRuntime *rt)

This name seems too generic. Maybe AssertGCIdle?

::: js/src/jscntxt.h
@@ +586,5 @@
> +    };
> +
> +    GCState             gcState;
> +
> +    bool gcHeapBusy() { return gcState != Idle; }

Can we call this heapBusy()?

::: js/src/jsgc.cpp
@@ +3413,4 @@
>   */
>  class AutoHeapSession {
>    public:
> +    AutoHeapSession(JSRuntime *rt, JSRuntime::GCState state);

Can you make state default to Iterating and only pass in GC for the AutoGCSession? This makes more sense to me.
Comment 41 Nicholas Nethercote [:njn] 2012-06-29 14:48:15 PDT
> >  class AutoHeapSession {
> >    public:
> > +    AutoHeapSession(JSRuntime *rt, JSRuntime::GCState state);
> 
> Can you make state default to Iterating and only pass in GC for the
> AutoGCSession? This makes more sense to me.

There are three places where we pass in |GC| that aren't within AutoGCSession, and one that is within AutoGCSession.  And there are four places where we pass in |Iterator|.  That doesn't feel like a good candidate for a default argument to me.
Comment 42 Bill McCloskey (:billm) 2012-06-29 14:54:01 PDT
(In reply to Nicholas Nethercote [:njn] from comment #41)
> > >  class AutoHeapSession {
> > >    public:
> > > +    AutoHeapSession(JSRuntime *rt, JSRuntime::GCState state);
> > 
> > Can you make state default to Iterating and only pass in GC for the
> > AutoGCSession? This makes more sense to me.
> 
> There are three places where we pass in |GC| that aren't within
> AutoGCSession, and one that is within AutoGCSession.  And there are four
> places where we pass in |Iterator|.  That doesn't feel like a good candidate
> for a default argument to me.

Sorry, I wasn't clear. I want the gcState to be Iterating for everything except AutoGCSession. So all the instances of AutoHeapSession would use the default argument. None of them are actually doing any collection.
Comment 43 Andrew McCreight [:mccr8] 2012-06-30 11:31:58 PDT
I'm not sure I'm any more familiar with this code than bholley, but I'll take a crack at it early next week.
Comment 44 Nicholas Nethercote [:njn] 2012-06-30 14:53:26 PDT
(In reply to Andrew McCreight [:mccr8] from comment #43)
> I'm not sure I'm any more familiar with this code than bholley, but I'll
> take a crack at it early next week.

I asked bholley for review because he was the one who explained to me how to get the nsGlobalWindow for a JSCompartment, which is the heart of the patch... along with the addition of the new header XPCJSMemoryReporter.h.

/me shrugs
Comment 45 Justin Lebar (not reading bugmail) 2012-07-02 03:40:53 PDT
Comment on attachment 637797 [details] [diff] [review]
(part 3) - Report JS memory consumption for compartments that are associated with |window| objects under "window-objects".

FYI: Looks like this patch was made with -U4 instead of -U8.

>diff --git a/dom/workers/WorkerPrivate.cpp b/dom/workers/WorkerPrivate.cpp
>--- a/dom/workers/WorkerPrivate.cpp
>+++ b/dom/workers/WorkerPrivate.cpp
>@@ -133,11 +133,18 @@ struct WorkerJSRuntimeStats : public JS:
>   virtual void initExtraCompartmentStats(JSCompartment *c,
>                                          JS::CompartmentStats *cstats) MOZ_OVERRIDE
>   {
>-    MOZ_ASSERT(!cstats->extra);
>+    MOZ_ASSERT(!cstats->extra1);
>+    MOZ_ASSERT(!cstats->extra2);
>     
>-    // ReportJSRuntimeExplicitTreeStats expects that cstats->extra is a char pointer
>-    const char *name = js::IsAtomsCompartment(c) ? "Web Worker Atoms" : "Web Worker";
>-    cstats->extra = const_cast<char *>(name);
>+    // Using NULL here means that we'll end up using
>+    // WorkerMemoryReporter::mPathPrefix as the path prefix, because that's
>+    // what is passed to xpc::ReportJSRuntimeExplicitTreeStats().

Could you say that mPathPrefix is passed as the "runtime prefix" (or
"rtPrefix")?  I was confused by this for a while.

>+    const char *cPathPrefix = NULL;
>+    cstats->extra1 = const_cast<char *>(cPathPrefix);
>+
>+    // ReportJSRuntimeExplicitTreeStats expects that cstats->extra[12] are char pointers.

Update comment?

>+    const char *cName = js::IsAtomsCompartment(c) ? "Web Worker Atoms" : "Web Worker";
>+    cstats->extra2 = const_cast<char *>(cName);

I'd rather do without the extra variables and static casts.  You can document
what extra1 and extra2 mean via a comment...

>diff --git a/js/xpconnect/src/XPCJSMemoryReporter.h b/js/xpconnect/src/XPCJSMemoryReporter.h
>new file mode 100644
>--- /dev/null
>+++ b/js/xpconnect/src/XPCJSMemoryReporter.h
>@@ -0,0 +1,34 @@
>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
>+ * vim: set ts=8 sw=4 et tw=78:
>+ *
>+ * This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#ifndef XPCJSMemoryReporter_h
>+#define XPCJSMemoryReporter_h
>+
>+#include "nsGlobalWindow.h"
>+
>+class nsISupports;
>+class nsIMemoryMultiReporterCallback;
>+
>+namespace xpc {
>+
>+typedef nsDataHashtable<nsPtrHashKey<nsGlobalWindow>, nsCString> WindowPaths;

I'd rather this be keyed off the window's ID (nsPIDOMWindow::WindowID()) than a raw pointer to the window.  That way, we don't have to worry about using stale pointers.

I'm not sold on the overall design here, though.  It seems that anything which wants to report per-tab memory usage is now going to have to be explicitly invoked by nsWindowMemoryReporter.  That's not so bad in and of itself, but then perhaps we should design this more generally: We have one multi-reporter at the top (what's currently nsWindowMemoryReporter), and you can register with it sub-multi-reporters which get the windowPaths argument.  Then the DOM and layout memory reporting can be separate from the main reporter as well.

We could put off re-designing this until later, if you wanted.  What's in this pach works well enough; it's just not designed in a particularly obvious or flexible way.  What do you think?

>     virtual void initExtraCompartmentStats(JSCompartment *c,
>                                            JS::CompartmentStats *cstats) MOZ_OVERRIDE {
>-        nsCAutoString name;
>-        GetCompartmentName(c, name);
>-        cstats->extra = strdup(name.get());
>+        nsCAutoString cName, cPathPrefix;
>+        GetCompartmentName(c, cName);
>+
>+        // Get the compartment's global.
>+        nsresult rv;
>+        JSObject *global = JS_GetGlobalForCompartment(mCx, c);
>+        if (global) {
>+            nsISupports *native = nsXPConnect::GetXPConnect()->GetNativeOfWrapper(mCx, global);
>+            nsCOMPtr<nsPIDOMWindow> piwindow = do_QueryInterface(native, &rv);
>+            if (NS_SUCCEEDED(rv)) {

Nit: We usually do |if (piwindow)| instead.

>+                // The global is a |window| object!  Get its path.
>+                nsGlobalWindow *window = static_cast<nsGlobalWindow*>(piwindow.get());
>+                if (mWindowPaths->Get(window, &cPathPrefix)) {
>+                    cPathPrefix.AppendLiteral("/js/");
>+                }

Another advantage of keying off the window's ID is that we don't have to add more ugly static casts, and maybe we don't have to include nsGlobalWindow.h here...


r=me as is, although if we decide to overhaul this as I suggested, I'd of course like to take another look.
Comment 46 Justin Lebar (not reading bugmail) 2012-07-02 03:44:55 PDT
>+    // ReportJSRuntimeExplicitTreeStats expects that cstats->extra[12] are char pointers.
>
> Update comment?

Ooh, you meant cstats->extra1 and cstats->extra2, not that cstats->extra was a char[12].  That really confused me; perhaps you can change it.  :)
Comment 47 Bobby Holley (:bholley) (busy with Stylo) 2012-07-02 05:14:19 PDT
Comment on attachment 637797 [details] [diff] [review]
(part 3) - Report JS memory consumption for compartments that are associated with |window| objects under "window-objects".

>+        // Get the compartment's global.
>+        nsresult rv;
>+        JSObject *global = JS_GetGlobalForCompartment(mCx, c);
>+        if (global) {
>+            nsISupports *native = nsXPConnect::GetXPConnect()->GetNativeOfWrapper(mCx, global);
>+            nsCOMPtr<nsPIDOMWindow> piwindow = do_QueryInterface(native, &rv);
>+            if (NS_SUCCEEDED(rv)) {

Just skip the optional rv param and check for piwindow directly.

>+                // The global is a |window| object!  Get its path.
>+                nsGlobalWindow *window = static_cast<nsGlobalWindow*>(piwindow.get());

I don't know what the path stuff here is for, but wouldn't we also want to handle sandboxes and JSMs?

Anyway, r=bholley on the wrapped global with that change, as well as the addition of XPCJSMemoryReporter. I've never looked at any of this code, so someone who has should probably anything else.
Comment 48 Nicholas Nethercote [:njn] 2012-07-03 17:15:19 PDT
Created attachment 638919 [details] [diff] [review]
(part 1) - Convert JSCompartment::gcRunning to gcState, v2

Bill: this version addresses your comments and makes additional name changes.  I'm asking for review on the names.   Details:

- JSRuntime::gcState            --> JSRuntime::heapState
- JSRuntime::GCState            --> JSRuntime::HeapState
- JSRuntime::{Idle,Iterator,GC} --> JSRuntime::{Idle,Tracing,Collecting}
- JSRuntime::gcHeapBusy()       --> JSRuntime::isHeapBusy()
- AssertIdle()                  --> AssertHeapIsIdle()
- AutoHeapSession               --> AutoTraceSession
Comment 49 Nicholas Nethercote [:njn] 2012-07-03 18:05:07 PDT
> >+namespace xpc {
> >+
> >+typedef nsDataHashtable<nsPtrHashKey<nsGlobalWindow>, nsCString> WindowPaths;
> 
> I'd rather this be keyed off the window's ID (nsPIDOMWindow::WindowID())
> than a raw pointer to the window.  That way, we don't have to worry about
> using stale pointers.

I'm happy to change to IDs to avoid the casts and the nsGlobalWindow.h import, that's a clear improvement.

But, in general, can't IDs become stale just the same as nsGlobalWindow pointers? 


> I'm not sold on the overall design here, though.  It seems that anything
> which wants to report per-tab memory usage is now going to have to be
> explicitly invoked by nsWindowMemoryReporter.  That's not so bad in and of
> itself, but then perhaps we should design this more generally: We have one
> multi-reporter at the top (what's currently nsWindowMemoryReporter), and you
> can register with it sub-multi-reporters which get the windowPaths argument.
> Then the DOM and layout memory reporting can be separate from the main
> reporter as well.
> 
> We could put off re-designing this until later, if you wanted.  What's in
> this pach works well enough; it's just not designed in a particularly
> obvious or flexible way.  What do you think?

I agree that the tying of nsWindowMemoryReporter to other reporters isn't great, but I don't feel that registering sub-multi-reporters is cleaner.  Would we need a new variant of nsIMemoryMultiReporter?  How/where would these be registered?
Comment 50 Nicholas Nethercote [:njn] 2012-07-03 18:07:54 PDT
> >+                // The global is a |window| object!  Get its path.
> >+                nsGlobalWindow *window = static_cast<nsGlobalWindow*>(piwindow.get());
> 
> I don't know what the path stuff here is for, but wouldn't we also want to
> handle sandboxes and JSMs?

I think JSMs are always system compartments, right?  They get reported under the generic "explicit/js/..." path, i.e. not reported under a particular tab.

As for sandboxes... is there a way to associate a sandbox with a particular nsGlobalWindow?  Are there any other kinds of compartments/globals that can also be associated with a nsGlobalWindow?
Comment 51 Nicholas Nethercote [:njn] 2012-07-03 18:37:29 PDT
> I think JSMs are always system compartments, right?  They get reported under
> the generic "explicit/js/..." path, i.e. not reported under a particular tab.
> 
> As for sandboxes... is there a way to associate a sandbox with a particular
> nsGlobalWindow?  Are there any other kinds of compartments/globals that can
> also be associated with a nsGlobalWindow?

khuey tells me that sandboxes are chrome-only, in which case I'm happy for them to go in the generic "explicit/js/..." sub-tree, just like JSMs.

khuey also tells me that, aside from web workers, user code cannot create a global that isn't also a |window| object.  So my patch should categorize all user globals correctly, which is what I want.  (Web workers don't get categorized under a tab, but they're rare enough that I'm happy to fix that up later.)
Comment 52 Nicholas Nethercote [:njn] 2012-07-03 18:58:16 PDT
Comment on attachment 637797 [details] [diff] [review]
(part 3) - Report JS memory consumption for compartments that are associated with |window| objects under "window-objects".

jlebar's a DOM peer and he looked at the worker changes (thanks!), so there's no need for bent to review unless he really wants to.
Comment 53 Justin Lebar (not reading bugmail) 2012-07-03 21:41:44 PDT
> But, in general, can't IDs become stale just the same as nsGlobalWindow pointers? 

Window IDs are never re-used, if that's what you're asking.

> Would we need a new variant of nsIMemoryMultiReporter?  How/where would these be 
> registered?

We definitely could do that.  It's more plumbing, but perhaps cleaner from an architectural PoV.

Unlike our existing memory reporters, you'd need to have a way to run these as a batch, since we really want the list and categorization of windows to be consistent across our DOM/layout/JS reporters.

Anyway, if you wanted to do it this way, you could register the SpecialMemoryReporter the same way we register other reporters.  The main difference is the API for /running/ them, I think.  We could start out that you run all of them, and then later add the capability to run a subset of them, if we ever wanted that.  If you didn't want to add a third class of reporter about:memory has to handle, this code could create a fake multi-reporter which runs all the SpecialMemoryReporters, so it's transparent to clients...
Comment 54 Johnny Stenback (:jst, jst@mozilla.com) 2012-07-03 21:48:16 PDT
(In reply to Nicholas Nethercote [:njn] from comment #49)
> But, in general, can't IDs become stale just the same as nsGlobalWindow
> pointers? 

Sure, but you'd need to call into nsGlobalWindow to get to a pointer to a window, and that code returns null for a stale ID, so less risk of dangling pointers. Not that I know that that's necessarily a big risk here in general... And IDs are 64 bit integers incrementally incremented on window creation, so it'd take a while for them to wrap around and start referring to another window.
Comment 55 Bill McCloskey (:billm) 2012-07-05 18:37:43 PDT
Comment on attachment 638919 [details] [diff] [review]
(part 1) - Convert JSCompartment::gcRunning to gcState, v2

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

::: js/src/jsapi.cpp
@@ +229,4 @@
>  }
>  
>  static void
> +AssertIdleOrFlatString(JSContext *cx, JSString *str)

Maybe AssertHeapIsIdleOrFlatString?
Comment 56 Nicholas Nethercote [:njn] 2012-07-05 18:43:29 PDT
> > +AssertIdleOrFlatString(JSContext *cx, JSString *str)
> 
> Maybe AssertHeapIsIdleOrFlatString?

I must have forgotten to update my patch before posting... I have AssertHeapIsIdleOrStringIsFlat() in my local version :)

Note You need to log in before you can comment on or make changes to this bug.