686345 - Negative value for canvas-2d-pixel-bytes in about:memory

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Andreas Jung]

Other Measurements
   -7,859,276 B -- canvas-2d-pixel-bytes
            0 B -- gfx-d2d-surfacecache
            0 B -- gfx-d2d-surfacevram
        5,424 B -- gfx-surface-image
   37,351,417 B -- gfx-surface-win32
  434,671,722 B -- heap-allocated
  552,775,680 B -- heap-committed
    2,793,472 B -- heap-dirty
  412,568,708 B -- heap-unallocated
             43 -- js-compartments-system
            321 -- js-compartments-user
  250,609,664 B -- js-gc-heap
   39,413,696 B -- js-gc-heap-arena-unused
            0 B -- js-gc-heap-chunk-clean-unused
   82,848,516 B -- js-gc-heap-chunk-dirty-unused
         48.78% -- js-gc-heap-unused-fraction
  951,693,312 B -- private
  854,806,528 B -- resident
      970,752 B -- shmem-allocated
      970,752 B -- shmem-mapped
1,394,135,040 B -- vsize

Note the negative value for canvas-2d-pixel-bytes.

According to the explanation in the tooltip it's (width * height * 4) bytes, so how can it be negative?

Steps to reproduce ? (*)
1) Open deviantart.com
2) Open a lot of images in new tabs
3) Close about half of the tabs again

(*)
I didn't try to reproduce yet, but this is what I was doing when I noticed the bug.

Comment 1

[Daniel C]

Mozilla/5.0 (X11; Linux i686; rv:9.0a1) Gecko/20110913 Firefox/9.0a1

Confirming.

I also see a negative value for canvas-2d-pixel-bytes. I think, but I can't be sure, that the value wasn't negative until I clicked "Minimize memory usage". I'll attach the verbose output.

Bug 572274 comment 5 also mentions this.

Comment 2

[Daniel C]

Attachment: Verbose output showing negative canvas-2d-pixel-bytes

This is the verbose output of about:memory showing the negative value for canvas-2d-pixel-bytes. There are also a couple of zombie compartments, but I'll look into that separately.

This is with no other tabs open and after clicking "Minimize memory usage", leaving it for a few minutes, and clicking it again. I think that before I clicked it, the value wasn't negative and it was only when I clicked it that I noticed it change, but I can't be sure.

If there's anything else suspicious in there then let me know and I can file a new bug.

This was with my usual profile which has a few extensions. I haven't been able to reproduce it with this profile or a clean one.

Comment 3

[Justin Lebar (not reading bugmail)]

Could you please attach a concrete STR?  I've tried "open a bunch of deviantart images", but the ones I'm selecting apparently aren't creating canvas objects.

Comment 4

[Andreas Jung]

Well, like I said I didn't try to reproduce and trying now I can't reproduce the issue with these steps. Probably it was coincidence that I noticed the issue while being on deviantart and the site is not related to the issue.

Though the reason I opened about:memory was that I tried to investigate an issue with compartments staying around for too long. I therefore clicked on the GC and CC buttons. I didn't really pay attention if the value was negative before or only after clicking the GC and CC buttons.

Comment 2 mentions "Minimize memory usage" which triggers GC and CC as well, so maybe this issue is related to GC and/or CC...

Comment 5

[Justin Lebar (not reading bugmail)]

(In response to comment 1)
> Confirming.

Daniel, do you have a STR for this?

Comment 6

[Justin Lebar (not reading bugmail)]

Oh, apparently just loading a random canvas page (e.g. [1]) and closing it was enough to trigger this bug for me.

[1] http://aerotwist.com/lab/creating-particles-with-three-js/demo/

Comment 7

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Comment 8

[Justin Lebar (not reading bugmail)]

The site in comment 6 doesn't actually trigger this -- it's a webgl page.  I must have gone somewhere else right beforehand which triggered it, but I haven't been able to reproduce.

In any case, the change here makes the code obviously correct.  I think.  :)

Comment 9

[Andreas Jung]

I found steps to reproduce the bug reliably in a clean profile:

Open http://www.atopon.org/dissolve/dissolve.html in a new tab
=> 3.17 MB -- canvas-2d-pixel-bytes

