Closed Bug 922094 Opened 6 years ago Closed 6 years ago

memory allocated by AllocateProtoAndIfaceCache goes unreported

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox28 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(2 files, 3 obsolete files)

Seen in a fresh session:

Unreported: 344 blocks in stack trace record 4 of 5,590
 2,818,048 bytes (2,785,024 requested / 33,024 slop)
 0.63% of the heap (8.40% cumulative);  2.11% of unreported (27.93% cumulative)
 Allocated at
   replace_malloc (/home/froydnj/src/mozilla-central-official/memory/replace/dmd/DMD.cpp:1227) 0x7fdcb0f20f44
   moz_xmalloc (/home/froydnj/src/mozilla-central-official/memory/mozalloc/mozalloc.cpp:55) 0x7fdcb0f12121
   mozilla::dom::AllocateProtoAndIfaceCache(JSObject*) (/opt/build/froydnj/build-mc/js/xpconnect/src/../../../dist/include/mozilla/dom/BindingUtils.h:267) 0x7fdcad5a595f
   xpc::CreateGlobalObject(JSContext*, JSClass const*, nsIPrincipal*, JS::CompartmentOptions&) (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/nsXPConnect.cpp:475) 0x7fdcad5a4e02
   XPCWrappedNative::WrapNewGlobal(xpcObjectHelper&, nsIPrincipal*, bool, JS::CompartmentOptions&, XPCWrappedNative**) (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/XPCWrappedNative.cpp:313) 0x7fdcad593c7a
   nsXPConnect::InitClassesWithNewWrappedGlobal(JSContext*, nsISupports*, nsIPrincipal*, unsigned int, JS::CompartmentOptions&, nsIXPConnectJSObjectHolder**) (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/nsXPConnect.cpp:505) 0x7fdcad5a502f
   nsFrameScriptExecutor::InitTabChildGlobalInternal(nsISupports*, nsACString_internal const&) (/home/froydnj/src/mozilla-central-official/content/base/src/nsFrameMessageManager.cpp:1151) 0x7fdcad144a7a
   nsInProcessTabChildGlobal::InitTabChildGlobal() (/home/froydnj/src/mozilla-central-official/content/base/src/nsInProcessTabChildGlobal.cpp:323) 0x7fdcad155c5c
   operator new (/opt/build/froydnj/build-mc/content/base/src/../../../dist/include/mozilla/mozalloc.h:201) 0x7fdcad155c85
   nsInProcessTabChildGlobal::LoadFrameScript(nsAString_internal const&) (/home/froydnj/src/mozilla-central-official/content/base/src/nsInProcessTabChildGlobal.cpp:353) 0x7fdcad1561c3
   nsAsyncScriptLoad::Run() (/home/froydnj/src/mozilla-central-official/content/base/src/nsInProcessTabChildGlobal.cpp:337) 0x7fdcad15657d
   nsCOMPtr<nsIRunnable>::operator=(nsIRunnable*) (/opt/build/froydnj/build-mc/content/base/src/../../../dist/include/nsCOMPtr.h:656) 0x7fdcad109d97
   nsDocument::EndUpdate(unsigned int) (/home/froydnj/src/mozilla-central-official/content/base/src/nsDocument.cpp:4427) 0x7fdcad1328c1
   mozilla::dom::XULDocument::EndUpdate(unsigned int) (/home/froydnj/src/mozilla-central-official/content/xul/document/src/XULDocument.cpp:3361) 0x7fdcad2df797
   mozAutoDocUpdate::~mozAutoDocUpdate() (/home/froydnj/src/mozilla-central-official/content/base/src/mozAutoDocUpdate.h:38) 0x7fdcad04956c
   nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*, mozilla::ErrorResult&) (/home/froydnj/src/mozilla-central-official/content/base/src/nsINode.cpp:2071) 0x7fdcad152803
   appendChild (/opt/build/froydnj/build-mc/dom/bindings/NodeBinding.cpp:525) 0x7fdcad9e6217
   genericMethod (/opt/build/froydnj/build-mc/dom/bindings/NodeBinding.cpp:1232) 0x7fdcad9e557a
   CallJSNative (/home/froydnj/src/mozilla-central-official/js/src/jscntxtinlines.h:218) 0x7fdcae140653
   Interpret (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:2488) 0x7fdcae135e8d
   RunScript (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:419) 0x7fdcae1402c6
   RunScript (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:388) 0x7fdcae140587
   js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:512) 0x7fdcae1425cd
   js::DirectProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) (/home/froydnj/src/mozilla-central-official/js/src/jsproxy.cpp:451) 0x7fdcae2ada18

