Closed Bug 1259785 Opened 4 years ago Closed 4 years ago

memory leak in canvas draw operations (regression)

Categories

(Core :: Graphics, defect)

45 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox45 --- wontfix
firefox46 + wontfix
firefox47 + fixed
firefox48 + fixed
firefox-esr45 - wontfix

People

(Reporter: szdy12, Assigned: bas.schouten)

References

Details

(Keywords: regression, testcase, Whiteboard: [MemShrink])

Attachments

(3 files)

Attached file oom.html
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160322075646

Steps to reproduce:

Open the attached html file.
Click on "Start" to start a canvas animation.
Watch memory usage in Task Manager: Firefox will be using more and more memory over time.


Actual results:

Firefox is using more and more memory during canvas animation.


Expected results:

Memory usage should be constant since there are no new memory allocations in the animation loop.

This is a regression issue starting from Firefox 45. Firefox 46.0b4 is still leaking memory.
Component: Untriaged → Canvas: 2D
Product: Firefox → Core
Attached file memory-report.json.gz
Can reproduce it on Win10x64 with 64bit Nightly. My STR is slightly differen :

1. Open the attachement
2. Click on start
3. Repeatedly rapidly click on "Single Step" for 10-20 seconds.

The content-process gets 100% CPU use, and memory keeps on increasing.
Regression range: 
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b4c27a
b3cafeef188e84a704cbe72f27419a1eea&tochange=e4944eb37eecb1587c5acec96a14bd7f7d6f
de57

Bas Schouten — Bug 1221616: Use ID2D1CommandList instead of a bitmap for temporary D2D drawing. r=jrmuizel

Bas, could you check why your patch is leaking memory when drawing on canvas?
Blocks: 1221616
Status: UNCONFIRMED → NEW
Component: Canvas: 2D → Graphics
Ever confirmed: true
Flags: needinfo?(bas)
Keywords: regression, testcase
Whiteboard: [MemShrink]
(In reply to Loic from comment #2)
> Regression range: 
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=b4c27a
> b3cafeef188e84a704cbe72f27419a1eea&tochange=e4944eb37eecb1587c5acec96a14bd7f7
> d6f
> de57
> 
> Bas Schouten — Bug 1221616: Use ID2D1CommandList instead of a bitmap for
> temporary D2D drawing. r=jrmuizel
> 
> Bas, could you check why your patch is leaking memory when drawing on canvas?

It's not actually leaking, it's just.. leaving it around longer than it should, when you delete the canvas it will go away. Can you verify this issue isn't occurring on trunk? I believe this should be fixed on all branches.
Flags: needinfo?(bas)
The default behaviour is bad, memory use grows up indefinitely and disappears only if the user closes the tab. I tried with Nightly and in 1-2 min, the memory of the plugin-container (e10s was on) grew up to 500 MB before closing the tab.
OK, let's sort out if this is a problem in Nightly and any other branch.  If this is going to increase the number of OOMs, we probably can't afford it.
Assignee: nobody → bas
It would also be useful to know if this happens with/without e10s enabled.
Attachment #8736893 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8736893 [details]
MozReview Request: Bug 1259785: Do a proper flush when taking a snapshot so our dependent targets and command lists get appropriately cleared. r=jrmuizel

https://reviewboard.mozilla.org/r/43581/#review40143
https://hg.mozilla.org/mozilla-central/rev/db49ab7e9e4f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
It would be nice to have this fix released at least in FF47. Each day, many of our users are getting OOM errors. It'll take 3 months until FF48 is out, we'll have to force them to switch to another browser meanwhile.
Bas, is this safe to uplift?
Flags: needinfo?(bas)
(In reply to Daniel Szabo from comment #11)
> It would be nice to have this fix released at least in FF47. Each day, many
> of our users are getting OOM errors. It'll take 3 months until FF48 is out,
> we'll have to force them to switch to another browser meanwhile.

It's -incredibly- unlikely that this problem is responsible for any of there issues, it requires a very specific set of circumstances. But yes, this patch is perfectly safe to uplift.
Flags: needinfo?(bas)
Comment on attachment 8736893 [details]
MozReview Request: Bug 1259785: Do a proper flush when taking a snapshot so our dependent targets and command lists get appropriately cleared. r=jrmuizel

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Canvas is using more memory on Windows with D2D. Nothing critical, but the patch is simple and safe to uplift.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Low rist, simple patch.
[String/UUID change made/needed]: None.


I'm somewhat hesitant to uplift all the way to Beta since it's not critical, but since this is a simple patch we could have it at least on aurora.
Attachment #8736893 - Flags: approval-mozilla-beta?
Attachment #8736893 - Flags: approval-mozilla-aurora?
(In reply to Bas Schouten (:bas.schouten) from comment #13)
> (In reply to Daniel Szabo from comment #11)
> > It would be nice to have this fix released at least in FF47. Each day, many
> > of our users are getting OOM errors. It'll take 3 months until FF48 is out,
> > we'll have to force them to switch to another browser meanwhile.
> 
> It's -incredibly- unlikely that this problem is responsible for any of there
> issues, it requires a very specific set of circumstances. But yes, this
> patch is perfectly safe to uplift.

The testcase was isolated from an online game. Users having OOMs all have FF45, no OOM on Chrome/IE or pre FF45.
On my PC, using FF46b5 memory footprint is growing at a rate of 100MB/25secs while running the testcase.
(In reply to Daniel Szabo from comment #15)
> The testcase was isolated from an online game. Users having OOMs all have
> FF45, no OOM on Chrome/IE or pre FF45.
> On my PC, using FF46b5 memory footprint is growing at a rate of 100MB/25secs
> while running the testcase.

This might be something else that needs to be fixed, then. Can you run this test case on the latest nightly and let us know if the problem persists?
Flags: needinfo?(szdy12)
(In reply to Nicolas Silva [:nical] from comment #16)
> (In reply to Daniel Szabo from comment #15)
> > The testcase was isolated from an online game. Users having OOMs all have
> > FF45, no OOM on Chrome/IE or pre FF45.
> > On my PC, using FF46b5 memory footprint is growing at a rate of 100MB/25secs
> > while running the testcase.
> 
> This might be something else that needs to be fixed, then. Can you run this
> test case on the latest nightly and let us know if the problem persists?

Latest nightly works fine, memory usage stays constant.
Flags: needinfo?(szdy12)
Comment on attachment 8736893 [details]
MozReview Request: Bug 1259785: Do a proper flush when taking a snapshot so our dependent targets and command lists get appropriately cleared. r=jrmuizel

May help with OOM issues, ok to uplift to aurora.
We are heading into late beta so it seems best we keep this to ship with 47.
Attachment #8736893 - Flags: approval-mozilla-beta?
Attachment #8736893 - Flags: approval-mozilla-beta-
Attachment #8736893 - Flags: approval-mozilla-aurora?
Attachment #8736893 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.