canvas memory is leaking from call to GetImageBuffer

RESOLVED FIXED in Firefox 27

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

Trunk
mozilla28
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox26 unaffected, firefox27+ fixed, firefox28+ fixed)

Details

(Whiteboard: [MemShrink][qa-])

Attachments

(3 attachments)

(Assignee)

Description

5 years ago
The following stack turned up in mochitest-bc runs during investigation of bug 937997:

Unreported: 4 blocks in stack trace record 1 of 4,216
 9,584,640 bytes (9,584,640 requested / 0 slop)
 1.53% of the heap (1.53% cumulative);  14.22% of unreported (14.22% cumulative)
 Allocated at
   replace_malloc (/home/froydnj/src/mozilla-central-official/memory/replace/dmd/DMD.cpp:1227) 0x7ffeafcf4260
   mozilla::gfx::SurfaceToPackedBGRA(mozilla::gfx::SourceSurface*) (/opt/build/froydnj/build-mc/content/canvas/src/../../../dist/include/mozilla/gfx/DataSurfaceHelpers.h:50) 0x7ffeabc26abe
   mozilla::dom::CanvasRenderingContext2D::GetImageBuffer(unsigned char**, int*) (/home/froydnj/src/mozilla-central-official/content/canvas/src/CanvasRenderingContext2D.cpp:1078) 0x7ffeabc240e7
   mozilla::dom::CanvasRenderingContext2D::GetInputStream(char const*, char16_t const*, nsIInputStream**) (/home/froydnj/src/mozilla-central-official/content/canvas/src/CanvasRenderingContext2D.cpp:1090) 0x7ffeabc1ccea
   ~nsPromiseFlatString (/opt/build/froydnj/build-mc/content/canvas/src/../../../dist/include/nsTPromiseFlatString.h:61) 0x7ffeabc28bb9
   ~nsCOMPtr (/opt/build/froydnj/build-mc/content/canvas/src/../../../dist/include/nsCOMPtr.h:469) 0x7ffeabc28d25
   mozilla::dom::HTMLCanvasElement::ExtractData(nsAString_internal&, nsAString_internal const&, nsIInputStream**) (/home/froydnj/src/mozilla-central-official/content/html/content/src/HTMLCanvasElement.cpp:395) 0x7ffeabc7a553
   mozilla::dom::HTMLCanvasElement::ToDataURLImpl(JSContext*, nsAString_internal const&, JS::Value const&, nsAString_internal&) (/home/froydnj/src/mozilla-central-official/content/html/content/src/HTMLCanvasElement.cpp:469) 0x7ffeabc7aeca
   toDataURL (/opt/build/froydnj/build-mc/dom/bindings/HTMLCanvasElementBinding.cpp:253) 0x7ffeac3ed6d0
   genericMethod (/opt/build/froydnj/build-mc/dom/bindings/HTMLCanvasElementBinding.cpp:606) 0x7ffeac3f06a1
   CallJSNative (/home/froydnj/src/mozilla-central-official/js/src/jscntxtinlines.h:220) 0x7ffeacd5933b
   Interpret (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:2505) 0x7ffeacd4c325
   RunScript (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:420) 0x7ffeacd58f97
   SendToGenerator (/home/froydnj/src/mozilla-central-official/js/src/jsiter.cpp:1654) 0x7ffeacc8f435
   NativeMethod<js::LegacyGeneratorObject, legacy_generator_next> (/home/froydnj/src/mozilla-central-official/js/src/jsiter.cpp:1813) 0x7ffeacc8fa00
   CallJSNative (/home/froydnj/src/mozilla-central-official/js/src/jscntxtinlines.h:220) 0x7ffeacd5933b
   js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:513) 0x7ffeacd5b4ed
   js::DirectProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) (/home/froydnj/src/mozilla-central-official/js/src/jsproxy.cpp:468) 0x7ffeacccb518
   js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) (/home/froydnj/src/mozilla-central-official/js/src/jswrapper.cpp:457) 0x7ffeacd17ecf
   js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) (/home/froydnj/src/mozilla-central-official/js/src/jsproxy.cpp:2657) 0x7ffeaccd2ef9
   proxy_Call (/home/froydnj/src/mozilla-central-official/js/src/jsproxy.cpp:3065) 0x7ffeaccd2fef
   CallJSNative (/home/froydnj/src/mozilla-central-official/js/src/jscntxtinlines.h:220) 0x7ffeacd5947d
   Interpret (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:2505) 0x7ffeacd4c325
   RunScript (/home/froydnj/src/mozilla-central-official/js/src/vm/Interpreter.cpp:420) 0x7ffeacd58f97