Unreported: 335 blocks in stack trace record 5 of 5,590
 2,744,320 bytes (2,712,160 requested / 32,160 slop)
 0.62% of the heap (9.01% cumulative);  2.05% of unreported (29.98% cumulative)
 Allocated at
   replace_malloc (/home/froydnj/src/mozilla-central-official/memory/replace/dmd/DMD.cpp:1227) 0x7fdcb0f20f44
   moz_xmalloc (/home/froydnj/src/mozilla-central-official/memory/mozalloc/mozalloc.cpp:55) 0x7fdcb0f12121
   mozilla::dom::AllocateProtoAndIfaceCache(JSObject*) (/opt/build/froydnj/build-mc/js/xpconnect/src/../../../dist/include/mozilla/dom/BindingUtils.h:267) 0x7fdcad5a595f
   xpc::CreateGlobalObject(JSContext*, JSClass const*, nsIPrincipal*, JS::CompartmentOptions&) (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/nsXPConnect.cpp:475) 0x7fdcad5a4e02
   XPCWrappedNative::WrapNewGlobal(xpcObjectHelper&, nsIPrincipal*, bool, JS::CompartmentOptions&, XPCWrappedNative**) (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/XPCWrappedNative.cpp:313) 0x7fdcad593c7a
   nsXPConnect::InitClassesWithNewWrappedGlobal(JSContext*, nsISupports*, nsIPrincipal*, unsigned int, JS::CompartmentOptions&, nsIXPConnectJSObjectHolder**) (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/nsXPConnect.cpp:505) 0x7fdcad5a502f
   CreateNativeGlobalForInner (/home/froydnj/src/mozilla-central-official/dom/base/nsGlobalWindow.cpp:2117) 0x7fdcad325dea
   nsDocumentViewer::InitInternal(nsIWidget*, nsISupports*, nsIntRect const&, bool, bool, bool) (/home/froydnj/src/mozilla-central-official/layout/base/nsDocumentViewer.cpp:898) 0x7fdcacf77537
   nsDocumentViewer::Init(nsIWidget*, nsIntRect const&) (/home/froydnj/src/mozilla-central-official/layout/base/nsDocumentViewer.cpp:644) 0x7fdcacf77889
   nsDocShell::SetupNewViewer(nsIContentViewer*) (/home/froydnj/src/mozilla-central-official/docshell/base/nsDocShell.cpp:8375) 0x7fdcadc29f2c
   nsCOMPtr<nsIURI>::get() const (/opt/build/froydnj/build-mc/docshell/base/../../dist/include/nsCOMPtr.h:800) 0x7fdcadc32296
   nsCOMPtr<nsIURI>::get() const (/opt/build/froydnj/build-mc/docshell/base/../../dist/include/nsCOMPtr.h:800) 0x7fdcadc2f438
   nsDocShell::EnsureContentViewer() (/home/froydnj/src/mozilla-central-official/docshell/base/nsDocShell.cpp:7046) 0x7fdcadc2f787
   nsDocShell::GetInterface(nsID const&, void**) (/home/froydnj/src/mozilla-central-official/docshell/base/nsDocShell.cpp:937) 0x7fdcadc2b338
   nsGetInterface::operator()(nsID const&, void**) const (/home/froydnj/src/mozilla-central-official/xpcom/glue/nsIInterfaceRequestorUtils.cpp:19) 0x7fdcadaf7be2
   nsCOMPtr_base::assign_from_helper(nsCOMPtr_helper const&, nsID const&) (/home/froydnj/src/mozilla-central-official/xpcom/glue/nsCOMPtr.cpp:110) 0x7fdcadaf5f08
   nsPIDOMWindow::MaybeCreateDoc() (/home/froydnj/src/mozilla-central-official/dom/base/nsGlobalWindow.cpp:3220) 0x7fdcad30f5f4
   nsPIDOMWindow::GetDoc() (/home/froydnj/src/mozilla-central-official/dom/base/nsPIDOMWindow.h:189) 0x7fdcad3038a8
   nsGlobalWindow::WrapObject(JSContext*, JS::Handle<JSObject*>) (/home/froydnj/src/mozilla-central-official/dom/base/nsGlobalWindow.h:326) 0x7fdcad327a0f
   XPCConvert::NativeInterface2JSObject(JS::Value*, nsIXPConnectJSObjectHolder**, xpcObjectHelper&, nsID const*, XPCNativeInterface**, bool, tag_nsresult*) (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/XPCConvert.cpp:840) 0x7fdcad57ba0e
   XPCConvert::NativeData2JS(JS::Value*, void const*, nsXPTType const&, nsID const*, tag_nsresult*) (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/XPCConvert.cpp:330) 0x7fdcad57c27d
   CallMethodHelper::GatherAndConvertResults() (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/XPCWrappedNative.cpp:2344) 0x7fdcad59615a
   XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1307) 0x7fdcad599dc9
   CallJSNative (/home/froydnj/src/mozilla-central-official/js/src/jscntxtinlines.h:218) 0x7fdcae140653
