Closed
Bug 925416
Opened 11 years ago
Closed 11 years ago
Implement "refcount logging" for blob URIs
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: mccr8, Assigned: jld)
Details
(Keywords: perf, Whiteboard: [c=memory p= s= u=] [MemShrink:P2])
Attachments
(1 file, 3 obsolete files)
9.96 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
Bug 919864 seems to involve a leak caused by calls to URL.createObjectURL without a matching call to URL.revokeObjectURL(). In addition to counting how many are live (bug 924696) it would be cool if we could enable a mode that, when you create a new one, took a stack trace, including JS (or maybe only JS) and stuck it in the hash table or whatever that keeps track of the blobs that are alive. Once you'd enabled this mode, you could show the stacks for any live blobs, which would help a lot with debugging. Blobs are tricky because they are C++, but not cycle collected, so we can't really use GC/CC logs to understand why they are alive. The blobs themselves may also not show up in DMD logs.
![]() |
||
Updated•11 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P2]
Assignee | ||
Comment 1•11 years ago
|
||
I've already been poking at this code, and I think I mostly know what needs to happen for this. I guess we'd use a pref to enable the stacks?
Assignee: nobody → jld
Reporter | ||
Comment 2•11 years ago
|
||
Great! Yes, a pref seems reasonable. I'd think we'd want something that a B2G app developer could easily toggle.
Updated•11 years ago
|
Assignee | ||
Comment 3•11 years ago
|
||
I finally found some time to work on this. A few notes: The stack info, if enabled, is stored in the reporter path so we don't need an extra log file; it more or less fits. The handling of multiply-URLed blobs is maybe a little unwieldy (and possibly also more elaborate than it needs to be), but I suspect they'll be uncommon in practice.
Also, even without setting the stack walking pref ahead of time, we still get memory usage divided by owning page and a list of the blobs with URLs (which might be held in JS strings and thus show up in the GC log), which could be useful.
Caveat: there's currently no way to tell how many of these blobs are already accounted for. Creating a blob from an ArrayBuffer causes it to show up under explicit/dom, but we've observed that blobs received over IPC are dark matter.
Example (hopefully bugzilla doesn't mangle this too much):
0.18 MB (100.0%) -- blob-urls
└──0.18 MB (100.0%) -- owner(app://system.gaiamobile.org/index.html)/js(/shared/js/settings_url.js, line=21)
├──0.08 MB (47.37%) ── js(/js/bootstrap.js, line=83)/blob:133efbfd-f37a-45a0-9ab8-cba7898f4919
├──0.08 MB (47.37%) ── js(/js/lockscreen.js, line=271)/blob:8bc99296-bca6-4347-854e-a2c90d3a95b5
└──0.01 MB (05.27%) ── js(/js/notifications.js, line=101)/blob:f62815fd-963f-41f5-9b3e-7402e5222a8f
Attachment #8343467 -
Flags: review?(khuey)
![]() |
||
Comment 4•11 years ago
|
||
Comment on attachment 8343467 [details] [diff] [review]
bug925416-report-blobs-hg0.diff
Review of attachment 8343467 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/base/src/nsHostObjectProtocolHandler.cpp
@@ +56,5 @@
> + NS_IMETHOD GetName(nsACString& aName)
> + {
> + aName.AssignLiteral("blob-urls");
> + return NS_OK;
> + }
Thanks to bug 943660, you no longer need GetName() in a memory reporter.
@@ +59,5 @@
> + return NS_OK;
> + }
> +
> + NS_IMETHOD CollectReports(nsIMemoryReporterCallback *aCallback,
> + nsISupports *aClosure)
I've been using nsIHandleReportCallback, which is a typedef for nsIMemoryReporterCallback, instead. Also, aData is better than aClosure, because it's not really a closure. (Both of these are recent changes, I admit.)
@@ +72,5 @@
> + }
> + return NS_OK;
> + }
> +
> + // Initialize info->mStack to record JS tack info, if enabled.
s/tack/stack/
@@ +141,5 @@
> + }
> +
> + private:
> + struct EnumArg {
> + nsIMemoryReporterCallback *mCallback;
s/MemoryReporter/HandleReport/
@@ +142,5 @@
> +
> + private:
> + struct EnumArg {
> + nsIMemoryReporterCallback *mCallback;
> + nsISupports *mClosure;
s/mClosure/mData/.
Comment on attachment 8343467 [details] [diff] [review]
bug925416-report-blobs-hg0.diff
Review of attachment 8343467 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/base/src/nsHostObjectProtocolHandler.cpp
@@ +59,5 @@
> + return NS_OK;
> + }
> +
> + NS_IMETHOD CollectReports(nsIMemoryReporterCallback *aCallback,
> + nsISupports *aClosure)
stars on the left
@@ +74,5 @@
> + }
> +
> + // Initialize info->mStack to record JS tack info, if enabled.
> + // The string generated here is used in ReportCallback, below.
> + static void GetJSStackForBlob(DataInfo *aInfo)
here too
@@ +76,5 @@
> + // Initialize info->mStack to record JS tack info, if enabled.
> + // The string generated here is used in ReportCallback, below.
> + static void GetJSStackForBlob(DataInfo *aInfo)
> + {
> + nsCString &stack = aInfo->mStack;
& too
@@ +78,5 @@
> + static void GetJSStackForBlob(DataInfo *aInfo)
> + {
> + nsCString &stack = aInfo->mStack;
> + MOZ_ASSERT(stack.IsEmpty());
> + uint32_t maxFrames = Preferences::GetUint("memory.blob_report.stack_frames");
nit-picky, but const uint32_t
@@ +87,5 @@
> +
> + nsresult rv;
> + nsCOMPtr<nsIXPConnect> xpc(do_GetService(nsIXPConnect::GetCID(), &rv));
> + NS_ENSURE_SUCCESS_VOID(rv);
> + NS_ENSURE_TRUE_VOID(xpc);
Just use nsContentUtils::XPConnect() and skip all this XPCOM boilerplate.
@@ +101,5 @@
> + }
> +
> + for (uint32_t i = 0; i < maxFrames && frame; ++i) {
> + nsAutoCString fileNameEscaped;
> + char *fileName;
and here
@@ +107,5 @@
> +
> + rv = frame->GetFilename(&fileName);
> + NS_ENSURE_SUCCESS_VOID(rv);
> + rv = frame->GetLineNumber(&lineNumber);
> + NS_ENSURE_SUCCESS_VOID(rv);
I would just ignore return values here.
@@ +149,5 @@
> +
> + // Determine number of URLs per blob, to handle the case where it's > 1.
> + static PLDHashOperator CountCallback(nsCStringHashKey::KeyType aKey,
> + DataInfo *aInfo,
> + void *aUserArg)
line up your args, stars on the left
@@ +155,5 @@
> + EnumArg *envp = static_cast<EnumArg *>(aUserArg);
> + nsIDOMBlob *blob;
> +
> + aInfo->mObject->QueryInterface(NS_GET_IID(nsIDOMBlob),
> + reinterpret_cast<void**>(&blob));
This leaks, QueryInterface AddRefs.
This should be
nsCOMPtr<nsIDOMBlob> blob;
blob = do_QueryInterface(aInfo->mObject);
@@ +170,5 @@
> + EnumArg *envp = static_cast<EnumArg *>(aUserArg);
> + nsIDOMBlob *blob;
> +
> + aInfo->mObject->QueryInterface(NS_GET_IID(nsIDOMBlob),
> + reinterpret_cast<void**>(&blob));
Same here.
Attachment #8343467 -
Flags: review?(khuey) → review-
Assignee | ||
Comment 6•11 years ago
|
||
Updated with suggested changes.
Attachment #8343467 -
Attachment is obsolete: true
Attachment #8357437 -
Flags: review?(khuey)
Comment on attachment 8357437 [details] [diff] [review]
bug925416-report-blobs-hg1.diff
Review of attachment 8357437 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/base/src/nsHostObjectProtocolHandler.cpp
@@ +80,5 @@
> + return;
> + }
> +
> + nsresult rv;
> + nsCOMPtr<nsIXPConnect> xpc = nsContentUtils::XPConnect();
This can just be nsIXPConnect* xpc. The function does not return an already_AddRefed, and there's no reason to hold a strong ref.
Attachment #8357437 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 8•11 years ago
|
||
Carrying over r=khuey.
Attachment #8357437 -
Attachment is obsolete: true
Attachment #8357965 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 9•11 years ago
|
||
Keywords: checkin-needed
Comment 10•11 years ago
|
||
Backed out for bustage. Please make sure future patches compile before requesting checkin (this is the second patch of yours I've backed out this morning for bustage).
https://hg.mozilla.org/integration/mozilla-inbound/rev/441af05d123b
https://tbpl.mozilla.org/php/getParsedLog.php?id=32818000&tree=Mozilla-Inbound
Comment 11•11 years ago
|
||
The build bustage was on OSX only. However, other platforms were crashing in mochitest-bc.
https://tbpl.mozilla.org/php/getParsedLog.php?id=32819371&tree=Mozilla-Inbound
Assignee | ||
Comment 12•11 years ago
|
||
Fix a null pointer deref and some integer coercion ambiguity C++ weirdness.
Currently trying: https://tbpl.mozilla.org/?tree=Try&rev=5a75c56e233e
(Passed against Friday's m-c, more or less; attempts, in order: https://tbpl.mozilla.org/?tree=Try&rev=0ca0c38bfa1d https://tbpl.mozilla.org/?tree=Try&rev=f168bc116f6c https://tbpl.mozilla.org/?tree=Try&rev=06ef6edbf05f )
Attachment #8357965 -
Attachment is obsolete: true
Attachment #8359348 -
Flags: review?(khuey)
Attachment #8359348 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 13•11 years ago
|
||
Trying this again. https://tbpl.mozilla.org/?tree=Try&rev=5a75c56e233e is green except for the Linux debug mochitest-bc, which has gotten several different oranges that look unrelated.
Keywords: checkin-needed
Comment 14•11 years ago
|
||
Keywords: checkin-needed
Comment 15•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•