938612 - canvas memory is leaking from call to GetImageBuffer

Status: RESOLVED FIXED

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

Description

[Nathan Froyd [:froydnj]]

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.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: 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.

Comment 2

[Nathan Froyd [:froydnj]]

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?

Comment 3

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

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?

Comment 4

[Stephen A Pohl [:spohl]]

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

Comment 5

[Nathan Froyd [:froydnj]]

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

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: free the image buffer for WebGL canvases, too (to be folded)

The same fix for the WebGL case.

Comment 7

[Seth Fowler [:seth] [:s2h]]

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

Comment 8

[Nathan Froyd [:froydnj]]

Landed on the CLOSED TREE: https://hg.mozilla.org/integration/mozilla-inbound/rev/f29af144b651

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

Looks like this was introduced by

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

Comment 10

[Stephen A Pohl [:spohl]]

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

Comment 11

[Stephen A Pohl [:spohl]]

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

Filed bug 938971.

Comment 12

[Stephen A Pohl [:spohl]]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f29af144b651

Comment 14

[Nathan Froyd [:froydnj]]

Attachment: ensure the image buffers for canvases are freed

This is the rollup patch for aurora.

Comment 15

[Nathan Froyd [:froydnj]]

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7cffbb50b68c

Comment 17

[Nathan Froyd [:froydnj]]

Huh, the status-firefox27 flag set in comment 16 somehow got lost.  Let's try that again.

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d3b1da6c42c6

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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