Additionally, a little further down in the log:


Unreported: 97 blocks in stack trace record 14 of 5,590
 794,624 bytes (785,312 requested / 9,312 slop)
 0.18% of the heap (12.61% cumulative);  0.59% of unreported (41.95% cumulative)
 Allocated at
   replace_malloc (/home/froydnj/src/mozilla-central-official/memory/replace/dmd/DMD.cpp:1227) 0x7fdcb0f20f44
   moz_xmalloc (/home/froydnj/src/mozilla-central-official/memory/mozalloc/mozalloc.cpp:55) 0x7fdcb0f12121
   mozilla::dom::AllocateProtoAndIfaceCache(JSObject*) (/opt/build/froydnj/build-mc/js/xpconnect/src/../../../dist/include/mozilla/dom/BindingUtils.h:267) 0x7fdcad5a595f
   xpc::CreateGlobalObject(JSContext*, JSClass const*, nsIPrincipal*, JS::CompartmentOptions&) (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/nsXPConnect.cpp:475) 0x7fdcad5a4e02
   xpc::CreateSandboxObject(JSContext*, JS::Value*, nsISupports*, xpc::SandboxOptions&) (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/Sandbox.cpp:969) 0x7fdcad5a71c7
   nsXPCComponents_utils_Sandbox::CallOrConstruct(nsIXPConnectWrappedNative*, JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&, bool*) (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/Sandbox.cpp:1449) 0x7fdcad5a8b59
   nsXPCComponents_utils_Sandbox::Call(nsIXPConnectWrappedNative*, JSContext*, JSObject*, JS::CallArgs const&, bool*) (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/Sandbox.cpp:1080) 0x7fdcad5a8c4c
   XPC_WN_Helper_Call (/home/froydnj/src/mozilla-central-official/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:871) 0x7fdcad598ec1
   CallJSNative (/home/froydnj/src/mozilla-central-official/js/src/jscntxtinlines.h:218) 0x7fdcae140762
   Interpret (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:2488) 0x7fdcae135e8d
   RunScript (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:419) 0x7fdcae1402c6
   RunScript (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:388) 0x7fdcae140587
   js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:512) 0x7fdcae1425cd
   js::DirectProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) (/home/froydnj/src/mozilla-central-official/js/src/jsproxy.cpp:451) 0x7fdcae2ada18
   js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) (/home/froydnj/src/mozilla-central-official/js/src/jswrapper.cpp:454) 0x7fdcae2fa26f
   js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) (/home/froydnj/src/mozilla-central-official/js/src/jsproxy.cpp:2616) 0x7fdcae2b4a19
   proxy_Call (/home/froydnj/src/mozilla-central-official/js/src/jsproxy.cpp:3135) 0x7fdcae2b4aff
   CallJSNative (/home/froydnj/src/mozilla-central-official/js/src/jscntxtinlines.h:218) 0x7fdcae140762
   Interpret (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:2488) 0x7fdcae135e8d
   RunScript (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:419) 0x7fdcae1402c6
   RunScript (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:388) 0x7fdcae1419ab
   JS_ExecuteScript(JSContext*, JSObject*, JSScript*, JS::Value*) (/home/froydnj/src/mozilla-central-official/js/src/jsapi.cpp:4858) 0x7fdcae1faa5c
   mozJSSubScriptLoader::LoadSubScript(nsAString_internal const&, JS::Value const&, nsAString_internal const&, JSContext*, JS::Value*) (/home/froydnj/src/mozilla-central-official/js/xpconnect/loader/mozJSSubScriptLoader.cpp:320) 0x7fdcad5ad321
   NS_InvokeByIndex (/home/froydnj/src/mozilla-central-official/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:164) 0x7fdcadb38b23