It comes up a couple more times in the run, though with smaller numbers, maybe another 1.5MB of memory total.  Apologies for the bogus bits in the stack, though it should be pretty easy to figure out what's going on there.

Looking through the code, it certainly appears that this ought to be transient memory.
(Assignee)

Comment 1

5 years ago
Created attachment 832281 [details] [diff] [review]
properly free the image buffer in CanvasRenderingContext2D::GetInputStream

The point at which the image buffer is allocated is:

   replace_malloc (/home/froydnj/src/mozilla-central-official/memory/replace/dmd/DMD.cpp:1227) 0x7ffeafcf4260
   mozilla::gfx::SurfaceToPackedBGRA(mozilla::gfx::SourceSurface*) (/opt/build/froydnj/build-mc/content/canvas/src/../../../dist/include/mozilla/gfx/DataSurfaceHelpers.h:50) 0x7ffeabc26abe
   mozilla::dom::CanvasRenderingContext2D::GetImageBuffer(unsigned char**, int*) (/home/froydnj/src/mozilla-central-official/content/canvas/src/CanvasRenderingContext2D.cpp:1078) 0x7ffeabc240e7
   mozilla::dom::CanvasRenderingContext2D::GetInputStream(char const*, char16_t const*, nsIInputStream**) (/home/froydnj/src/mozilla-central-official/content/canvas/src/CanvasRenderingContext2D.cpp:1090) 0x7ffeabc1ccea
   mozilla::dom::ImageEncoder::ExtractDataInternal(...)
   mozilla::dom::ImageEncoder::ExtractData(...)
   mozilla::dom::HTMLCanvasElement::ExtractData(nsAString_internal&, nsAString_internal const&, nsIInputStream**) (/home/froydnj/src/mozilla-central-official/content/html/content/src/HTMLCanvasElement.cpp:395) 0x7ffeabc7a553
   mozilla::dom::HTMLCanvasElement::ToDataURLImpl(JSContext*, nsAString_internal const&, JS::Value const&, nsAString_internal&) (/home/froydnj/src/mozilla-central-official/content/html/content/src/HTMLCanvasElement.cpp:469) 0x7ffeabc7aeca

SurfaceToPackedBGRA allocates with new[]:

http://mxr.mozilla.org/mozilla-central/source/gfx/2d/DataSurfaceHelpers.h#49

so nsAutoArrayPtr is the right smart pointer class to use here (although non-obvious).

ImageEncoder::GetInputStream is used to get the actual input stream, and that calls
imgIEncoder::initFromData:

http://mxr.mozilla.org/mozilla-central/source/image/public/imgIEncoder.idl#83

AFAICS, the existing implementations of initFromData don't store the passed in data,
so the data can be freed afterward, even though that's not explicitly specified.
Attachment #832281 - Flags: review?(bugs)
(Assignee)

Comment 2

5 years ago
Um, not sure who to direct imagelib questions to anymore, picking Seth as a closer-than-me target.  Seth, does the ownership model for imgIEncoder::initFromData in comment 1 make sense?
Flags: needinfo?(seth)
Whiteboard: [MemShrink]

Comment 3

5 years ago
Comment on attachment 832281 [details] [diff] [review]
properly free the image buffer in CanvasRenderingContext2D::GetInputStream

r+, though I think GetImageBuffer should be called after we know we have the
right encoder.

This needs to go to FF27 too, right?
Attachment #832281 - Flags: review?(bugs) → review+
Blocks: 937997
Thank you for catching this and looking into it. We should probably make the same change in WebGLContext [1], no?

[1] http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLContext.cpp#797
(Assignee)

Comment 5