Close the tab and wait a few seconds
=> 0.90 MB -- canvas-2d-pixel-bytes

Undo Close Tab
=> 2.27 MB -- canvas-2d-pixel-bytes

Close the tab again and wait a few seconds
=> -1.80 MB -- canvas-2d-pixel-bytes

You can repeat steps 3 and 4 to get an even bigger negative value.

Comment 10

[Daniel C]

(In reply to Justin Lebar [:jlebar] from comment #5)
> (In response to comment 1)
> > Confirming.
> 
> Daniel, do you have a STR for this?

I just saw this bug in the middle of a browsing session, opened about:memory and went "oh yeah, same here".

Comment 9 works reliably for me though.

Comment 11

[Justin Lebar (not reading bugmail)]

(In reply to Andreas Jung from comment #9)
> I found steps to reproduce the bug reliably in a clean profile:

Thank you, Andreas!

Just tested with my patch, and it works properly.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 560509 [details] [diff] [review]
Patch v1

Can you give a description of what this patch is doing?

Comment 13

[Justin Lebar (not reading bugmail)]

So there's a global count of how much memory is being used by canvases.

Previously, nsCanvasRenderingContext2D::Reset() would subtract out this canvas's size from the global count if |mValid && !mDocShell|.

But this was wrong somehow in an edge case; either we subtracted outselves twice, or once subtracted without adding ourselves first.

Rather than try to figure this out and potentially miss another edge case, I just added a member to the canvas object so it remembers if it's added itself to the global count.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

(In reply to Justin Lebar [:jlebar] from comment #13)
> So there's a global count of how much memory is being used by canvases.
> 
> Previously, nsCanvasRenderingContext2D::Reset() would subtract out this
> canvas's size from the global count if |mValid && !mDocShell|.
> 
> But this was wrong somehow in an edge case; either we subtracted outselves
> twice, or once subtracted without adding ourselves first.

I'd like to know more about this edge case. Can we simplify the code so that the correspondence between when we increment and decrement is obvious?

Comment 16

[Nicholas Nethercote [inactive]]

This is a great example of why I prefer "crawl over a data structure" memory reporters to "maintain a global counter" memory reporters :)  Can we make this memory reporter a crawler?

Comment 17

[Justin Lebar (not reading bugmail)]

Hm, the current code is pretty broken.  When I load [1] in a instrumented build, I get

this=0x4710010 - gCanvasMemoryUsed -= 180000, new value -180000
this=0x4710010 - gCanvasMemoryUsed -= 805200, new value -985200
this=0x4710010 - gCanvasMemoryUsed += 4310504, new value 3325304

That is, we start subtracting for a canvas before we ever add anything!

[1] http://www.atopon.org/dissolve/dissolve.html

Comment 18

[Justin Lebar (not reading bugmail)]

So the problem seems to be that we're calling nsCanvasRenderingContext2D::Reset() before calling nsCanvasRenderingContext2D::EnsureSurface().  I'll post a patch which seems to fix the reporting in a moment...

Comment 19

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2

Comment 20

[Justin Lebar (not reading bugmail)]

Comment on attachment 563116 [details] [diff] [review]
Patch v2

Actually, I just tested on my Windows 7 machine, and Azure works properly.

Comment 21

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 563116 [details] [diff] [review]
Patch v2

It seems like 'if (mSurface)' should be a sufficient test here no?

Comment 22

[Justin Lebar (not reading bugmail)]

I don't think so; I think someone can call InitializeWithSurface and pass a surface in without incrementing gCanvasMemoryUsed.

mSurface && !mDocShell might be sufficient, but checking mValid seems semantically right.

Honestly, it feels pretty fragile to do it this way rather than to explicitly remember whether we incremented gCanvasMemoryUsed, precisely because figuring out the right condition is nontrivial.

Comment 23

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/131fcce099af

Comment 24

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/131fcce099af

Comment 25

[Ulrich Windl]

I wonder whether the bug should be re-opened: After my Firefox 20.0 had used 1.3GB of virtual memory, I had a look in about:memory, and I found:
-311,966,480 B ── canvas-2d-pixel-bytes [?!]