Closed Bug 924696 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
blocking-b2g koi+
Tracking Status
firefox26 --- fixed
firefox27 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: khuey, Assigned: jld)

Details

(Keywords: perf, Whiteboard: [c=memory p= s=2013.10.25 u=1.2] [MemShrink:P2])

Attachments

(1 file, 1 obsolete file)

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.
Would having the URIs themselves be useful?  I guess probably not unless you are actually at the machine.
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?
(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).
We should implement refcount logging for blobs.  I'm only sort of joking.
No longer blocks: 919864
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?
Keywords: perf
Whiteboard: [MemShrink] → [c=memory p= s= u=1.2] [MemShrink]
Comment #5 is covered by bug 925416; this bug is just for exposing the count/size of blobbed items.
Whiteboard: [c=memory p= s= u=1.2] [MemShrink] → [c=memory p= s= u=1.2] [MemShrink:P2]
Attached patch bug924696-count-blobs-hg0.diff (obsolete) — Splinter Review
Assignee: nobody → jld
Status: NEW → ASSIGNED
Attachment #821300 - Flags: review?(khuey)
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.".
Attachment #821300 - Flags: review?(khuey) → review+
Yeah I'm happy to have njn review here.
(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?
Carrying over r=njn.
Attachment #821300 - Attachment is obsolete: true
Attachment #821866 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/f09869b05ae3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Whiteboard: [c=memory p= s= u=1.2] [MemShrink:P2] → [c=memory p= s=2013.10.25 u=1.2] [MemShrink:P2]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.