826521 - XHR-related dark matter string

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Nicholas Nethercote [inactive]]

I did an 8 hour DMD run of my normal daily browsing.  The top unreported stack
trace record was this:

> Unreported: 1 block in stack trace record 1 of 4,082
>  2,101,248 bytes (2,097,226 requested / 4,022 slop)
>  0.70% of the heap (0.70% cumulative);  3.92% of unreported (3.92% cumulative)
>  Allocated at
>    nsStringBuffer::Realloc(nsStringBuffer*, unsigned long) (/home/njn/moz/mi0/xpcom/string/src/nsSubstring.cpp:207) 0x7f0e2272f12d
>    nsAString_internal::MutatePrep(unsigned int, unsigned short**, unsigned int*) (/home/njn/moz/mi0/xpcom/string/src/nsTSubstring.cpp:103) 0x7f0e2272f366
>    nsAString_internal::SetCapacity(unsigned int, mozilla::fallible_t const&) (/home/njn/moz/mi0/xpcom/string/src/nsTSubstring.cpp:553) 0x7f0e2272fd36
>    nsXMLHttpRequest::AppendToResponseText(char const*, unsigned int) (/home/njn/moz/mi0/content/base/src/nsXMLHttpRequest.cpp:776) 0x7f0e21c3f543
>    nsXMLHttpRequest::StreamReaderFunc(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*) (/home/njn/moz/mi0/content/base/src/nsXMLHttpRequest.cpp:1901) 0x7f0e21c420c8
>    nsPipeInputStream::ReadSegments(tag_nsresult (*)(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*), void*, unsigned int, unsigned int*) (/home/njn/moz/mi0/xpcom/io/nsPipe3.cpp:775) 0x7f0e226fd456
>    nsXMLHttpRequest::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) (/home/njn/moz/mi0/content/base/src/nsXMLHttpRequest.cpp:1980) 0x7f0e21c4242a
>    nsBaseChannel::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) (/home/njn/moz/mi0/netwerk/base/src/nsBaseChannel.cpp:764) 0x7f0e217a4dc5
>    nsInputStreamPump::OnStateTransfer() (/home/njn/moz/mi0/netwerk/base/src/nsInputStreamPump.cpp:482) 0x7f0e217ae9f4
>    nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (/home/njn/moz/mi0/netwerk/base/src/nsInputStreamPump.cpp:371) 0x7f0e217ae738
>    nsInputStreamReadyEvent::Run() (/home/njn/moz/mi0/xpcom/io/nsStreamUtils.cpp:83) 0x7f0e226fec70
>    nsThread::ProcessNextEvent(bool, bool*) (/home/njn/moz/mi0/xpcom/threads/nsThread.cpp:627) 0x7f0e227123b4
>    NS_ProcessNextEvent_P(nsIThread*, bool) (/home/njn/moz/mi0/dmdo64/xpcom/build/nsThreadUtils.cpp:237) 0x7f0e226d8ac9
>    mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/njn/moz/mi0/ipc/glue/MessagePump.cpp:82) 0x7f0e2252833c
>    MessageLoop::Run() (/home/njn/moz/mi0/ipc/chromium/src/base/message_loop.cc:183) 0x7f0e22741d71
>    nsBaseAppShell::Run() (/home/njn/moz/mi0/widget/xpwidgets/nsBaseAppShell.cpp:165) 0x7f0e2244e2eb
>    nsAppStartup::Run() (/home/njn/moz/mi0/toolkit/components/startup/nsAppStartup.cpp:288) 0x7f0e222daf38
>    XREMain::XRE_mainRun() (/home/njn/moz/mi0/toolkit/xre/nsAppRunner.cpp:3823) 0x7f0e2177c3cb
>    XREMain::XRE_main(int, char**, nsXREAppData const*) (/home/njn/moz/mi0/toolkit/xre/nsAppRunner.cpp:3890) 0x7f0e2177ca40
>    XRE_main (/home/njn/moz/mi0/toolkit/xre/nsAppRunner.cpp:4088) 0x7f0e2177ce81
>    do_main(int, char**) (/home/njn/moz/mi0/browser/app/nsBrowserApp.cpp:174) 0x402f95
>    main (/home/njn/moz/mi0/browser/app/nsBrowserApp.cpp:279) 0x402b56
>    __libc_start_main (/build/buildd/eglibc-2.15/csu/libc-start.c:258) 0x7f0e247b176d
>    _start (??:?) 0x402929

