925416 - Implement "refcount logging" for blob URIs

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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?

Comment 2

[Andrew McCreight [:mccr8]]

Great!  Yes, a pref seems reasonable.  I'd think we'd want something that a B2G app developer could easily toggle.

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug925416-report-blobs-hg0.diff

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

Comment 4

[Nicholas Nethercote [inactive]]

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 5

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

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.

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug925416-report-blobs-hg1.diff

Updated with suggested changes.

Comment 7

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

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.

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug925416-report-blobs-hg2.diff

Carrying over r=khuey.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ce9e632939e

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug925416-report-blobs-hg3.diff

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 )

Comment 13

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e90119dec6f

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1e90119dec6f