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

RESOLVED FIXED in Firefox 26, Firefox OS v1.2

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: khuey, Assigned: jld)

Tracking

({perf})

unspecified
mozilla27
Points:
---

Firefox Tracking Flags

(blocking-b2g:koi+, firefox26 fixed, firefox27 fixed, b2g-v1.2 fixed)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

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.
(Assignee)

Updated

5 years ago
No longer blocks: 919864
(Assignee)

Comment 5

5 years ago
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?

Updated

5 years ago
Keywords: perf
Whiteboard: [MemShrink] → [c=memory p= s= u=1.2] [MemShrink]
(Assignee)

Comment 6

5 years ago
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]
(Assignee)

Comment 7

5 years ago
Created attachment 821300 [details] [diff] [review]
bug924696-count-blobs-hg0.diff
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.
(Assignee)

Comment 10

5 years ago
(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?
(Assignee)

Comment 11

5 years ago
Created attachment 821866 [details] [diff] [review]
bug924696-count-blobs-hg1.diff

Carrying over r=njn.
Attachment #821300 - Attachment is obsolete: true
Attachment #821866 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f09869b05ae3
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27

Updated

5 years ago
status-firefox26: --- → fixed

Updated

5 years ago
Whiteboard: [c=memory p= s= u=1.2] [MemShrink:P2] → [c=memory p= s=2013.10.25 u=1.2] [MemShrink:P2]
status-b2g-v1.2: --- → fixed
status-firefox27: --- → fixed
You need to log in before you can comment on or make changes to this bug.