Comment 1

[Nicholas Nethercote [inactive]]

This allocation ends up in nsXMLHttpRequest::mResponseText.  nsXMLHttpRequest doesn't have any SizeOf methods, interestingly.  I don't know where the nsXMLHttpRequest will be stored, though.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

nsXMLHttpRequest will be owned by the JSObject wrapper, in general.

Comment 3

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

We could add SizeOf to all the nsDOMEventTargetHelpers and enumerate nsGlobalWindow.mEventTargetObjects
That should catch most of XHRs.

Comment 4

[Andrew McCreight [:mccr8]]

The orphan finder could be modified to look for XHR.  QI to XHR in addition to node, or whatever, and call a sizeof method if it works.

Comment 5

[Nicholas Nethercote [inactive]]

Notes from talking to bz on IRC:

XHRs are WebIDL objects, which means the nsISupports is not stored in the private slot but in slot 0.  Such objects have JSCLASS_IS_DOMJSCLASS set instead of JSCLASS_HAS_PRIVATE and JSCLASS_PRIVATE_IS_NSISUPPORTS.

I can extract an XHR from a JSObject like this:

  nsXMLHttpRequest* xhr;
  nsresult rv = mozilla::dom::UnwrapObject<nsXMLHttpRequest>(cx, obj, xhr);
  if (NS_SUCCEEDED(rv)) {
    ...
  }

|cx| can be NULL because we know this is a DOMJSCLASS, and therefore not a cross-compartment wrapper, and therefore no security check is needed.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

NB: There are also nsXMLHttpRequests only used from C++, such as the underlying object for worker thread XHR.

Comment 7

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

nsGlobalWindow.mEventTargetObjects should keep pointers also to worker XHRs, at least until the 
window is destroyed.

Comment 8

[Nicholas Nethercote [inactive]]

To summarize:

1. The nsGlobalWindow.mEventTargetObjects approach will get worker XHRs, but will miss orphaned XHRs.

2. The JSObject/UnwrapObject will miss worker XHRs, but will get orphaned XHRs.

Hmm.  I might just do (1), and then (2) later if DMD indicates orphaned XHRs are significant.

Comment 9

[Nicholas Nethercote [inactive]]

> We could add SizeOf to all the nsDOMEventTargetHelpers and enumerate
> nsGlobalWindow.mEventTargetObjects
> That should catch most of XHRs.

So... nsGlobalWindow::mEventTargetObjects looks like this:

  nsTHashtable<nsPtrHashKey<nsDOMEventTargetHelper> > mEventTargetObjects;

How are XHRs stored here?  I couldn't work that out even after looking at nsDOMEventTargetHelper and the classes it inherits from, such as mozilla::dom::EventTarget, and nsIDomEventTarget.

Comment 10

[Nicholas Nethercote [inactive]]

> How are XHRs stored here?

Ehsan helped me with this.  nsXMLHttpRequest is a sub-class of nsXHREventTarget, which is a sub-class of nsDOMEventTargetHelper.

Comment 11

[Nicholas Nethercote [inactive]]

I have a draft patch but I've hit a problem.  nsXHR::mResponseText is the one that takes up all the space, but it's a shared string, and currently we don't have a way to measure shared strings.

Is anyone (bz?) able to tell me what other structure might be sharing that string?  Depending on the answer, it might be worth adding a SizeOfExcludingThisEvenIfShared() function that is suitable for use if you know that other references won't measure the string.

Alternatively, we could divide the size by the refcount and teach DMD about fractional blocks (whatever that means).

Comment 12

[Justin Lebar (not reading bugmail)]

FWIW we saw yesterday the calendar app having many mb of heap-unclassified due to this.

Comment 13

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

By default it is not shared with anything, but if someone does something like
var text = XHR.responseText; it gets shared with JS. I don't know whether we report external
strings as JS mem usage.

I think you could check the refcnt of the string buffer. If it is 1 (== !ReadOnly), the string 
certainly isn't shared with anything. So, something like 
nsStringBuffer* b = nsStringBuffer::FromString(mResponseText);
if (b && !b->ReadOnly()) {
  // report memory usage...
}

But yes, in general perhaps memory reporter should understand shared string buffers and
for each case the string buffer is used size/refcnt.

Comment 14

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