It looks like this is a data structure that hangs off globals in a reserved slot.
This is just the nsWindowMemoryReporter bits; the actual measurements come later.
Attachment #831794 - Flags: review?(n.nethercote)
This patch is the actual measurement.

I called FastGetGlobalJSObject twice because I wasn't sure if putting it in a
(explicit) temporary variable was some sort of rooting hazard or not...I see
that nsOuterWindowProxy::GetSubframeWindow uses a raw JSObject*, but I don't
think you're supposed to do that...
Attachment #831795 - Flags: review?(bzbarsky)
Comment on attachment 831795 [details] [diff] [review]
part 2 - make nsGlobalWindow report its proto/iface cache size, if appropriate

It's not a rooting hazard unless GC can happen between the two uses.  which in this case it can't, so you can just call it once.

r=me
Attachment #831795 - Flags: review?(bzbarsky) → review+
Comment on attachment 831795 [details] [diff] [review]
part 2 - make nsGlobalWindow report its proto/iface cache size, if appropriate

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

::: dom/base/nsGlobalWindow.cpp
@@ +12661,5 @@
> +
> +  if (IsInnerWindow() && FastGetGlobalJSObject()) {
> +    JS::Heap<JSObject*>* cache = GetProtoAndIfaceArray(FastGetGlobalJSObject());
> +    aWindowSizes->mProtoIfaceCacheSize += aWindowSizes->mMallocSizeOf(cache);
> +  }

So this cache is just a big array of JSObject pointers?
Comment on attachment 831794 [details] [diff] [review]
part 1 - report proto/iface cache size to about:memory

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

I'd merge this with part 2 before landing;  splitting it will just result in a revision that reports 0 bytes for this measurement, which isn't useful.

::: dom/base/nsWindowMemoryReporter.cpp
@@ +333,5 @@
>    aWindowTotalSizes->mDOMOtherSize += windowSizes.mDOMOtherSize;
>  
> +  REPORT_SIZE("/proto-iface-cache", windowSizes.mProtoIfaceCacheSize,
> +              "Memory used for prototype and interface binding caches "
> +              "with a window.");

I'm having trouble parsing this sentence.  Did you mean "within a window"?
Attachment #831794 - Flags: review?(n.nethercote) → review+
> So this cache is just a big array of JSObject pointers?

Yes.
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #10)
> Backed out for Windows debug mochitest-4 crashes.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/987504ee4b4c
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=30775696&tree=Mozilla-
> Inbound

I guess we can get a handle on the proto and iface cache after it's been deleted?  Probably should null out that cache and null-check during AddSizeOfIncludingThis.
We need this (and a subsequent check during memory reporting) to make
sure that things haven't changed out from underneath us.
Attachment #8335254 - Flags: review?(bzbarsky)
Now with additional check that we have the array.
Attachment #831795 - Attachment is obsolete: true
Attachment #8335255 - Flags: review?(bzbarsky)
Comment on attachment 8335254 [details] [diff] [review]
part 0 - mark the DOM_PROTOTYPE_SLOT as undefined after releasing the proto/iface cache

