1314011 - MemoryReporter for BlobImplString

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

Attachment: blob_memory_report_string.patch

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: blob_memory_report_string.patch

Comment 3

[Nicholas Nethercote [inactive]]

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.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: blob_memory_report_string.patch

Comment 5

[Nicholas Nethercote [inactive]]

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.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/90f8b4647d6e
MemoryReporter for BlobImplString, r=njn

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/90f8b4647d6e