Closed Bug 925416 Opened 8 years ago Closed 7 years ago

Implement "refcount logging" for blob URIs

Categories

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

defect
Not set
normal

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)

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.
Whiteboard: [MemShrink] → [MemShrink:P2]
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
Great!  Yes, a pref seems reasonable.  I'd think we'd want something that a B2G app developer could easily toggle.
Status: NEW → ASSIGNED
Keywords: perf
Whiteboard: [MemShrink:P2] → [c=memory p= s= u=] [MemShrink:P2]
Attached patch bug925416-report-blobs-hg0.diff (obsolete) — Splinter Review
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 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-
Attached patch bug925416-report-blobs-hg1.diff (obsolete) — Splinter Review
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+
Attached patch bug925416-report-blobs-hg2.diff (obsolete) — Splinter Review
Carrying over r=khuey.
Attachment #8357437 - Attachment is obsolete: true
Attachment #8357965 - Flags: review+
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
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
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)
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
https://hg.mozilla.org/mozilla-central/rev/1e90119dec6f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.