Per discussion, this patch is bogus.
Attachment #8335254 - Flags: review?(bzbarsky)
Comment on attachment 8335255 [details] [diff] [review]
part 2 - make nsGlobalWindow report its proto/iface cache size, if appropriate

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

::: dom/base/nsGlobalWindow.cpp
@@ +12664,5 @@
> +
> +  JSObject* global = FastGetGlobalJSObject();
> +  if (IsInnerWindow() && global && HasProtoAndIfaceArray(global)) {
> +    ProtoAndIfaceArray* cache = GetProtoAndIfaceArray(global);
> +    aWindowSizes->mProtoIfaceCacheSize += aWindowSizes->mMallocSizeOf(cache);

Now that bug 940573 has added a ProtoAndIFace array class, it would be better to give it a SizeOfIncludingThis() method and call that here instead of the raw |mMallocSizeof(cache)|.
Comment on attachment 8335255 [details] [diff] [review]
part 2 - make nsGlobalWindow report its proto/iface cache size, if appropriate

> HasProtoAndIfaceArray(global)

How is this better than just null-checking the result of GetProtoAndIfaceArray(global)?  And, again, when is this actually false?
(In reply to Boris Zbarsky [:bz] from comment #16)
> Comment on attachment 8335255 [details] [diff] [review]
> part 2 - make nsGlobalWindow report its proto/iface cache size, if
> appropriate
> 
> > HasProtoAndIfaceArray(global)
> 
> How is this better than just null-checking the result of
> GetProtoAndIfaceArray(global)?

Because DOM_PROTOTYPE_SLOT is an undefined JS::Value when we haven't set it.  And if I'm reading correctly, trying toPrivate on an undefined JS::Value will assert and break debug builds.

> And, again, when is this actually false?

That I do not know the answer to.  I'm doing a try run with the HasProtoAndIfaceArray check, but not clearing out the private value when we've deleted the proto/iface array.  We'll see what mochitests say.
> trying toPrivate on an undefined JS::Value

Ah, yes, that's no good.  ;)

> That I do not know the answer to. 

I would really like to understand that bit.
Try says (with just the validity check before measuring memory):

https://tbpl.mozilla.org/?tree=Try&rev=6bf3b963d600

Windows builds are apparently very backed up right now, but at least we have an M4 windows debug build, where the previous problem occurred, and it is green.  So we can have a global on an inner window that...hasn't had its cache set up?  That doesn't sound right.  Or isUndefined returns true on a Value that holds a private...that doesn't sound right either.

I'll poke at this tomorrow.
Comment on attachment 8335255 [details] [diff] [review]
part 2 - make nsGlobalWindow report its proto/iface cache size, if appropriate

Unsetting review request pending explanation for how we can get here or a global with no proto and iface array...
Attachment #8335255 - Flags: review?(bzbarsky)
This patch follows Nick's suggestion in adding a SizeOfIncludingThis method to
ProtoAndIfaceCache.  It makes the code read a little nicer and since I'm also
seeing ProtoAndIfaceCache come up quite a bit in xpconnect code, it'll be
helpful for that eventual patch.  No other changes, so carrying over the r+ bz
originally gave this patch.

I pushed essentially this same patch to try yesterday:

https://tbpl.mozilla.org/?tree=Try&rev=b356c8939728

with a lot of M4 greens.  I don't know why M4 was bugging out on Windows previously.
Maybe a clobber issue or something?  Anyway, this patch was pushed:

https://tbpl.mozilla.org/?tree=Try&rev=d3c62cce67dd

and I'll be retriggering M4 there and relanding this if everything turns out OK.
Attachment #8335254 - Attachment is obsolete: true
Attachment #8335255 - Attachment is obsolete: true
Attachment #8343127 - Flags: review+
Blocks: 946781
Assignee: nobody → nfroyd
https://hg.mozilla.org/mozilla-central/rev/54405d36e0fb
https://hg.mozilla.org/mozilla-central/rev/837a6f6f4c2f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.