5 years ago
(In reply to Stephen Pohl [:spohl] from comment #4)
> Thank you for catching this and looking into it. We should probably make the
> same change in WebGLContext [1], no?
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/
> WebGLContext.cpp#797

Ah, yes!  I *knew* there was a WebGL version around, I just didn't look hard enough for it.  Thanks for the pointer, patch in just a moment.
(Assignee)

Comment 6

5 years ago
Created attachment 832379 [details] [diff] [review]
free the image buffer for WebGL canvases, too (to be folded)

The same fix for the WebGL case.
Attachment #832379 - Flags: review?(bugs)
Summary: canvas memory either leaking or needs a memory reporter → canvas memory is leaking from call to GetImageBuffer
(Assignee)

Updated

5 years ago
Blocks: 938702
(In reply to Nathan Froyd (:froydnj) from comment #2)
> Um, not sure who to direct imagelib questions to anymore, picking Seth as a
> closer-than-me target.  Seth, does the ownership model for
> imgIEncoder::initFromData in comment 1 make sense?

With the caveat that I've never actually touched the image encoder code (so I'm answering this question by inspecting the source, just as you are), this analysis appears correct to me. It also makes sense intuitively. The same appears to be true for addImageFrame, which initFromData generally calls internally. It'd make sense to add comments to both of these methods explicitly stating that they do not take ownership of or retain a reference to the data.
Flags: needinfo?(seth)

Updated

5 years ago
Attachment #832379 - Flags: review?(bugs) → review+
(Assignee)

Comment 8

5 years ago
Landed on the CLOSED TREE: https://hg.mozilla.org/integration/mozilla-inbound/rev/f29af144b651
Assignee: nobody → nfroyd
Flags: in-testsuite-
Looks like this was introduced by

Bug 817700 - Make <canvas>.toBlob run asynchronously - canvas changes. r=roc,bz
Blocks: 817700

Updated

5 years ago
status-firefox27: --- → affected
tracking-firefox27: --- → ?
(In reply to Benoit Jacob [:bjacob] from comment #9)
> Looks like this was introduced by
> 
> Bug 817700 - Make <canvas>.toBlob run asynchronously - canvas changes.
> r=roc,bz

Argh! This doesn't seem to be a problem for the toBlob case because we're handing ownership of the image buffer to EncodingRunnable (via the new ImageEncoder class), which already uses nsAutoArrayPtr<uint8_t> to store the buffer. I refactored the logic to get the image buffer in bug 817700, but must have missed that toDataURL also calls GetInputStream, which calls GetImageBuffer. :-(

Unrelated, but I'll file a bug to make toDataURL async as well.
(In reply to Stephen Pohl [:spohl] from comment #10)
> I'll file a bug to make toDataURL async as well.

Filed bug 938971.
Blocks: 938971
(In reply to Stephen Pohl [:spohl] from comment #10)
> Argh! This doesn't seem to be a problem for the toBlob case because we're
> handing ownership of the image buffer to EncodingRunnable (via the new
> ImageEncoder class), which already uses nsAutoArrayPtr<uint8_t> to store the
> buffer. I refactored the logic to get the image buffer in bug 817700, but
> must have missed that toDataURL also calls GetInputStream, which calls
> GetImageBuffer. :-(

It might be helpful if someone could just double-check that this is indeed done properly in bug 817700. I'll be happy to make any changes/improvements necessary.
https://hg.mozilla.org/mozilla-central/rev/f29af144b651
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Updated

5 years ago
status-firefox26: --- → unaffected
status-firefox28: --- → fixed
tracking-firefox27: ? → +
tracking-firefox28: --- → +
(Assignee)

Comment 14

5 years ago
Created attachment 8333811 [details] [diff] [review]
ensure the image buffers for canvases are freed

This is the rollup patch for aurora.
Attachment #8333811 - Flags: review+
(Assignee)

Comment 15

5 years ago
Comment on attachment 8333811 [details] [diff] [review]
ensure the image buffers for canvases are freed

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 817700
User impact if declined: Running out of memory on sites that use canvas heavily.
Testing completed (on m-c, etc.): On m-c.  Leak detection also confirms that the leak is gone.
Risk to taking this patch (and alternatives if risky): Minimal.
String or IDL/UUID changes made by this patch: None.
Attachment #8333811 - Flags: approval-mozilla-aurora?

Updated

5 years ago
Attachment #8333811 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 17

5 years ago
Huh, the status-firefox27 flag set in comment 16 somehow got lost.  Let's try that again.
status-firefox27: affected → fixed
Whiteboard: [MemShrink] → [MemShrink][qa-]
(In reply to Wes Kocher (:KWierso) from comment #18)
> https://hg.mozilla.org/mozilla-central/rev/d3b1da6c42c6

Ignore this, it should've been in bug 983612, not here.
You need to log in before you can comment on or make changes to this bug.