924696 - Add a counting memory reporter for the number of blob URIs outstanding

Status: RESOLVED FIXED

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

Description

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

Doing createObjectURL and forgetting to revokeObjectURL is an easy way to leak blobs.  Blobs can be very large if they're backed by memory instead of disk, and some of them show up in heap-unclassified.  We should report a count of outstanding blob URIs (that is, the count of entries in the gDataTable hashtable) in about:memory.

Comment 1

[Andrew McCreight [:mccr8]]

Would having the URIs themselves be useful?  I guess probably not unless you are actually at the machine.

Comment 2

[Andrew McCreight [:mccr8]]

Though maybe we could somehow dump the blob's URI into the GC log for its reflector (?) and then correlate things in about:memory with that?

Comment 3

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

(In reply to Andrew McCreight [:mccr8] from comment #2)
> Though maybe we could somehow dump the blob's URI into the GC log for its
> reflector (?) and then correlate things in about:memory with that?

The mapping here is neither injective nor surjective (that is, there may be multiple or zero URIs for a blob, and there may be no reflector at all if the wrapper has been GCd but the URI is keeping it alive).

Comment 4

[Andrew McCreight [:mccr8]]

We should implement refcount logging for blobs.  I'm only sort of joking.

Comment 5

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

The info that I needed in bug 919864 was the JS stack, or at least the JS leaf frame.  We could try to record the stack in the DataInfo (`extern "C" char* PrintJSStack();` is probably the wrong way to do that), or part of it, and expose that in about:memory, maybe?

Comment 6

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

Comment #5 is covered by bug 925416; this bug is just for exposing the count/size of blobbed items.

Comment 7

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

Attachment: bug924696-count-blobs-hg0.diff

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 821300 [details] [diff] [review]
bug924696-count-blobs-hg0.diff

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

Thanks!  This is simple enough on the content/ side that I will steal the review.  I'm pretty sure khuey won't mind.

::: content/base/src/nsHostObjectProtocolHandler.cpp
@@ +24,5 @@
>    nsCOMPtr<nsIPrincipal> mPrincipal;
>  };
>  
>  static nsClassHashtable<nsCStringHashKey, DataInfo>* gDataTable;
> +static bool gMemoryReportingInited = false;

Move this in nsHostObjectProtocolHandler()'s constructor, to minimize its exposure?

@@ +36,5 @@
> +  HostObjectURLsReporter()
> +    : MemoryUniReporter("host-object-urls",
> +                        KIND_OTHER, UNITS_COUNT,
> +                        "The number of host objects stored for access via URLs "
> +                        "(e.g., blobs passed to URL.createObjectURL).")

I'd remove the comma after "e.g.".

Comment 9

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

Yeah I'm happy to have njn review here.

Comment 10

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

(In reply to Nicholas Nethercote [:njn] from comment #8)
> ::: content/base/src/nsHostObjectProtocolHandler.cpp
> @@ +24,5 @@
> >    nsCOMPtr<nsIPrincipal> mPrincipal;
> >  };
> >  
> >  static nsClassHashtable<nsCStringHashKey, DataInfo>* gDataTable;
> > +static bool gMemoryReportingInited = false;
> 
> Move this in nsHostObjectProtocolHandler()'s constructor, to minimize its
> exposure?

Good point; done.

> 
> @@ +36,5 @@
> > +  HostObjectURLsReporter()
> > +    : MemoryUniReporter("host-object-urls",
> > +                        KIND_OTHER, UNITS_COUNT,
> > +                        "The number of host objects stored for access via URLs "
> > +                        "(e.g., blobs passed to URL.createObjectURL).")
> 
> I'd remove the comma after "e.g.".

Until now I'd only run into claims that the comma must be present, but sources indicate this is one of those locali[zs]ation issues.  That said, I don't particularly care… and shorter strings are better, right?

Comment 11

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

Attachment: bug924696-count-blobs-hg1.diff

Carrying over r=njn.

Comment 12

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f09869b05ae3

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f09869b05ae3

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f93851154cf0