1123472 - Too many JS compartments at www.donegaldaily.com

Status: RESOLVED INCOMPLETE

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: Memory reports

Caspy7 emailed me this article: http://www.myce.com/news/firefox-has-a-serious-memory-leak-and-ie11-uses-least-ram-cpu-in-our-test-74323/. In short, Firefox's memory usage slowly spikes (and then recedes?) on http://www.donegaldaily.com/. Other browsers behave more nicely.

The author reports this on Windows and I can reproduce something similar on Linux; there are stupid numbers of JS compartments being created.

In a fairly recent trunk build I created a new profile, disabled e10s, and then navigated to the site and let the browser sit there undisturbed for maybe 10 minutes are so. I get this from about:memory:

> │  ├──299.42 MB (44.60%) -- top(http://www.donegaldaily.com/, id=8)
> │  │  ├──189.68 MB (28.25%) -- active
> │  │  │  ├──176.43 MB (26.28%) -- window(http://www.donegaldaily.com/)
> │  │  │  │  ├──122.13 MB (18.19%) -- js-compartment(http://www.donegaldaily.com/)
> │  │  │  │  │  ├──108.01 MB (16.09%) -- classes
> │  │  │  │  │  │  ├───47.66 MB (07.10%) -- class(Function)
> │  │  │  │  │  │  │   ├──42.80 MB (06.38%) -- objects
> │  │  │  │  │  │  │   │  ├──38.80 MB (05.78%) ── gc-heap [68]
> │  │  │  │  │  │  │   │  └───4.00 MB (00.60%) ── malloc-heap/slots [68]
> ...
> │  │  │  │  ├───24.80 MB (03.69%) -- layout
> │  │  │  │  │   ├──12.96 MB (01.93%) ── pres-shell [72]
> │  │  │  │  │   ├───8.20 MB (01.22%) ── style-sets [72]
> ...
> │  │  └──109.73 MB (16.34%) -- js-zone(0x7fc00bc13000)
> │  │     ├───35.39 MB (05.27%) ── type-pool
> │  │     ├───34.98 MB (05.21%) -- lazy-scripts
> │  │     │   ├──29.86 MB (04.45%) ── gc-heap
> │  │     │   └───5.12 MB (00.76%) ── malloc-heap
> │  │     ├───30.08 MB (04.48%) -- type-objects
> │  │     │   ├──29.66 MB (04.42%) ── gc-heap
> │  │     │   └───0.43 MB (00.06%) ── malloc-heap
> │  │     └────9.28 MB (01.38%) ++ (5 tiny)

The numbers like [68] and [72] indicate duplicate reports that are aggregated. So there are actually at least 68 compartments with the same number, and 72 windows. The sizes of the things in the zone are also unusually large.

Then there is also a detached window:

> │  ├──155.79 MB (23.21%) -- top(none)/detached
> │  │  ├──148.31 MB (22.09%) -- window([system])
> │  │  │  ├──128.77 MB (19.18%) -- js-compartment(http://www.donegaldaily.com/)
> │  │  │  │  ├──119.42 MB (17.79%) -- classes
> │  │  │  │  │  ├───51.72 MB (07.70%) -- class(Function)
> │  │  │  │  │  │   ├──45.42 MB (06.77%) -- objects
> │  │  │  │  │  │   │  ├──40.65 MB (06.05%) ── gc-heap [62]
> │  │  │  │  │  │   │  └───4.78 MB (00.71%) ── malloc-heap/slots [62]

Again, the [62] indicates many duplicates. And I don't know why these are showing up under |window([system])|... that's weird.

Finally, we have this, which is interesting.

> 0 ── ghost-windows

I've attached the full reports.

When I hit "minimize memory usage" the size of each windiw/compartment drops but the number of windows/compartments does not drop.

I suspect the site is doing something silly. But it's interesting that other browsers don't seem to have similarly bad behaviour, at least according to the original article.

Comment 1

[Nicholas Nethercote [inactive]]

And a while later the compartment/window count is up to 140 and I have this:

> └──-11.68 MB (-1.27%) ── heap-unclassified [?!]

Something very strange is going on.

Comment 2

[Jan de Mooij [:jandem]]

They have this in the global scope:

setInterval(function(){googletag.pubads().refresh([adslot2]);}, Math.floor(Math.random()*4001)+15000);
setInterval(function(){googletag.pubads().refresh([adslot3]);}, Math.floor(Math.random()*4001)+15000);
setInterval(function(){googletag.pubads().refresh([adslot4]);}, Math.floor(Math.random()*4001)+15000);
setInterval(function(){googletag.pubads().refresh([adslot5]);}, Math.floor(Math.random()*4001)+15000);
setInterval(function(){googletag.pubads().refresh([adslot6]);}, Math.floor(Math.random()*4001)+15000);
setInterval(function(){googletag.pubads().refresh([adslot8]);}, Math.floor(Math.random()*4001)+15000);
setInterval(function(){googletag.pubads().refresh([adslot9]);}, Math.floor(Math.random()*4001)+15000);

So the ads are refreshed every 15 seconds... This ends up calling http://partner.googleadservices.com/gpt/pubads_impl_56.js:119, which looks like this (beautified):

if (... Check for IE...) {
    ...
} else {
    ...
    try {
        g = a.contentWindow ? a.contentWindow.document : a.contentDocument,
        -1 != navigator.userAgent.indexOf("Firefox") && g.open("text/html", "replace"),
        g.write(h),
        g.close()
    } catch (ib) {}
}

The .open call ends up creating a new compartment (note that this call is Firefox-specific..)

Comment 3

[Nicholas Nethercote [inactive]]

Thanks for the diagnosis, Jan!

So there are two aspects here:

- Evangelism: we should contact the site to fix this.

- Memory reporting: I should work out what's causing the negative heap-unclassified measurements.

Comment 4

[Jan de Mooij [:jandem]]

But the number of compartments keeps growing right? I don't know what's keeping the old ones alive.

Comment 5

[Andrew McCreight [:mccr8]]

There is a detached window but no ghost window because a window is only a ghost window when there are no other active windows of that origin.  I think this can happen if an iframe or whatever is being held alive through JS but isn't being displayed.  I don't know why it is showing [system] though.

Comment 6

[Nicholas Nethercote [inactive]]

> I don't know why it is showing [system] though.

https://dxr.mozilla.org/mozilla-central/source/dom/base/nsWindowMemoryReporter.cpp#185 explains it -- if GetWindowURI() returns null then the reporter uses "[system]" instead. Perhaps that should be changed to "???" or something like that.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: DMD output showing twice-reported stuff

I did a DMD run. The total amount of twice-reported memory was only 2.2 MB, which isn't enough to cause the negative "heap-unclassified" value of -64.47 MiB, but the number of twice-reported blocks (409) was still much higher than I've seen before. And those twice-reported blocks are from all over: NSS, CSS stuff, layout stuff, PresArena stuff. It feels like some major data structures are getting reported twice.

Comment 8

[Nicholas Nethercote [inactive]]

More broken measurements -- note that "window-objects" exceeds "explicit", which explains the "[?!]" annotation:

> 2,819.64 MB (100.0%) -- explicit
> ├──2,868.24 MB (101.72%) -- window-objects [?!]
> │  ├──2,807.27 MB (99.56%) -- top(http://www.donegaldaily.com/, id=8)
> │  │  ├──2,162.98 MB (76.71%) -- active
> │  │  │  ├──2,146.09 MB (76.11%) -- window(http://www.donegaldaily.com/)
> │  │  │  │  ├──1,575.61 MB (55.88%) -- js-compartment(http://www.donegaldaily.com/)

Comment 9

[Boris Zbarsky [:bzbarsky]]

> It feels like some major data structures are getting reported twice.

In the document.open() case, multiple inner windows share a single document.  Could that do it?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

(and remember also the case when the same inner window is shared with multiple documents)

Comment 11

[Nicholas Nethercote [inactive]]

(In reply to Boris Zbarsky [:bz] from comment #9)
> 
> In the document.open() case, multiple inner windows share a single document.
> Could that do it?

Yes! The DOM memory reporter assumes that a docshell owned by a global window is uniquely owned by that window: 
https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#13268

I did some diagnostic printfs and confirmed that docshells are being shared between global windows. So I need to modify the reporter so it handles the case when N global windows share 1 docshell.

I don't understand how that code causes this to happen. Can you explain? Thank you. It'd be good to have a small test case that reproduces this scenario.

(In reply to Olli Pettay [:smaug] from comment #10)
> (and remember also the case when the same inner window is shared with
> multiple documents)

You mean that 1 global window is shared between N docshells? How does that work? The memory reporter certainly won't handle that... I think the only way a docshell can get measured is if it is pointed to by nsGlobalWindow::mDoc.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> I don't understand how that code causes this to happen. Can you explain?

Sure.  Consider the following web page:

  <script>
    onload = function() {
      document.open();
      document.close();
    }
  </script>

In this case, when the load event fires, document.open() has to create a new Window, per spec, but it gets associated with the existing Document (whose object identity can't so much change due to calling a method on it).

The simplest thing to do is probably to just change the condition in the code you linked to

  if (mDoc && mDoc->GetInnerWindow() == this)

> You mean that 1 global window is shared between N docshells?

Not docshells, documents. Two documents, to be precise.

Here's a simple example:

  <iframe></iframe>
  <script>
    var oldDoc = frames[0].document;
    frames[0].location = someSameOriginURI;
    document.querySelector("iframe").onload = function() {
      // frames[0].document != oldDoc here, but they share the same inner window in the
      // special case here, where oldDoc was the initial about:blank in that window and
      // the thing that got loaded is same-origin with that initial about:blank.
    }
  </script>

Comment 13

[Nicholas Nethercote [inactive]]

> The simplest thing to do is probably to just change the condition in the
> code you linked to
> 
>   if (mDoc && mDoc->GetInnerWindow() == this)

I tried this on donegaldaily.com but I got some instances of non-shared documents for which the second part of the condition failed. So this would cause us to fail to measure some documents. I think I'll need the hashtable after all.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> but I got some instances of non-shared documents for which the second part of the
> condition failed.

Oh?  Was mDoc->GetInnerWindow() null or some other value?

Comment 15

[Nicholas Nethercote [inactive]]

(In reply to Boris Zbarsky [:bz] from comment #14)
> > but I got some instances of non-shared documents for which the second part of the
> > condition failed.
> 
> Oh?  Was mDoc->GetInnerWindow() null or some other value?

In all cases it was null. So this condition might work...

> if (mDoc && (mDoc->GetInnerWindow() == this || !mDoc->GetInnerWindow))

Comment 16

[Nicholas Nethercote [inactive]]

> In all cases it was null. So this condition might work...
> 
> > if (mDoc && (mDoc->GetInnerWindow() == this || !mDoc->GetInnerWindow))

With that condition applied DMD tells me I get no double counting. I've spun off bug 1124476 for that.

Comment 17

[Nicholas Nethercote [inactive]]

> With that condition applied DMD tells me I get no double counting. I've spun
> off bug 1124476 for that.

I just checked a long-running session. I'm up to 882 donegaldaily.com compartments and heap-unclassified is 2.64%. Which is rather low, but still positive.

So that's good. But the site still drags Firefox to a halt after a few hours. I don't understand the site code well enough to understand if this is expected -- in which case we need to evangelize -- or if there's something sub-optimal about Firefox.

Comment 18

[Mike Hommey [:glandium]]

How about testing with another browser with a Firefox UA (since the script does a check on the UA) and see what happens?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Note that the other browser would probably need to be IE.  Safari and Chrome don't follow the spec and don't create a new Window on document.open().

Comment 20

[Nicholas Nethercote [inactive]]

(In reply to Boris Zbarsky [:bz] from comment #19)
> Note that the other browser would probably need to be IE.  Safari and Chrome
> don't follow the spec and don't create a new Window on document.open().

Are you talking about step 12 in dev.w3.org/html5/spec-LC/apis-in-html-documents.html#dom-document-open ?

Why do the replaced |window| objects continue to hang around?

Comment 21

[Boris Zbarsky [:bzbarsky]]

> Are you talking about step 12

Yes.

> Why do the replaced |window| objects continue to hang around?

The most likely reason is that someone is holding on to some object from that global somewhere in the page's script.  If not, then it could be due to bfcache (though that should only hold a few of them), or a bug of some sort in Gecko...