Closed Bug 1314011 Opened 3 years ago Closed 3 years ago

MemoryReporter for BlobImplString

Categories

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

50 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: baku, Assigned: baku)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch blob_memory_report_string.patch (obsolete) — Splinter Review
No description provided.
Attachment #8805917 - Flags: review?(bugs)
Summary: MemoryReporter of BlobImplString → MemoryReporter for BlobImplString
Comment on attachment 8805917 [details] [diff] [review]
blob_memory_report_string.patch

This has similar issues as Bug 1313859 (including the raw pointer usage!). So, f+, but someone more familiar with memory report should review.

I wish nsIMemoryReporter.idl told how callback() can be used.
Attachment #8805917 - Flags: review?(bugs) → feedback+
Attached patch blob_memory_report_string.patch (obsolete) — Splinter Review
Attachment #8805917 - Attachment is obsolete: true
Attachment #8805935 - Flags: review?(n.nethercote)
Comment on attachment 8805935 [details] [diff] [review]
blob_memory_report_string.patch

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

This looks very similar to the patch in bug 1313859 and my comments are basically the same.

::: dom/base/File.cpp
@@ +1067,5 @@
> +                 nsISupports* aData, bool aAnonymize) override
> +  {
> +    if (mBlobImpl) {
> +      MOZ_COLLECT_REPORT(
> +        "explicit/dom/memory-file-data/small",

Same issue as before with the repeated path name.

@@ +1068,5 @@
> +  {
> +    if (mBlobImpl) {
> +      MOZ_COLLECT_REPORT(
> +        "explicit/dom/memory-file-data/small",
> +        KIND_HEAP, UNITS_BYTES, mBlobImpl->mData.Length(),

Again, this length is computed analytically rather than being measured with a MallocSizeOf function.

@@ +1086,5 @@
> +  ~MemoryReporter()
> +  {}
> +
> +  // Raw pointer because this class is kept alive by the BlobImplString.
> +  BlobImplString* mBlobImpl;

This is the kind of trickiness that can be avoided if BlobImplString implements nsIMemoryReporter itself.
Attachment #8805935 - Flags: review?(n.nethercote) → feedback+
Attachment #8805935 - Attachment is obsolete: true
Attachment #8806243 - Flags: review?(n.nethercote)
Comment on attachment 8806243 [details] [diff] [review]
blob_memory_report_string.patch

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

r=me with the separate Init() function added to avoid the refcounting problem. Thanks.

::: dom/base/File.cpp
@@ +1056,5 @@
> +                               const nsAString& aContentType)
> +  : BlobImplBase(aContentType, aData.Length())
> +  , mData(aData)
> +{
> +  RegisterWeakMemoryReporter(this);

Again, registering from a separate Init() function will solve the refcount problem.
Attachment #8806243 - Flags: review?(n.nethercote) → review+
https://hg.mozilla.org/mozilla-central/rev/90f8b4647d6e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.