887791 - Higher memory consumption on Mac 10.7.5 and 10.8.4 since June the 22nd

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[Mihaela Velimiroviciu (:mihaelav)]

Mac OS X (10.7.5 and 10.8.4) results show higher memory consumption on Nightly branch since June the 22nd: ~20MB for resident memory and ~10MB (Mac 10.8.4) and ~15MB (Mac 10.7.5) for explicit memory. The regression appeared between builds 20130621031051 (http://hg.mozilla.org/mozilla-central/rev/7ba8c86f1a56) and 20130622031042 (http://hg.mozilla.org/mozilla-central/rev/cea75ce9a559).

The tests that regressed most are Flash related: 
/testFlash_SWFVideoEmbed/test1.js - testFlashViaEmbedTag 	
/testFlash_SWFVideoIframe/test1.js - testFlashIframe 	
/testFlash_SWFVideoObject/test1.js  - testFlashObject 	
/testFlash_SWFVideoURL/test1.js - testFlashURL 	

Reports:
* Mac 10.7.5 21June: http://mozmill-ci.blargon7.com/#/endurance/report/91511d5536e962e0de7f57ed1e214e4c
* Mac 10.8.4 21June: http://mozmill-ci.blargon7.com/#/endurance/report/91511d5536e962e0de7f57ed1e205e8c
* Mac 10.7.5 22June: http://mozmill-ci.blargon7.com/#/endurance/report/91511d5536e962e0de7f57ed1e3fbfe7
* Mac 10.8.4 22June: http://mozmill-ci.blargon7.com/#/endurance/report/91511d5536e962e0de7f57ed1e3fa4e5

Charts:
http://mozmill-ci.blargon7.com/#/endurance/charts?branch=24.0&platform=Mac&from=2013-06-20&to=2013-06-27

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7ba8c86f1a56&tochange=cea75ce9a559

Note: The regression is reproducible locally (tested on Mac 10.7.5) Reports:
* June 21st build: http://mozmill-crowd.blargon7.com/#/endurance/report/91511d5536e962e0de7f57ed1ee66826
* June 22nd build: http://mozmill-crowd.blargon7.com/#/endurance/report/91511d5536e962e0de7f57ed1ed38cc2

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Thanks Mihaela, this looks like a genuine regression! Please raise a bug against Firefox, and set this as a blocking bug. Then, try to reduce the regression range by testing against tinderbox builds. You can find details of how to do this here: https://quality.mozilla.org/docs/mozmill/investigating-memory-regressions/

Comment 2

[u279076]

> http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7ba8c86f1a56&tochange=cea75ce9a559
The following bugs might be good candidates for introducing this regression:
* Bug 883333 seems to be a rather large refactor
* Bug 885158 is related to IPC channels

Comment 3

[u279076]

That said the pushlog contains well over 100 changes so it'd be advisable to reduce the regression range further as Dave has suggested.

Comment 4

[Mihaela Velimiroviciu (:mihaelav)]

The first bad revision is:
changeset:   135929:03fcdac57879
parent:      135928:03a709825eff
parent:      135870:7ba8c86f1a56
user:        Ryan VanderMeulen <[e-mail]>
date:        Thu Jun 20 20:12:31 2013 -0400
summary:     Merge m-c to inbound.

I'm currently bisecting and building locally in order to narrow down the regression

Comment 5

[Mihai Morar, (:MihaiMorar)]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #3)
> That said the pushlog contains well over 100 changes so it'd be advisable to
> reduce the regression range further as Dave has suggested.

Mihaela is on PTO, and for now I took responsibility for endurance triage and investigations.

I reduced those 100 changesets to about 10 between last good and first bad and I hope until tomorrow I'll find the real causing changeset for this issue. 

Sorry I'm not able to bisect faster but it takes about 3h for an endurance testrun.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Can we narrow that down to a specific test or is the memory constantly growing through the whole testrun? If only some tests can cause this behavior you might want to trim the endurance testrun.

Comment 7

[Mihai Morar, (:MihaiMorar)]

Thanks Henrik, I had created a new testrun with those 4 tests that are causing this behavior, mentioned in Comment 0.

Here is the real causing changeset for this issue:

author	Ehsan Akhgari <[e-mail]>
Sun Jun 23 20:58:53 2013 -0400 (at Sun Jun 23 20:58:53 2013 -0400)
changeset 136213	76820c6dff7b
parent 136212	9b5816c7fa6e
child 136214	9661a68f0bff
pushlog:	76820c6dff7b

First bad:
http://mozmill-crowd.blargon7.com/#/endurance/report/180cf2548ef2865af3ae441d61be25f6

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I don't see how http://hg.mozilla.org/mozilla-central/rev/76820c6dff7b could be the cause for this issue. Can you please check that again with a full endurance testrun?

Comment 9

[Mihai Morar, (:MihaiMorar)]

Checked as requested in Comment 8:

Last Good:
author	Ryan VanderMeulen <[e-mail]>
Fri Jun 14 22:23:21 2013 -0400 (at Fri Jun 14 22:23:21 2013 -0400)
changeset 135149	3d16d59c9317
parent 135148	05d9196b27a1
child 135150	7f8de495bf6d
pushlog:	3d16d59c9317

http://mozmill-crowd.blargon7.com/#/endurance/report/180cf2548ef2865af3ae441d61cb67d8

First bad:
author	Ehsan Akhgari <[e-mail]>
Sun Jun 23 20:58:53 2013 -0400 (at Sun Jun 23 20:58:53 2013 -0400)
changeset 136213	76820c6dff7b
parent 136212	9b5816c7fa6e
child 136214	9661a68f0bff
pushlog:	76820c6dff7b

http://mozmill-crowd.blargon7.com/#/endurance/report/180cf2548ef2865af3ae441d61d40bc6

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ehsan, could you imagine what could be the cause of this memory increase for our Flash tests? Could this really be the landing of one of the patches on bug 579517?

Comment 11

[Dave Hunt [:davehunt] [he/him] ⌚BST]

These changesets are eight days apart, and there are many changes between them: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3d16d59c9317&tochange=76820c6dff7b

We already established a much smaller regression range in comment 2, so please try to narrow this down further.

Comment 12

[Dave Hunt [:davehunt] [he/him] ⌚BST]

After discussion on IRC we established the regression showed up between these tinderbox builds:

last good: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-macosx64/1372029844/

first bad: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-macosx64/1372053243/

This makes the relevant pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=aef71cf41cec&tochange=76820c6dff7b

Please continue by bisecting and building Firefox. I'm cancelling the needinfo for now.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

But even that doesn't match what we got in comment 2, and has a time drift of nearly 2 days. So I doubt, this is correct.

Comment 14

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Henrik Skupin (:whimboo) from comment #13)
> But even that doesn't match what we got in comment 2, and has a time drift
> of nearly 2 days. So I doubt, this is correct.

Good point, I missed that.

Comment 15

[Mihai Morar, (:MihaiMorar)]

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=aef71cf41cec&tochange=76820c6dff7b

All changesets are good in the latest investigations that I had done.

Here are the results:

http://mozmill-crowd.blargon7.com/#/endurance/reports?branch=24.0&platform=Mac&from=2013-07-26&to=2013-07-26

I'll continue next week to bisect following pushlog from Comment 2. Dave could this be a better solution?

Comment 16

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Mihai Morar, QA (:MihaiMorar) from comment #15)
> I'll continue next week to bisect following pushlog from Comment 2. Dave
> could this be a better solution?

Yes, in fact you should have done this instead. As Henrik pointed out in comment 13 there's doubt that the regression range you had provided was accurate as it disagreed with the initial nightly regression range. Please continue by bisecting the range from comment 2.

Comment 17

[Tracy Walker [:tracy]]

Just to to rule it out; was anything done to the mozmill test farm that may have caused this?

Comment 18

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Tracy Walker [:tracy] from comment #17)
> Just to to rule it out; was anything done to the mozmill test farm that may
> have caused this?

None that I am aware of. Mihai: have you made any further progress here? It's important to move quickly on potential memory regressions. This was reported almost two months ago now.

Comment 19

[Tracy Walker [:tracy]]

benjamin this is the bug I mentioned in the stability meeting.

Comment 20

[Ioana (away)]

Mihai's on PTO now, so I investigated this further. I tested on Mac OSX 10.7.5 with the whole endurance suite.

When running the endurance suite with a Firefox 26 nightly, I reproduce this bug. When running it with the 06/22 Firefox 24 nightly or any build built after a bisection of the comment 2 pushlog, it doesn't reproduce.


Here are the results of all my test runs:
http://mozmill-crowd.blargon7.com/#/endurance/reports?branch=All&platform=All&from=2013-08-12&to=2013-08-20

Any ideas of what's happening here?

Comment 21

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Please can you provide the results of your bisect? It was confirmed previously that the regression range is 7ba8c86f1a56 to cea75ce9a559, therefore it should just be a case of performing a bisect/build to determine which commit introduced the regression. It would be worth also building the above mentioned revisions to confirm that a local build can replicate the regression.

Comment 22

[Ioana (away)]

(In reply to Dave Hunt (:davehunt) from comment #21)
> Please can you provide the results of your bisect? It was confirmed
> previously that the regression range is 7ba8c86f1a56 to cea75ce9a559,
> therefore it should just be a case of performing a bisect/build to determine
> which commit introduced the regression. It would be worth also building the
> above mentioned revisions to confirm that a local build can replicate the
> regression.

The results of my testing were posted in my above comment, but I will detail them here if that's what you need.

I run the endurance test suite on a Firefox 26 build (latest nightly at that point) to make sure I reproduce the bug. It reproduced: http://mozmill-crowd.blargon7.com/#/endurance/report/b7ef1fb3d9703aeaf2c46e07d2b18652

Bisected the pushlog in comment 2, built Firefox, and tested the following commits. None reproduced this bug:
2b0640fa5b18 - http://mozmill-crowd.blargon7.com/#/endurance/report/b7ef1fb3d9703aeaf2c46e07d27b97b6
918fd1e35b39 - http://mozmill-crowd.blargon7.com/#/endurance/report/b7ef1fb3d9703aeaf2c46e07d288a733
a0b216d3be12 - http://mozmill-crowd.blargon7.com/#/endurance/report/b7ef1fb3d9703aeaf2c46e07d2abfea1
8ce7857353a6 - http://mozmill-crowd.blargon7.com/#/endurance/report/b7ef1fb3d9703aeaf2c46e07d2bb9495
0c5c910798b7 - http://mozmill-crowd.blargon7.com/#/endurance/report/b7ef1fb3d9703aeaf2c46e07d2c1780a
43c6fe6a9301 - http://mozmill-crowd.blargon7.com/#/endurance/report/b7ef1fb3d9703aeaf2c46e07d2f81226
25fba7984dc9 - http://mozmill-crowd.blargon7.com/#/endurance/report/b7ef1fb3d9703aeaf2c46e07d2fe1bb0
19a51caf7582 - http://mozmill-crowd.blargon7.com/#/endurance/report/e503b7b0a70a3839c66c64572e0adec9
c082decf4957 - http://mozmill-crowd.blargon7.com/#/endurance/report/e503b7b0a70a3839c66c64572e242221
808302fb793e - http://mozmill-crowd.blargon7.com/#/endurance/report/e503b7b0a70a3839c66c64572e6b6ffe
deb4d87d1684 - http://mozmill-crowd.blargon7.com/#/endurance/report/e503b7b0a70a3839c66c64572e6eee22

cea75ce9a559 - http://mozmill-crowd.blargon7.com/#/endurance/report/e503b7b0a70a3839c66c64572e7b7b65

Comment 23

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Thanks for the detail Ioana, this is indeed much more useful. This implies either:

1. Your local build differs from the nightlies produced by release engineering
2. The original regression range in incorrect
3. You are unable to reproduce the original issue and are seeing a different regression in latest nightlies

You should be able to find out which (if any) of these are true. For item 1 you can directly compare a locally built Firefox with a nightly built from the same changeset. For items 2 and 3 you should be able to determine a regression range based on the regression you are able to reproduce.

Also, looking at your bisect, nothing before changeset 2b0640fa5b18 was tested. You probably need to at least compare 7ba8c86f1a56 with cea75ce9a559 to see if there is a memory regression, otherwise how are you determining a good/bad build?

Comment 24

[Ioana (away)]

(In reply to Dave Hunt (:davehunt) from comment #23)
> Thanks for the detail Ioana, this is indeed much more useful. This implies
> either:
> 
> 1. Your local build differs from the nightlies produced by release
> engineering
> 2. The original regression range in incorrect
> 3. You are unable to reproduce the original issue and are seeing a different
> regression in latest nightlies
> 
> You should be able to find out which (if any) of these are true. For item 1
> you can directly compare a locally built Firefox with a nightly built from
> the same changeset. For items 2 and 3 you should be able to determine a
> regression range based on the regression you are able to reproduce.
> 
> Also, looking at your bisect, nothing before changeset 2b0640fa5b18 was
> tested. You probably need to at least compare 7ba8c86f1a56 with cea75ce9a559
> to see if there is a memory regression, otherwise how are you determining a
> good/bad build?

I already ran the test suite on the cea75ce9a559 Nightly, and I get the same results as for the one built locally.

I determined if good/bad by the memory consumption. On Fx 26 I got about the same as comment 0 specifies for the buggy build. On the other builds I got about the same that was specified for the build without the bug. ("about" means a difference of at most 2-3mb)

I am currently working on determining if I'm in case 2 or 3 and will post the results here as soon as I have them.

Comment 25

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Unfortunately you can't compare your results directly with results gathered on another machine, as they will be dependent on many things (not least the amount of memory present/available). The best way to determine if you can replicate the regression is to compare against your own results.

Comment 26

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Any updates on this? It's been over a week. We really need to react faster to memory regressions.

Comment 27

[Mihai Morar, (:MihaiMorar)]

We are tracking endurance results here:

https://wiki.mozilla.org/Endurace_WorkLog#Endurance_Results_Triage_Log

As the charts show, memory usage on Mac OS got decreased since 2013-08-29 and it is still decreasing.

Comment 28

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Thanks Mihai, but that does not get us any closer to understanding the regression that this bug is regarding. The recent memory improvements could be unrelated.

Comment 29

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I have replicated the regression using the two original nightly builds it was reported for with a single iteration. The regression was much less, but we could try running with more entities or less tests with more iterations to see if we can make it more pronounced for confident regression hunting. My results are summarised below:

Build: 20130621031051
Avg resident: 75MB
Avg explicit: 139MB
http://mozmill-crowd.blargon7.com/#/endurance/report/3c3cd991de01b704f3f65783a1d95440

Build: 20130622031042
Avg resident: 76MB
Avg explicit: 142MB
http://mozmill-crowd.blargon7.com/#/endurance/report/3c3cd991de01b704f3f65783a1d9cbc3

Looking at these results, the flash tests are the ones that appear to have increased the most.

Comment 30

[Ioana (away)]

(In reply to Dave Hunt (:davehunt) from comment #25)
> Unfortunately you can't compare your results directly with results gathered
> on another machine, as they will be dependent on many things (not least the
> amount of memory present/available). The best way to determine if you can
> replicate the regression is to compare against your own results.

I got the same results for the 06/21 and the 06/22 builds, so I couldn't replicate the regression. What I found was indeed another bug. After investigating further, I got:

Last good: http://hg.mozilla.org/mozilla-central/rev/a81349b61b4e
First bad: http://hg.mozilla.org/mozilla-central/rev/d4f1b39a8575

testFlashIframe used 17mb more resident memory, and testFlashObject 8mb more, with the first bad build. The differences for resident memory for the other tests go around 1-3mb. All tests used more explicit memory than with the last good build:
http://mozmill-crowd.blargon7.com/#/endurance/report/3c3cd991de01b704f3f65783a1d848b5
http://mozmill-crowd.blargon7.com/#/endurance/report/3c3cd991de01b704f3f65783a1e92b6d

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Ioana Budnar, QA [:ioana] from comment #30)
> Last good: http://hg.mozilla.org/mozilla-central/rev/a81349b61b4e
> First bad: http://hg.mozilla.org/mozilla-central/rev/d4f1b39a8575
> 
> testFlashIframe used 17mb more resident memory, and testFlashObject 8mb
> more, with the first bad build. The differences for resident memory for the
> other tests go around 1-3mb. All tests used more explicit memory than with

This might be unrelated to this bug and also an expected change given that the memory reporters have been changed for explicit memory. Nicholas might know more about it.

Otherwise I would also really appreciate that we can get the real issue sorted out here. It has been taken way too long so far.

Comment 32

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Ioana Budnar, QA [:ioana] from comment #30)
> testFlashIframe used 17mb more resident memory, and testFlashObject 8mb
> more, with the first bad build. The differences for resident memory for the
> other tests go around 1-3mb. All tests used more explicit memory than with
> the last good build:
> http://mozmill-crowd.blargon7.com/#/endurance/report/
> 3c3cd991de01b704f3f65783a1d848b5
> http://mozmill-crowd.blargon7.com/#/endurance/report/
> 3c3cd991de01b704f3f65783a1e92b6d

This is a different regression and should therefore be investigated in a separate bug. FWIW it's also likely to be the regression shown in our dashboard at: http://mozmill-daily.blargon7.com/#/endurance/charts?branch=25.0&platform=Mac&from=2013-07-29&to=2013-08-01

Mihai: Have you made any further progress here?

Comment 33

[Ioana (away)]

(In reply to Dave Hunt (:davehunt) from comment #32)
> This is a different regression and should therefore be investigated in a
> separate bug. FWIW it's also likely to be the regression shown in our
> dashboard at:
> http://mozmill-daily.blargon7.com/#/endurance/charts?branch=25.
> 0&platform=Mac&from=2013-07-29&to=2013-08-01

Now tracked in bug 912462.

Comment 34

[Mihai Morar, (:MihaiMorar)]

I am trying for 2 days to build firefox but always get same errors:

1)
 5:28.01   File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/distutils/sysconfig.py", line 407, in _init_posix
 5:28.01     raise DistutilsPlatformError(my_msg)
 5:28.01 distutils.errors.DistutilsPlatformError: $MACOSX_DEPLOYMENT_TARGET mismatch: now "10.6" but "10.7" during configure
 5:28.02 make[7]: *** [libs-xpcshell-tests] Error 1
 5:28.02 make[6]: *** [libs] Error 2
 5:28.02 make[5]: *** [libs] Error 2
 5:28.02 make[4]: *** [libs_tier_platform] Error 2
 5:28.02 make[3]: *** [tier_platform] Error 2
 5:28.02 make[2]: *** [default] Error 2
 5:28.04 make[1]: *** [realbuild] Error 2
 5:28.04 make: *** [build] Error 2
 5:28.08 34 compiler warnings present.
(mozmill-env)sl77-213:mozilla-central svuser$ 

See: https://bugzilla.mozilla.org/show_bug.cgi?id=659881

2)
(mozmill-env)sl77-213:mozilla-central-d8cce0fb9d9a svuser$ ~/mozmill-automation/testrun_endurance.py  --iterations=1 --entities=10 --report=http://mozmill-crowd.blargon7.com/db obj-x86_64-apple-darwin11.4.2/dist/Nightly.app
*** Platform: Mac OS X 10.7.5 64bit
*** Cloning repository to '/var/folders/ms/_pqw2rz91yn9lqh4yyj1xg7m0000gn/T/tmp0DS3qD.mozmill-tests'
requesting all changes
adding changesets
adding manifests
adding file changes
added 3386 changesets with 9970 changes to 780 files (+4 heads)
updating to branch default
412 files updated, 0 files merged, 0 files removed, 0 files unresolved
*** Application: Firefox 24.0a1
No option 'SourceRepository' in section: 'App'
*** Removing repository '/var/folders/ms/_pqw2rz91yn9lqh4yyj1xg7m0000gn/T/tmp0DS3qD.mozmill-tests'
Traceback (most recent call last):
  File "/Users/svuser/mozmill-automation/testrun_endurance.py", line 52, in <module>
    main()
  File "/Users/svuser/mozmill-automation/testrun_endurance.py", line 47, in main
    EnduranceTestRun().run()
  File "/Users/svuser/mozmill-automation/libs/testrun.py", line 439, in run
    raise self.last_exception
ConfigParser.NoOptionError: No option 'SourceRepository' in section: 'App'

I have reinstalled the entire OS and the environments and tools required, followed all steps from https://developer.mozilla.org/en-US/docs/Simple_Firefox_build but not able to build the requested changesets. Something had happened with the machine since I get back from PTO because I was not able to build any changesets since that.

Comment 35

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Mihai: Please raise these issues separately and keep this bug only for progress on the investigation of the memory regression.

Comment 36

[Mihai Morar, (:MihaiMorar)]

(In reply to Dave Hunt (:davehunt) from comment #32)
> Mihai: Have you made any further progress here?

I reduced the regression to about 15changesets. I have used 5 iterations and 10 entities because using only 1 iteration, I was not able to say if a build is good or bad due to the small memory differences. 

Last Good Changeset: 7ba8c86f1a56
Avg resident: 80MB
Avg explicit: 171MB
http://mozmill-crowd.blargon7.com/#/endurance/report/fb97b6210ae70da1b9ace67445689fca

First Bad Changeset: adfd8d9bfd0b
Avg resident: 89MB
Avg explicit: 181MB
http://mozmill-crowd.blargon7.com/#/endurance/report/fb97b6210ae70da1b9ace6744579bc5b

I will continue on Monday morning to bisect and find the real causing changeset. There are about 2-3 more changesets that have to be tested for that.

Comment 37

[Mihai Morar, (:MihaiMorar)]

Just finished the bisection. Here are my final results:

The first bad revision is:
changeset:   135929:03fcdac57879
parent:      135928:03a709825eff
parent:      135870:7ba8c86f1a56
user:        Ryan VanderMeulen <[e-mail]>
date:        Thu Jun 20 20:12:31 2013 -0400
summary:     Merge m-c to inbound.

Comment 38

[Mihai Morar, (:MihaiMorar)]

Something look weird here. If I bisect using the following regression: good 7ba8c86f1a56, bad 03fcdac57879 I get the results mentioned in Comment 37 but please take a look at the following URL which says there several changesets between my last good and first bad mentioned above.

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7ba8c86f1a56&tochange=03fcdac57879

Comment 39

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Mihai Morar, QA (:MihaiMorar) from comment #38)
> Something look weird here. If I bisect using the following regression: good
> 7ba8c86f1a56, bad 03fcdac57879 I get the results mentioned in Comment 37 but
> please take a look at the following URL which says there several changesets
> between my last good and first bad mentioned above.
> 
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=7ba8c86f1a56&tochange=03fcdac57879

Why are you using 7ba8c86f1a56 as your fromchange value? Your bisect will have identified later changesets that were also good. Do you have the full history of your bisect?

I wouldn't expect http://hg.mozilla.org/mozilla-central/rev/03fcdac57879 to cause a regression as this is a merge from mozilla-central to mozilla-inbound.

Comment 40

[Mihai Morar, (:MihaiMorar)]

(In reply to Dave Hunt (:davehunt) from comment #39)

> Why are you using 7ba8c86f1a56 as your fromchange value? 
Based on Comment 2 it looks like 7ba8c86f1a56 was last good build known.

> Do you have the full history of your bisect?
I have used following range:

-good 7ba8c86f1a56 (Nightly 2013/06/21) - http://mozmill-crowd.blargon7.com/#/endurance/report/fb97b6210ae70da1b9ace67445689fca
-bad cea75ce9a559 (Nightly 2013/06/22) - http://mozmill-crowd.blargon7.com/#/endurance/report/fb97b6210ae70da1b9ace674456796b5

Tested following changesets:

d8cce0fb9d9a - BAD http://mozmill-crowd.blargon7.com/#/endurance/report/fb97b6210ae70da1b9ace674456a6c5d

adfd8d9bfd0b - BAD http://mozmill-crowd.blargon7.com/#/endurance/report/fb97b6210ae70da1b9ace6744579bc5b

5eabc0ad7e33 - BAD http://mozmill-crowd.blargon7.com/#/endurance/report/fb97b6210ae70da1b9ace67445d73a4f

86ddaf01b346 - BAD http://mozmill-crowd.blargon7.com/#/endurance/report/fb97b6210ae70da1b9ace67445d9db92

16ddd7268abc - BAD http://mozmill-crowd.blargon7.com/#/endurance/report/fb97b6210ae70da1b9ace67445e1c5bf

03fcdac57879 - BAD http://mozmill-crowd.blargon7.com/#/endurance/report/fb97b6210ae70da1b9ace67445ea4141

After I tested changeset 03fcdac57879 I got this results:

The first bad revision is:
changeset:   135929:03fcdac57879
parent:      135928:03a709825eff
parent:      135870:7ba8c86f1a56
user:        Ryan VanderMeulen <[e-mail]>
date:        Thu Jun 20 20:12:31 2013 -0400
summary:     Merge m-c to inbound.

Comment 41

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Mihai Morar, QA (:MihaiMorar) from comment #40)
> (In reply to Dave Hunt (:davehunt) from comment #39)
> 
> > Why are you using 7ba8c86f1a56 as your fromchange value? 
> Based on Comment 2 it looks like 7ba8c86f1a56 was last good build known.

Yes, but that was before you started bisecting.

Were you using hg bisect to automatically bisect? I don't understand why it stops at 03fcdac57879 when there are still several changesets to test and none have been found to be good. Please try a new bisect between the changesets 7ba8c86f1a56 and 03fcdac57879.

Comment 42

[Mihai Morar, (:MihaiMorar)]

Attachment: Print Screen

Yes I am using Hg bisect. I almost got to an end, I'm pretty close. After receiving results mentioned in Comment 40 I continue to bisect using following range:

-good 7ba8c86f1a56 - http://mozmill-crowd.blargon7.com/#/endurance/report/fb97b6210ae70da1b9ace67445689fca
-bad 03a709825eff - http://mozmill-crowd.blargon7.com/#/endurance/report/26d5854914a68aa13b333f28157aacda

It took 3b16a9c6fb6a which was good - http://mozmill-crowd.blargon7.com/#/endurance/report/26d5854914a68aa13b333f28152f8f45 and my actual range is mentioned in the attachment.

Comment 43

[Mihai Morar, (:MihaiMorar)]

Final results:

The first bad revision is:
changeset:   135911:b029e52490da
user:        Matt Woodrow <[e-mail]>
date:        Fri Jun 21 09:32:04 2013 +1200
summary:     Bug 756601 - Enable OMTC by default on OSX 10.7 and 10.8. r=roc

last good changeset: 79d6fa05cff4 - http://mozmill-crowd.blargon7.com/#/endurance/report/26d5854914a68aa13b333f2815a1c941
first bad changeset: b029e52490da - http://mozmill-crowd.blargon7.com/#/endurance/report/dc3d4d2f66af31b9e75e291fa0152801

Comment 44

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Great, thanks Mihai!

Robert, Matt: Is the memory increase expected as a result of this patch landing?

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I would expect some memory usage regression. I don't know if this much is reasonable.

Comment 46

[Matt Woodrow (:mattwoodrow)]

Some higher memory usage is definitely expected, but I wouldn't expect Flash to cause any more than other pages.

How do I reproduce this locally?

I followed the instructions from https://developer.mozilla.org/en-US/docs/Mozmill_Tests and come up with:

mozmill -t mozmill-tests/tests/endurance/testFlash_SWFVideoEmbed/test1.js -b "/Applications/FirefoxNightly.app/" --report=http://mozmill-crowd.blargon7.com/db/

But this fails with the error: Sending results to 'http://mozmill-crowd.blargon7.com/db/' failed (This document requires the field tests_changeset).

I also tried the instructions here https://quality.mozilla.org/docs/mozmill/investigating-memory-regressions/ for running against nightly builds, but download.py no longer exists.

Comment 47

[Matt Woodrow (:mattwoodrow)]

Ok, I think I have a lead on this.

We don't do leak checking on MacIOSurface (the graphics surfaces allocated for plugins on OSX), and manually adding one shows us leaking them.

Getting the docs for running MozMill would still be useful for the future though :)

Comment 48

[Matt Woodrow (:mattwoodrow)]

So, we're taking a refcounted object (MacIOSurface), adding a ref and wrapping it in a non-refcounted one (SharedTextureHandle).

We give that to an Image, which has no way of releasing the ref. If we never share the image with the compositor, then it leaks.

If we do share it with the compositor, then we're passing the raw pointer across IPDL (so it'll never work with e10s), plus we have complicated lifetime issues since both the Image* and the compositor's TextureHost have the same SharedTextureHandle and have to agree on who releases it.

Given this, I don't think that the SharedTextureHandle abstraction is worth the cost, so I'm attaching patches to have an explicit Image/TextureClient/TextureHost for MacIOSurface.

This lets us manage the lifetime sanely (using the refcount), and also share across IPDL using the ID number (which will work cross-process).

I can no longer reproduce any leaking MacIOSurface objects with my patch queue.

Comment 49

[Matt Woodrow (:mattwoodrow)]

Attachment: Add MacIOSurfaceImage

Comment 50

[Matt Woodrow (:mattwoodrow)]

Attachment: Add support for MacIOSurfaceImage to ImageClient

Note that this is just an intermediate step that preserves existing behaviour.

Comment 51

[Matt Woodrow (:mattwoodrow)]

Attachment: Use MacIOSurfaceImage for plugins

Comment 52

[Matt Woodrow (:mattwoodrow)]

Attachment: TextureClient implementation for MacIOSurface

Comment 53

[Matt Woodrow (:mattwoodrow)]

Attachment: TextureHost implementation for MacIOSurface

Comment 54

[Matt Woodrow (:mattwoodrow)]

Attachment: Use MacIOSurfaceTextureClient for sharing MacIOSurface

Comment 55

[Matt Woodrow (:mattwoodrow)]

Attachment: Remove a bunch of now-unused SharedTextureHandle code

Comment 56

[Matt Woodrow (:mattwoodrow)]

https://tbpl.mozilla.org/?tree=Try&rev=5c0636a1431a

Comment 57

[Matt Woodrow (:mattwoodrow)]

Attachment: Add support for MacIOSurfaceImage to ImageClient v2

Decided that there's no need to have these as separate patches.

Comment 58

[Nicolas Silva [:nical]]

Comment on attachment 823776 [details] [diff] [review]
TextureClient implementation for MacIOSurface

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

::: gfx/layers/opengl/TextureClientOGL.h
@@ +87,5 @@
> +  virtual TextureClientData* DropTextureData() MOZ_OVERRIDE
> +  {
> +    mSurface = nullptr;
> +    MarkInvalid();
> +    return nullptr;

Please add a comment here explaining why it is OK to clear your reference to the data without returning a TextureClientData to be deallocated later.

This breaks the semantics of TEXTURE_DEALLOCATE_CLIENT because then we don't really deallocate on the client (unless the MacIOSurface is externally owned but in that case it would be safer to return a TextureClientData that would signal the owner when the TextureClientData is destroyed).

SharedTextureClientOGL also does that but it's not great. If we want do do that here as well there should be a comment explaining why it is safe to do that.

Comment 59

[Nicolas Silva [:nical]]

Comment on attachment 823777 [details] [diff] [review]
TextureHost implementation for MacIOSurface

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

::: gfx/layers/opengl/TextureHostOGL.h
@@ +422,5 @@
> +  virtual bool Lock() MOZ_OVERRIDE;
> +
> +  virtual gfx::SurfaceFormat GetFormat() const MOZ_OVERRIDE {
> +    MOZ_ASSERT(mTextureSource);
> +    return mTextureSource->GetFormat();

With all other TextureHosts we can call GetFormat without locking which is not the case here (mTextureSource is null until the first call to Lock()).
The PrintInfo stuff calls GetFormat without locking the TextureHost (which is not tested on tbpl but eventually someone uses it for debugging and it crashes, we already had a bug like that with GrallocTextureHostOGL).
So you should make it possible to call GetFormat() without locking the host.

Comment 60

[Nicolas Silva [:nical]]

Comment on attachment 823784 [details] [diff] [review]
Add support for MacIOSurfaceImage to ImageClient v2

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

::: gfx/layers/client/ImageClient.cpp
@@ +238,5 @@
> +      return false;
> +    }
> +
> +    GetForwarder()->UseTexture(this, mFrontBuffer);
> +#endif

Please override MacIOSurfaceImage::GetTextureClient() and MacIOSurface::AsSharedImage() instead of adding this block

Comment 61

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Matt Woodrow (:mattwoodrow) from comment #47)
> Getting the docs for running MozMill would still be useful for the future
> though :)

Thanks, I've updated the documentation at https://quality.mozilla.org/docs/mozmill/investigating-memory-regressions/

Comment 62

[Matt Woodrow (:mattwoodrow)]

Attachment: TextureClient implementation for MacIOSurface v2

Comment 63

[Matt Woodrow (:mattwoodrow)]

Attachment: TextureHost implementation for MacIOSurface v2

Comment 64

[Matt Woodrow (:mattwoodrow)]

Attachment: Implement AsSharedImage for MacIOSurfaceImage

Comment 65

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Given that this bug has now been transformed to an integration bug for the fix, Matt would you mind to move this bug into the right component and update the summary? Thanks.

Comment 66

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9ad2b09c37f
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d521539a285
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1e603203bce
https://hg.mozilla.org/integration/mozilla-inbound/rev/feb665b0ffb7
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f551ea01b0c
https://hg.mozilla.org/integration/mozilla-inbound/rev/d73c866f9c0c

Comment 67

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a9ad2b09c37f
https://hg.mozilla.org/mozilla-central/rev/2d521539a285
https://hg.mozilla.org/mozilla-central/rev/b1e603203bce
https://hg.mozilla.org/mozilla-central/rev/feb665b0ffb7
https://hg.mozilla.org/mozilla-central/rev/8f551ea01b0c
https://hg.mozilla.org/mozilla-central/rev/d73c866f9c0c

Comment 68

[Nicholas Nethercote [inactive]]

Matt, please nominate these for Aurora.  (Given the non-simple nature of the patches and the fact that we already shipped this regression in FF24, we probably shouldn't backport to Beta.)

Comment 69

[Matt Woodrow (:mattwoodrow)]

I no longer seem to be able to request approval, the flag option isn't there under patch details. Is this a recent bugzilla change?

Comment 70

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #69)
> I no longer seem to be able to request approval, the flag option isn't there
> under patch details. Is this a recent bugzilla change?

Byron, do you know?

Comment 71

[Andrew McCreight [:mccr8]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #69)
> I no longer seem to be able to request approval, the flag option isn't there
> under patch details. Is this a recent bugzilla change?

I think that's because this is filed under Mozilla QA rather than Core.  So just move it over to whatever appropriate component and you should be able to request approval.

Comment 72

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 823773 [details] [diff] [review]
Add MacIOSurfaceImage

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 756601
User impact if declined: Leaking memory when using plugins. Leak should be relative to the number of plugin instances used, not number of frames presented.
Testing completed (on m-c, etc.): Been on m-c for a few days.
Risk to taking this patch (and alternatives if risky): Big change, but no real alternative other than ignoring it.
String or IDL/UUID changes made by this patch: None.

Comment 73

[Matt Woodrow (:mattwoodrow)]

Note that the above request is for all patches in this bug.

(In reply to Andrew McCreight [:mccr8] from comment #71)
> I think that's because this is filed under Mozilla QA rather than Core.  So
> just move it over to whatever appropriate component and you should be able
> to request approval.

That worked, thanks.

Comment 74

[Dave Hunt [:davehunt] [he/him] ⌚BST]

The endurance results look much better on mozilla-central now.
20131101030205: http://mozmill-daily.blargon7.com/#/endurance/report/ad464590da4913b3dafe381be116bc70
20131103030204: http://mozmill-daily.blargon7.com/#/endurance/report/ad464590da4913b3dafe381be13aef51

Comment 75

[bhavana bajaj [:bajaj]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #72)
> Comment on attachment 823773 [details] [diff] [review]
> Add MacIOSurfaceImage
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): Bug 756601
> User impact if declined: Leaking memory when using plugins. Leak should be
> relative to the number of plugin instances used, not number of frames
> presented.
> Testing completed (on m-c, etc.): Been on m-c for a few days.
> Risk to taking this patch (and alternatives if risky): Big change, but no
> real alternative other than ignoring it.
> String or IDL/UUID changes made by this patch: None.

Would any fallout be obvious in the next couple of weeks while this bakes on aurora? Do you recommend any targeted QA testing here ? Would this be simple enough to backout if we found some serious regressions once we enter the beta cycle. Given your comment that the change is "big" want to have a few options before uplifting in case this causes some major web regressions.

Comment 76

[Matt Woodrow (:mattwoodrow)]

Fallout should be obvious, as any bugs would affect all Flash (and other plugin) content. I doubt we need an extra QA testing.

I don't imagine backing this out will be too difficult, we won't be doing much on top of it that would conflict.

Comment 77

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d23227011e29
https://hg.mozilla.org/releases/mozilla-aurora/rev/fbca562c65df
https://hg.mozilla.org/releases/mozilla-aurora/rev/d0b0cecee407
https://hg.mozilla.org/releases/mozilla-aurora/rev/149143b438d5
https://hg.mozilla.org/releases/mozilla-aurora/rev/58d7a9b25ddf
https://hg.mozilla.org/releases/mozilla-aurora/rev/1273dd1e9e92

Comment 78

[Ioana (away)]

Verified as fixed on Mac OS X 10.7, 10.8 and 10.8:
Firefox 27 - before the fix
http://mozmill-daily.blargon7.com/#/endurance/report/ad464590da4913b3dafe381be1450250
http://mozmill-daily.blargon7.com/#/endurance/report/ad464590da4913b3dafe381be144a8b8
http://mozmill-daily.blargon7.com/#/endurance/report/ad464590da4913b3dafe381be1449c6d

Firefox 27 - after the fix
http://mozmill-daily.blargon7.com/#/endurance/report/d17690a112360a2b3155acaa271deae1
http://mozmill-daily.blargon7.com/#/endurance/report/d17690a112360a2b3155acaa271dd778
http://mozmill-daily.blargon7.com/#/endurance/report/d17690a112360a2b3155acaa271dd327

Comment 79

[Ioana (away)]

Firefox 28 - before the fix
http://mozmill-daily.blargon7.com/#/endurance/report/ad464590da4913b3dafe381be116ec2f
http://mozmill-daily.blargon7.com/#/endurance/report/ad464590da4913b3dafe381be116dcfd
http://mozmill-daily.blargon7.com/#/endurance/report/ad464590da4913b3dafe381be116bc70

Firefox 28 - after the fix
http://mozmill-daily.blargon7.com/#/endurance/report/ad464590da4913b3dafe381be13afa8f
http://mozmill-daily.blargon7.com/#/endurance/report/ad464590da4913b3dafe381be13ad61f
http://mozmill-daily.blargon7.com/#/endurance/report/ad464590da4913b3dafe381be13aef51