FateRover Reporter
	Description • 8 years ago

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20150124010859

Steps to reproduce:

1 install flash
2 enable or disable HA（try both two sets,）
3 open  http://wenku.baidu.com/view/787a850e336c1eb91a375d50.html 
see the memory usage


Actual results:

memory usage grows to 1gb


Expected results:

 memory usage of old versions isnt grown

	FateRover Reporter
	Comment 1 • 8 years ago

actually can reproduce without flash;
did a memory report,most were iamages

1,288.42 MB (100.0%) -- explicit
├────892.99 MB (69.31%) -- images
│    ├──891.64 MB (69.20%) -- content
│    │  ├──891.60 MB (69.20%) -- raster
│    │  │  ├──891.60 MB (69.20%) -- used
│    │  │  │  ├──413.38 MB (32.08%) -- image(893x24199, http://bcs.wenku.bdimg.com/docconvert4787-nj/%2Fwk%2Fa517f0ab71d8e88719dc471ecf24d6b3%2F0.png?sign=MBOT:y1jXjmMD4FchJHFHIGN4z:Td1ABz5Tw3QTw8YGv3pFvg15WBk%3D&time=1458105147&range=15323-37893&response-cache-control=max-age=3888000&response-expires=Sat%2C%20 (truncated))
│    │  │  │  │  ├──413.35 MB (32.08%) -- unlocked
│    │  │  │  │  │  ├───82.44 MB (06.40%) -- surface(893x24199@0)
│    │  │  │  │  │  │   ├──82.44 MB (06.40%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───44.95 MB (03.49%) -- surface(518x22747@0)
│    │  │  │  │  │  │   ├──44.95 MB (03.49%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───39.21 MB (03.04%) -- surface(518x19843@0)
│    │  │  │  │  │  │   ├──39.21 MB (03.04%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───33.95 MB (02.64%) -- surface(518x17181@0)
│    │  │  │  │  │  │   ├──33.95 MB (02.64%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───33.95 MB (02.64%) -- surface(518x17182@0)
│    │  │  │  │  │  │   ├──33.95 MB (02.64%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───32.52 MB (02.52%) -- surface(518x16456@0)
│    │  │  │  │  │  │   ├──32.52 MB (02.52%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───32.52 MB (02.52%) -- surface(518x16455@0)
│    │  │  │  │  │  │   ├──32.52 MB (02.52%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───29.17 MB (02.26%) -- surface(518x14762@0)
│    │  │  │  │  │  │   ├──29.17 MB (02.26%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───29.17 MB (02.26%) -- surface(518x14761@0)
│    │  │  │  │  │  │   ├──29.17 MB (02.26%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───27.74 MB (02.15%) -- surface(518x14036@0)
│    │  │  │  │  │  │   ├──27.74 MB (02.15%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  └───27.73 MB (02.15%) -- surface(518x14035@0)
│    │  │  │  │  │      ├──27.73 MB (02.15%) ── decoded-nonheap
│    │  │  │  │  │      └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  └────0.02 MB (00.00%) ── source
│    │  │  │  ├──351.52 MB (27.28%) -- image(893x24829, http://bcs.wenku.bdimg.com/docconvert4787-nj/%2Fwk%2Fa517f0ab71d8e88719dc471ecf24d6b3%2F0.png?sign=MBOT:y1jXjmMD4FchJHFHIGN4z:Td1ABz5Tw3QTw8YGv3pFvg15WBk%3D&time=1458105147&range=37894-60968&response-cache-control=max-age=3888000&response-expires=Sat%2C%20 (truncated))
│    │  │  │  │  ├──351.49 MB (27.28%) -- unlocked
│    │  │  │  │  │  ├───84.58 MB (06.56%) -- surface(893x24829@0)
│    │  │  │  │  │  │   ├──84.58 MB (06.56%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───40.23 MB (03.12%) -- surface(518x20360@0)
│    │  │  │  │  │  │   ├──40.23 MB (03.12%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───40.23 MB (03.12%) -- surface(518x20359@0)
│    │  │  │  │  │  │   ├──40.23 MB (03.12%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───34.84 MB (02.70%) -- surface(518x17628@0)
│    │  │  │  │  │  │   ├──34.84 MB (02.70%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───34.84 MB (02.70%) -- surface(518x17629@0)
│    │  │  │  │  │  │   ├──34.84 MB (02.70%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───29.93 MB (02.32%) -- surface(518x15145@0)
│    │  │  │  │  │  │   ├──29.93 MB (02.32%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───29.93 MB (02.32%) -- surface(518x15146@0)
│    │  │  │  │  │  │   ├──29.93 MB (02.32%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───28.46 MB (02.21%) -- surface(518x14400@0)
│    │  │  │  │  │  │   ├──28.46 MB (02.21%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  └───28.46 MB (02.21%) -- surface(518x14401@0)
│    │  │  │  │  │      ├──28.46 MB (02.21%) ── decoded-nonheap
│    │  │  │  │  │      └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  └────0.02 MB (00.00%) ── source
│    │  │  │  ├──120.88 MB (09.38%) -- image(893x16380, http://bcs.wenku.bdimg.com/docconvert4787-nj/%2Fwk%2Fa517f0ab71d8e88719dc471ecf24d6b3%2F0.png?sign=MBOT:y1jXjmMD4FchJHFHIGN4z:Td1ABz5Tw3QTw8YGv3pFvg15WBk%3D&time=1458105147&range=0-15322&response-cache-control=max-age=3888000&response-expires=Sat%2C%2030%2 (truncated))
│    │  │  │  │  ├──120.87 MB (09.38%) -- unlocked
│    │  │  │  │  │  ├───55.80 MB (04.33%) -- surface(893x16380@0)
│    │  │  │  │  │  │   ├──55.80 MB (04.33%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───27.52 MB (02.14%) -- surface(518x13923@0)
│    │  │  │  │  │  │   ├──27.52 MB (02.14%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  ├───18.78 MB (01.46%) -- surface(518x9501@0)
│    │  │  │  │  │  │   ├──18.78 MB (01.46%) ── decoded-nonheap
│    │  │  │  │  │  │   └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  │  └───18.77 MB (01.46%) -- surface(518x9500@0)
│    │  │  │  │  │      ├──18.77 MB (01.46%) ── decoded-nonheap
│    │  │  │  │  │      └───0.00 MB (00.00%) ── decoded-heap
│    │  │  │  │  └────0.02 MB (00.00%) ── source
│    │  │  │  └────5.82 MB (00.45%) ++ (49 tiny)
│    │  │  └────0.00 MB (00.00%) ── unused/<non-notable images>/source
│    │  └────0.04 MB (00.00%) ── vector/used/image(0x0, https://tiles-cloudfront.cdn.mozilla.net/images/583de2b339502a7726bc05736a43b18e9c891350.2193.svg)/source
│    └────1.34 MB (00.10%) ++ (2 tiny)
├────112.92 MB (08.76%) -- js-non-window
│    ├───64.22 MB (04.98%) -- zones
│    │   ├──49.56 MB (03.85%) -- zone(0xb23800)
│    │   │  ├──33.75 MB (02.62%) ++ (435 tiny)
│    │   │  └──15.81 MB (01.23%) ── unused-gc-things
│    │   └──14.66 MB (01.14%) ++ (9 tiny)
│    ├───24.66 MB (01.91%) ++ runtime
│    └───24.04 MB (01.87%) -- gc-heap
│        ├──21.39 MB (01.66%) ── unused-arenas
│        └───2.64 MB (00.20%) ++ (2 tiny)
├─────71.32 MB (05.54%) ── heap-unclassified
├─────70.40 MB (05.46%) -- window-objects
│     ├──37.37 MB (02.90%) -- top(http://wenku.baidu.com/view/787a850e336c1eb91a375d50.html, id=67)
│     │  ├──22.13 MB (01.72%) -- active
│     │  │  ├──19.93 MB (01.55%) ++ window(http://wenku.baidu.com/view/787a850e336c1eb91a375d50.html)
│     │  │  └───2.20 MB (00.17%) ++ (8 tiny)
│     │  └──15.23 MB (01.18%) ++ js-zone(0x1a958000)
│     ├──23.23 MB (01.80%) -- top(about:memory, id=41)/active/window(about:memory)
│     │  ├──22.13 MB (01.72%) -- dom
│     │  │  ├──21.61 MB (01.68%) ── orphan-nodes
│     │  │  └───0.52 MB (00.04%) ++ (5 tiny)
│     │  └───1.10 MB (00.09%) ++ (4 tiny)
│     └───9.80 MB (00.76%) ++ (7 tiny)
├─────48.58 MB (03.77%) -- heap-overhead
│     ├──42.72 MB (03.32%) ── bin-unused
│     └───5.86 MB (00.45%) ++ (3 tiny)
├─────41.96 MB (03.26%) ++ (19 tiny)
├─────32.71 MB (02.54%) -- add-ons
│     ├──30.41 MB (02.36%) -- {d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d}/js-non-window/zones/zone(0xb23800)
│     │  ├──30.03 MB (02.33%) -- compartment([System Principal], jar:file:///D:/FireFox/45/Profiles/Profiles/extensions/%7Bd10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d%7D.xpi!/bootstrap.js (from: resource://gre/modules/addons/XPIProvider.jsm:4515))
│     │  │  ├──29.89 MB (02.32%) -- classes
│     │  │  │  ├──20.00 MB (01.55%) ++ class(Object)
│     │  │  │  └───9.88 MB (00.77%) ++ (5 tiny)
│     │  │  └───0.14 MB (00.01%) ++ (4 tiny)
│     │  └───0.38 MB (00.03%) ++ (10 tiny)
│     └───2.30 MB (00.18%) ++ (8 tiny)
└─────17.55 MB (01.36%) -- gfx
      ├──15.09 MB (01.17%) ── heap-textures
      └───2.45 MB (00.19%) ++ (5 tiny)

	Loic
	Comment 2 • 8 years ago

It's a regression:

15:19.73 INFO: Last good revision: 5bd6e09f074e (2014-09-22)
15:19.74 INFO: First bad revision: afc933adf723 (2014-09-23)
15:19.74 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5bd6e09f074e&tocha
nge=afc933adf723

Pretty sure it regressed by Bug 1060200.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 3 • 8 years ago

I can't seem to reproduce this...the page doesn't seem to load fully.

	Loic
	Comment 4 • 8 years ago

It loads but with some time. :)
But maybe the issue is restricted to Windows.

	Daniel Holbert [:dholbert]
	Comment 5 • 8 years ago

Firstly: thanks for the bug report!

I can sort of reproduce the about:memory report in comment 1, though for me the first (most expensive) image is using 113 MB, not 413 MB, and is 893x16380 instead of 893x24199.  That might be a result of my screen resolution or some other system-dependent factor -- not sure.

Regardless, this image is huge (in terms of its dimensions), so it's not surprising that it produces a huge decoded surface.

I don't think it's fair to call this issue a "leak".  This is simply our image code using the memory that's available to it, for better performance (keeping a decoded surface around in the cache, so we don't have to re-decode it later).  I believe we do this in a way where we mark the memory as available-to-be-reclaimed-if-the-OS-needs-it or something like that, too -- I forget the details but seth could comment more.

So, I think this bug is things working as-indended; hence, resolving it as INVALID.  *If* the memory usage is still high (and this image still shows up in about:memory reports) several minutes after the tab is closed, *then* I think it'd be appropriate to consider this a "leak" & a reportable bug.  But for now, I think this is

	Daniel Holbert [:dholbert]
	Comment 6 • 8 years ago

[didn't finish my last sentence there, sorry]
...I think this is things working as-intended.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 7 • 8 years ago

From the memory report in comment 1, one image has many, many surfaces. We don't want to have that many surfaces for one image unless there is a good reason.

	Daniel Holbert [:dholbert]
	Comment 8 • 8 years ago

Ah, didn't notice that -- thanks.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 9 • 8 years ago

If anyone can produce a mirror or local archive of the site that reproduces the problem that would be great because I am having a very hard time reproducing. The site is not very reliable. Trying to download the same image by itself in a separate tab will work once, and then time out on many attempts over an hour.

	FateRover Reporter
	Comment 10 • 8 years ago

local archive

	FateRover Reporter
	Comment 11 • 8 years ago

  may associate with https://bugzilla.mozilla.org/show_bug.cgi?id=1123976

	Timothy Nikkel (:tnikkel) Assignee
	Comment 12 • 8 years ago

Thank you, that helps.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 13 • 8 years ago

The page is hard to debug, but it looks like there is one large (800 x 16000) image that is used as a CSS sprite map. I'm guessing the image is designed for retina screens, and then dynamically the elements using it are scaled down. The sizes are the sprites are quite small (around 20 pixels I think), so I'm guessing pulling a 20 pixel rect from a 16000 image can introduce small differences in the overall requested frame size of the image (I saw 9500, 9501, 9992).

As for what to do about this, maybe we should cap the total memory use by one image.

	FateRover Reporter
	Comment 14 • 8 years ago

(In reply to Timothy Nikkel (:tnikkel) from comment #13)
> The page is hard to debug, but it looks like there is one large (800 x
> 16000) image that is used as a CSS sprite map. I'm guessing the image is
> designed for retina screens, and then dynamically the elements using it are
> scaled down. The sizes are the sprites are quite small (around 20 pixels I
> think), so I'm guessing pulling a 20 pixel rect from a 16000 image can
> introduce small differences in the overall requested frame size of the image
> (I saw 9500, 9501, 9992).
> 
> As for what to do about this, maybe we should cap the total memory use by
> one image.

actually the test file isnt load fully，there still another 11 pages to load by click ，i can only save thefirst page，you can scroll the page to simulate the last 11

	FateRover Reporter
	Comment 15 • 8 years ago

(In reply to Loic from comment #2)
> It's a regression:
> 
> 15:19.73 INFO: Last good revision: 5bd6e09f074e (2014-09-22)
> 15:19.74 INFO: First bad revision: afc933adf723 (2014-09-23)
> 15:19.74 INFO: Pushlog:
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=5bd6e09f074e&tocha
> nge=afc933adf723
> 
> Pretty sure it regressed by Bug 1060200.

the link https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5bd6e09f074e&tocha  make ff & IE stunk，lol

	FateRover Reporter
	Comment 16 • 8 years ago

any progress？

	Timothy Nikkel (:tnikkel) Assignee
	Comment 17 • 7 years ago

(In reply to Timothy Nikkel (:tnikkel) from comment #13)
> As for what to do about this, maybe we should cap the total memory use by
> one image.

A better idea would be to consider both the size of the decoded frames for the image and the size difference between the closest size already decoded and the new size being asked for. Something simple like "if the decoded image size is a few MBs and the requested size differs by less than 10% from an existing size just use the existing size". This adds some complexity as we'd be returning the WRONG_SIZE DrawResult and hence we'd be changing the meaning of WRONG_SIZE (it currently means that calling again and asking for syncdecode would result in a better frame, which would be false with this proposed change), or we'd need to detect this case and change the DrawResult to SUCCESS.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 19 • 6 years ago

Not sure if we should land this, but I did it anyway to make sure I had everything covered.

	Andrew Osmond [:aosmond] (he/him)
	Comment 23 • 6 years ago

Comment on attachment 9008665 [details] [diff] [review]
only mark images used if we use them

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

I realize I wasn't added to review this part, but I have a question for FrameAnimator::RequestRefresh:

::: image/FrameAnimator.cpp
@@ +427,5 @@
>      SurfaceCache::Lookup(ImageKey(mImage),
>                           RasterSurfaceKey(mSize,
>                                            DefaultSurfaceFlags(),
> +                                          PlaybackType::eAnimated),
> +                         /* aMarkUsed = */ false);

Is the assumption here that if the image is displayed, it will be locked, and thus cannot be evicted? RequestRefresh is otherwise the only thing that we know will happen that will mark it as used (because it could keep getting a composited frame, and be a very long animation so it takes a long time to loop back to the start.)

	Timothy Nikkel (:tnikkel) Assignee
	Comment 24 • 6 years ago

(In reply to Andrew Osmond [:aosmond] from comment #23)
> Comment on attachment 9008665 [details] [diff] [review]
> only mark images used if we use them
> 
> Review of attachment 9008665 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I realize I wasn't added to review this part, but I have a question for
> FrameAnimator::RequestRefresh:

That was a mistake, I meant to ask review.

> ::: image/FrameAnimator.cpp
> @@ +427,5 @@
> >      SurfaceCache::Lookup(ImageKey(mImage),
> >                           RasterSurfaceKey(mSize,
> >                                            DefaultSurfaceFlags(),
> > +                                          PlaybackType::eAnimated),
> > +                         /* aMarkUsed = */ false);
> 
> Is the assumption here that if the image is displayed, it will be locked,
> and thus cannot be evicted? RequestRefresh is otherwise the only thing that
> we know will happen that will mark it as used (because it could keep getting
> a composited frame, and be a very long animation so it takes a long time to
> loop back to the start.)

Ah, good catch. I was just setting aMarkUsed to false if the caller didn't actually use the frame data. But I guess AdvanceFrame (if it gets called after this) would use the frame data.

On the other hand, if we are calling RequestRefresh on an image that isn't displayed (if it was displayed it should be locked) then we would want to let the surface expire and get discarded.

I don't feel strongly about either way.

	Andrew Osmond [:aosmond] (he/him)
	Comment 25 • 6 years ago

Comment on attachment 9008665 [details] [diff] [review]
only mark images used if we use them

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

r+; only concern was what I commented in RequestRefresh, which I think it should mark it as used unless we are certain it will be locked in all cases.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 26 • 6 years ago

(In reply to Andrew Osmond [:aosmond] from comment #25)
> Comment on attachment 9008665 [details] [diff] [review]
> only mark images used if we use them
> 
> Review of attachment 9008665 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+; only concern was what I commented in RequestRefresh, which I think it
> should mark it as used unless we are certain it will be locked in all cases.

I just changed it to mark used. We can revisit it later if it ends up mattering.

	Pulsebot
	Comment 27 • 6 years ago

Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8421b16c011b
Only mark images as used in the surface cache if we actually use them. r=aosmond
https://hg.mozilla.org/integration/mozilla-inbound/rev/479653ce6d8c
Make aMarkUsed parameters required everywhere. r=aosmond
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8bb90ade0d6
Fix comment for imgIContainer::RequestDecodeForSize to match reality. r=aosmond
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cc5b3f51ec4
Fix comment in ImageLoader. r=aosmond

	Andrei Ciure[:aciure]
	Comment 28 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/8421b16c011b
https://hg.mozilla.org/mozilla-central/rev/479653ce6d8c
https://hg.mozilla.org/mozilla-central/rev/d8bb90ade0d6
https://hg.mozilla.org/mozilla-central/rev/2cc5b3f51ec4

	Pulsebot
	Comment 29 • 6 years ago

Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec0b10e9a616
Change MultipartImage to use RequestDecode instead of GetFrame to avoid marking the frame as used. r=aosmond

	Cristian Brindusan [:cbrindusan]
	Comment 30 • 6 years ago
bugherder

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

	Ionuț Goldan [:igoldan]
	Comment 31 • 6 years ago

Noticed some big perf improvements:

== Change summary for alert #16781 (as of Sat, 13 Oct 2018 02:31:39 GMT) ==

Improvements:

  6%  Images windows7-32 opt stylo                             5,773,777.05 -> 5,420,867.44
  6%  Images linux64 opt stylo                                 5,919,683.15 -> 5,559,192.11
  6%  Images linux64-stylo-sequential opt stylo-sequential     5,872,879.65 -> 5,528,974.58
  4%  Images windows10-64 opt stylo                            7,029,619.06 -> 6,733,006.45
  4%  Images windows10-64-qr opt stylo                         6,897,424.11 -> 6,651,303.82

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=16781