canvas.toBlob() doesn't increment the JS malloc counter

RESOLVED FIXED in Firefox 18

Status

()

defect
RESOLVED FIXED
7 years ago
a month ago

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

Tracking

Trunk
mozilla19
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

(Whiteboard: [MemShrink][soft-blocker])

Attachments

(1 attachment, 2 obsolete attachments)

There's a JS malloc counter which we should increment every time we do a big allocation; it affects how often the GC gets run.

Are we incrementing this counter when we take a blob screenshot (i.e., when we do canvas.toBlob)?  If not, we should be.
(Assignee)

Updated

7 years ago
Blocks: slim-fast
(Assignee)

Updated

7 years ago
Whiteboard: [MemShrink]
(Assignee)

Updated

7 years ago
Summary: Does taking blob screenshots increment the JS malloc counter? → canvas.toBlob() doesn't increment the JS malloc counter
Posted patch Patch, v1 (obsolete) — Splinter Review
Attachment #679397 - Flags: review?(khuey)
Comment on attachment 679397 [details] [diff] [review]
Patch, v1

Heh, I didn't actually compile this

> +  if (context) {

if (cx)
Posted patch Patch, v2 (obsolete) — Splinter Review
Increment the malloc counter in one other place.  This covers all instances of nsDOMMemoryFile, which I guess is what we want?
Attachment #679397 - Attachment is obsolete: true
Attachment #679397 - Flags: review?(khuey)
Attachment #679400 - Flags: review?(khuey)
(Assignee)

Updated

7 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Updated

7 years ago
Blocks: 798002
Comment on attachment 679400 [details] [diff] [review]
Patch, v2

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

::: content/html/content/src/nsHTMLCanvasElement.cpp
@@ +682,5 @@
>  
> +  JSContext* cx = nsContentUtils::GetCurrentJSContext();
> +  if (cx) {
> +    JS_updateMallocCounter(cx, imgSize);
> +  }

Why are you doing this twice here?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #5)
> Comment on attachment 679400 [details] [diff] [review]
> Patch, v2
> 
> Review of attachment 679400 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/html/content/src/nsHTMLCanvasElement.cpp
> @@ +682,5 @@
> >  
> > +  JSContext* cx = nsContentUtils::GetCurrentJSContext();
> > +  if (cx) {
> > +    JS_updateMallocCounter(cx, imgSize);
> > +  }
> 
> Why are you doing this twice here?

Because I was writing a code during a meeting.
Posted patch Patch v3Splinter Review
Now with less less repetition.
Attachment #679400 - Attachment is obsolete: true
Attachment #679400 - Flags: review?(khuey)
Attachment #679461 - Flags: review?(khuey)
Soft basecamp blocker because this should help us purge screenshots out of the heap more quickly and because it's a low-risk change.
blocking-basecamp: --- → +
Whiteboard: [MemShrink] → [MemShrink][soft-blocker]
https://hg.mozilla.org/mozilla-central/rev/284ad66e8bda
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
(Assignee)

Updated

7 years ago
Duplicate of this bug: 802662
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.