Oh, we have SizeOfIncludingThisIfUnshared in nsStringBuffer :)

Comment 15

[Nicholas Nethercote [inactive]]

(In reply to Olli Pettay [:smaug] from comment #13)
> By default it is not shared with anything, but if someone does something like
> var text = XHR.responseText; it gets shared with JS. I don't know whether we
> report external
> strings as JS mem usage.

External string chars aren't counted by the JS reporter:  http://mxr.mozilla.org/mozilla-central/source/js/src/vm/String.cpp#61.

Is there any way to tell that the XHR is sharing the StringBuffer with a JS external string?

> But yes, in general perhaps memory reporter should understand shared string
> buffers and for each case the string buffer is used size/refcnt.

Will that miss some of the memory when the sharing is with a JS external string?  E.g. from the XHR reporter we'll report N/2 bytes, and from the JS reporter we'll report 0 bytes.  I guess if we knew that the JS external string's chars were in a StringBuffer we could check the refcount, but is it possible to know that from the JS engine side?

The other tricky part of the N/refcnt approach is DMD.  It would need to support the notion of a partially reported block.  It's probably doable, but it certainly complicates things.

Comment 16

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Add SizeOfIncludingThisEvenIfShared() for strings.

This patch adds SizeOf{In,Ex}cludingThisEvenIfShared to multiple string
classes.

Comment 17

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Report memory used by event targets, especially XHRs.

This patch adds measurement for the event targets object table, and XHRs, under
"dom/event-targets".  In Gmail I've seen this account for 6 MiB of memory.

The patch also does some trivial clean-ups:

- Moves the "dom/other" reports last, to match the struct field order.

- Simplifies the descriptions for the window sum reports, to avoid some
  repetition.

- Removes some trailing whitespace.

- Fixes some mode lines.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 700815 [details] [diff] [review]
(part 1) - Add SizeOfIncludingThisEvenIfShared() for strings.

r=me

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 700816 [details] [diff] [review]
(part 2) - Report memory used by event targets, especially XHRs.

Worth documenting that these objects do NOT include DOM nodes....  Are we sure none of the other ones are already being reported, by the way?  r=me if we've checked the subclasses of nsDOMEventTargetHelper to make sure they're not already reported.

Comment 20

[Nicholas Nethercote [inactive]]

(In reply to Boris Zbarsky (:bz) from comment #19)
> Comment on attachment 700816 [details] [diff] [review]
> (part 2) - Report memory used by event targets, especially XHRs.
> 
> Worth documenting that these objects do NOT include DOM nodes....  Are we
> sure none of the other ones are already being reported, by the way?  r=me if
> we've checked the subclasses of nsDOMEventTargetHelper to make sure they're
> not already reported.

They'd have to implement nsISizeOf, and the only classes other than nsXHR to do that currently are:  nsSimpleURI, nsStandardURL and nsNullPrincipalURI.

And I checked the subclasses of nsDOMEventTargetHelper, and didn't recognize any other them.

But neither of those things is a terribly strong protection against double-counting in the long term.  Hmm.

Comment 21

[Nicholas Nethercote [inactive]]

So this patch relies on nsXHR implementing nsISizeOf.  The danger is if another subclass of nsDOMEventTargetHelper implements nsISizeOf, we will start counting it, which may not be what we want.

What about creating an interface nsISizeOfXHR that has the same methods as nsISizeOf?  nsXHR would implement it, nothing else would, and we'd be safe.  To generalize it slightly could instead name it nsISizeOfGlobalWindowEventTargetObjects, and potentially reuse it for other subclasses of nsDOMEventTargetHelper in the future.

Is that too grotty for words?

Comment 22

[Boris Zbarsky [:bzbarsky]]

nsISizeOfEventTarget would be fine, I think.

Comment 23

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Report memory used by event targets, especially XHRs.

New version that uses nsISizeOfEventTarget.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 703752 [details] [diff] [review]
(part 2) - Report memory used by event targets, especially XHRs.

r=me

Comment 25

[Nicholas Nethercote [inactive]]

4 minutes 31 seconds for a review?  O_o

Comment 26

[Boris Zbarsky [:bzbarsky]]

Well, I compared to the previous patch, which I'd already reviewed... ;)

Comment 27

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b924819d71a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/de2ab911692d

Comment 28

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/b924819d71a5
https://hg.mozilla.org/mozilla-central/rev/de2ab911692d