Bug 658738 (bc-leaks)

[meta] We seem to be leaking hundreds of windows until shutdown during browser-chrome tests

RESOLVED FIXED

Status

()

Firefox
General
--
major
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: bz, Assigned: dao)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug, {meta, mlk})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2], URL)

Attachments

(7 attachments, 29 obsolete attachments)

92.61 KB, patch
Details | Diff | Splinter Review
5.53 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
6.55 KB, patch
Details | Diff | Splinter Review
860 bytes, patch
Gavin
: review+
Details | Diff | Splinter Review
1.12 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
604 bytes, text/plain
Details
9.28 KB, text/plain
Details
I was looking at http://tinderbox.mozilla.org/Firefox/1305941364.1305945463.28696.gz for unrelated reasons, and noticed that at the end of the test there are about 500 DOMWINDOW objects around, and only a few docshells.  So I grabbed the log and looked at the urls of the windows being destroyed _after_ the hidden window.  The top offenders of the 500 or so windows left at that point are:

  339 about:blank
   96 chrome://browser/content/browser.xul
   84 about:addons

I tried a quick test of just opening and closing windows locally and the browser.xul windows go away.

Oh, during that whole test run about 186 chrome://browser/content/browser.xul windows go away.

And windows are going away through the run as we go, so it's not like _all_ windows leak.  Just a bunch of them.  Looking at the log, we seem to have both inner and outer browser.xul windows around at the end there.

The big question: is this just test code holding on to windows?  Or is this test code calling code that would normally run in our UI and holds on to windows?

Again, all the windows are _closed_ (no docshell) but the inner and outer window objects are hanging around at shutdown.

It'd be nice if someone familiar with the browser tests took a look at which tests are opening the windows that leak or something.  Do we have any other ways of finding their allocation points?
Created attachment 534189 [details]
Log as a zip file to make sure it doesn't go away

Though I bet any Moth log will show this.
Oh, and Gavin, Dao, please cc whoever might be the right people for this?
OS: Windows 2000 → Windows 7

Comment 3

6 years ago
This is totally unacceptable and we will fix. Assigning to me.
Assignee: nobody → sayrer
So to be clear, since I realized some stuff from comment 0 was a bit fuzzy:

1)  I can't reproduce the problem locally running a debug build for about a minute opening and closing random windows and tabs.  Things go away as they should when I do that.

2)  About half the browser.xul windows the Moth tests opened were around at browserchrome test shutdown.  The other half went away before that.
(Assignee)

Comment 5

6 years ago
(In reply to comment #0)
> The big question: is this just test code holding on to windows?

That's very likely the case. Browser chrome tests all run in the same window. A good portion of them will open new windows, do stuff and close them again.
> A good portion of them will open new windows, do stuff and close them again.

Yes, but do they then hold references to those windows in perpetuity?
(Assignee)

Comment 7

6 years ago
They might add a load event listener and not remove it.
Sure.  The question is whether that's happening or whether we have something going on that could bite in normal browsing too.
(Assignee)

Comment 9

6 years ago
It's certainly happening.
OK, can we stop it happening and see whether the behavior goes away?
I'm perhaps jumping ahead, but is it possible to add a test (or modify the test harness?) so that when this is fixed is stays fixed? EG, something makes noise and turns orange if a test (or real bug!) causes a window to leak to shutdown.
The hidden window is always alive until shutdown.  So we'd need to do something smart....
We can assert that, after a GC, there are no longer any windows that don't have a Components object (since we remove the Components object when the window is closed).  That's the heuristic that Leak Monitor uses.  Unfortunately, we haven't gotten back to cleanly meeting that even for simple testcases since Firefox 2.
(Assignee)

Comment 14

6 years ago
Out of 748 tests, I found 53 tests where addEventListener occurs more often than removeEventListener. This is a naive way to count and very likely inaccurate, but the scale seems reasonable.
(Assignee)

Comment 15

6 years ago
Created attachment 535361 [details]
candidate list
(Assignee)

Comment 16

6 years ago
Created attachment 537771 [details] [diff] [review]
remove event listeners in tests
[Checked in: Comment 17]
Assignee: sayrer → dao
(Assignee)

Comment 17

6 years ago
Comment on attachment 537771 [details] [diff] [review]
remove event listeners in tests
[Checked in: Comment 17]

http://hg.mozilla.org/mozilla-central/rev/2e45e66a9200
Attachment #537771 - Attachment description: remove event listeners in tests → remove event listeners in tests (checked in)
(Assignee)

Comment 18

6 years ago
Doesn't look like this made a difference.
(Assignee)

Comment 19

6 years ago
Created attachment 538233 [details] [diff] [review]
clean out each test's scope
[Checked in: Comment 27]

this seems to get the number of windows alive at the end of the test run down from 542 (http://tbpl.mozilla.org/?rev=5795ed49996d, http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1307606151.1307609559.14438.gz) to 287 (http://tbpl.mozilla.org/?tree=Try&rev=6ae034cb75f8, http://tinderbox.mozilla.org/showlog.cgi?log=Try/1307607473.1307610961.20131.gz)
Attachment #538233 - Flags: review?(gavin.sharp)
(Assignee)

Comment 20

6 years ago
(In reply to comment #19)
> Created attachment 538233 [details] [diff] [review] [review]
> clean out each test's scope
> 
> this seems to get the number of windows alive at the end of the test run
> down from 542 to 287

I should add that I don't actually expect the const -> var changes in this patch (so that 'delete' can kill the references) to contribute to the lower DOM window count. It's just something I wanted to rule out as a factor.
Comment on attachment 538233 [details] [diff] [review]
clean out each test's scope
[Checked in: Comment 27]

Can you put the destroy()/nulling of .scope in nextTest (after the duration stuff), rather than in the waitForWindowsState callback?
Attachment #538233 - Flags: review?(gavin.sharp) → review+
Hmm.  Is it expected that the test scopes will hang around like that?
(In reply to comment #22)
> Hmm.  Is it expected that the test scopes will hang around like that?

The test array is kept around for the duration of the run, so yes.
Can we pop things out of that array instead?  Seems cleaner....
The test objects keep track of the test results, so no. This patch clears the stuff that needs to be/can be cleared, I'm not sure I understand why you think it's "unclean".
Just generally, clearing properties seems like something that someone could forget to do in other circumstances.  In this case it sounds like that's a non-issue.
(Assignee)

Comment 27

6 years ago
Comment on attachment 538233 [details] [diff] [review]
clean out each test's scope
[Checked in: Comment 27]

http://hg.mozilla.org/mozilla-central/rev/443c7cd5bdf8
Attachment #538233 - Attachment description: clean out each test's scope → clean out each test's scope (checked in)
(Assignee)

Comment 28

6 years ago
Created attachment 538468 [details]
URIs
Attachment #534189 - Attachment is obsolete: true
Attachment #535361 - Attachment is obsolete: true
Apparently mobile-browser-chrome was depending on things not being cleaned up, since that patch took it from two permaoranges to fifteen.
(Assignee)

Updated

6 years ago
Depends on: 663401
(Assignee)

Comment 30

6 years ago
(In reply to comment #29)
> Apparently mobile-browser-chrome was depending on things not being cleaned
> up, since that patch took it from two permaoranges to fifteen.

Sounds like these mobile tests are just broken, as the scopes get cleared after the tests have declared that they're finished.
(Assignee)

Comment 31

6 years ago
(In reply to comment #30)
> (In reply to comment #29)
> > Apparently mobile-browser-chrome was depending on things not being cleaned
> > up, since that patch took it from two permaoranges to fifteen.
> 
> Sounds like these mobile tests are just broken, as the scopes get cleared
> after the tests have declared that they're finished.

Apparently it's still 2 failures with my patch: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1307717565.1307718056.14042.gz
Yep, I can't read: between the suite just being turned on on -inbound (after the regressing patch landed) and the merges in both directions and the total failure rate, I missed by one push, and it was the merge after you. Sorry for the distraction.

Updated

6 years ago
Summary: We seem to be leaking hundreds of windows through shutdown during browser chrome tests → We seem to be leaking hundreds of windows until shutdown during browser-chrome tests
(Assignee)

Comment 33

6 years ago
Created attachment 538835 [details] [diff] [review]
reset TabContextMenu.contextTab when manually calling TabContextMenu.updateContextMenu
[Checked in: Comment 33]

http://hg.mozilla.org/mozilla-central/rev/bbbc80a2bd8c
(Assignee)

Comment 34

6 years ago
Created attachment 538848 [details]
script for building the URI list

putting this here since I already lost it once
Blocks: 659858
Keywords: mlk
Whiteboard: [MemShrink:P1]
(Assignee)

Updated

6 years ago
Depends on: 664116
(Assignee)

Updated

6 years ago
Depends on: 664472
(Assignee)

Updated

6 years ago
Depends on: 664519
(Assignee)

Updated

6 years ago
Depends on: 664549
(Assignee)

Updated

6 years ago
Depends on: 664672
Dão, I know some dependent bugs have landed here, can you summarize where we're at and what's left to do? Thanks!
(Assignee)

Updated

6 years ago
Depends on: 666221
(Assignee)

Comment 36

6 years ago
The dependent bugs have helped here and there, but on balance the window count is actually growing as people land more tests.

I just disabled sessionstore tests on Try and the number went from ~330 to 194. The URIs affected by this are about:blank (130 -> 66) and chrome://browser/content/browser.xul (79 -> 13). I now need to figure out if sessionstore code or the tests are to blame.
(Assignee)

Comment 37

6 years ago
Created attachment 541165 [details]
script for building the URI list
Attachment #538848 - Attachment is obsolete: true
(Assignee)

Comment 38

6 years ago
Created attachment 541166 [details]
URIs
Attachment #538468 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Depends on: 668000
(Assignee)

Updated

6 years ago
Depends on: 668972
(Assignee)

Updated

6 years ago
Depends on: 669178
(Assignee)

Updated

6 years ago
Keywords: meta
(Assignee)

Comment 39

6 years ago
Created attachment 544020 [details]
URIs
Attachment #541166 - Attachment is obsolete: true
Summary: We seem to be leaking hundreds of windows until shutdown during browser-chrome tests → [meta] We seem to be leaking hundreds of windows until shutdown during browser-chrome tests
(Assignee)

Comment 40

6 years ago
Created attachment 544046 [details] [diff] [review]
schedule final GC
[Checked in: Comment 43]

This shaves off some noise. about:blank still fluctuates slightly and pluginInstallerWizard.xul remains random as ever.
Attachment #544046 - Flags: review?(gavin.sharp)
(Assignee)

Updated

6 years ago
OS: Windows 7 → All
Hardware: x86 → All
The installtrigger windows may be http://hg.mozilla.org/mozilla-central/annotate/4df47729b74d/browser/base/content/browser.js#l740 , though that does try to take care of releasing the references when the notifications are removed, so I'm not sure why they'd be leaking until the end of the run.
Attachment #544046 - Flags: review?(gavin.sharp) → review+

Comment 42

6 years ago
(In reply to comment #40)
> Created attachment 544046 [details] [diff] [review] [review]
> schedule final GC
> 
> This shaves off some noise. about:blank still fluctuates slightly and
> pluginInstallerWizard.xul remains random as ever.

Should probably do CC and MP in addition to the precise GC. (Look at what about:memory does.)
(Assignee)

Comment 43

6 years ago
Comment on attachment 544046 [details] [diff] [review]
schedule final GC
[Checked in: Comment 43]

http://hg.mozilla.org/mozilla-central/rev/58101c64c83c
Attachment #544046 - Attachment description: schedule final GC → schedule final GC (checked in)
(Assignee)

Comment 44

6 years ago
Created attachment 544169 [details]
URIs

http://example.com/browser/toolkit/components/console/hudservice/tests/browser/test-error.html is new, introduced by the TM merge (6840fbf4dcdd).
Attachment #544020 - Attachment is obsolete: true
(Assignee)

Comment 45

6 years ago
Don't leak test-console.html in browser_webconsole_bug_580001_closing_after_completion.js:
http://hg.mozilla.org/mozilla-central/rev/7cbeab4cc868
Interestingly, this also seems to have removed test-error.html from the list.
(Assignee)

Comment 46

6 years ago
Created attachment 544183 [details]
URIs
Attachment #544169 - Attachment is obsolete: true
(In reply to comment #43)
> Comment on attachment 544046 [details] [diff] [review] [review]
> schedule final GC (checked in)
> 
> http://hg.mozilla.org/mozilla-central/rev/58101c64c83c

I filed bug 669617 for an apparently-new intermittent mochitest-chrome crash that occurred on the push following this one; I don't know if it's related in any way but the timing looks a bit suspicious.
(Assignee)

Updated

6 years ago
Depends on: 670140
(Assignee)

Comment 48

6 years ago
The four places.xul instances originate from these tests:

browser_410196_paste_into_tags.js
browser_416459_cut.js
browser_library_batch_delete.js
browser_library_left_pane_commands.js
(Assignee)

Comment 49

6 years ago
The number of docshells is now 55 (previously 7) starting with <http://hg.mozilla.org/mozilla-central/rev/a447e63943e1>.

Comment 50

6 years ago
... which was backed out (along with the patch it was a bustage fix for).
(Assignee)

Updated

6 years ago
Depends on: 670463

Updated

6 years ago
Alias: bc-leaks

Updated

6 years ago
Blocks: 671421
(Assignee)

Updated

6 years ago
Depends on: 671543
(Assignee)

Comment 51

6 years ago
(In reply to comment #11)
> I'm perhaps jumping ahead, but is it possible to add a test (or modify the
> test harness?) so that when this is fixed is stays fixed? EG, something
> makes noise and turns orange if a test (or real bug!) causes a window to
> leak to shutdown.

Has anyone ideas on how to achieve this?

The list is slowly growing every few days, which makes this bug an uphill battle that I'd have to keep fighting for multiple months.
In Bug 633670 I'm going to implement some code to test to see that a window has been GCd.  Plan is to work on this at the beginning of next week.

Comment 53

6 years ago
We can make new leaks turn b-c orange even before all the dependencies are fixed. For example, we could make it turn orange if the number of leaked windows changes.
(Assignee)

Updated

6 years ago
Depends on: 633670
(Assignee)

Updated

6 years ago
Depends on: 672470
(Assignee)

Comment 54

6 years ago
Created attachment 547413 [details] [diff] [review]
more GC/CC
[Checked in: Comment 62]

Cuts away more about:blank and pluginInstallerWizard.xul (down to 1). Calling garbageCollect only once also helps, but not quite as much.
Attachment #547413 - Flags: review?(gavin.sharp)

Comment 55

6 years ago
The most effective way to clear memory is to fire several memory-pressure notifications separated by trips to the event loop, perhaps with some schedulePreciseGC tossed in.  See my comments in bug 654041.

Ideally, the mochitest shutdown sequence would make it possible to see how many windows remain after each memory-pressure notification.  This way, we can file bugs when windows take multiple GCs/CCs to clean up (which often means we're one additional edge away from a leak).
(Assignee)

Comment 56

6 years ago
(In reply to comment #55)
> The most effective way to clear memory is to fire several memory-pressure
> notifications separated by trips to the event loop, perhaps with some
> schedulePreciseGC tossed in.  See my comments in bug 654041.

I don't think we're interested in flushing caches here.
(In reply to comment #56)
> 
> I don't think we're interested in flushing caches here.

Then do multiple GC+CC runs via the event loop.
(Assignee)

Comment 58

6 years ago
(In reply to comment #57)
> (In reply to comment #56)
> > 
> > I don't think we're interested in flushing caches here.
> 
> Then do multiple GC+CC runs via the event loop.

Doesn't seem to make a difference.
Comment on attachment 547413 [details] [diff] [review]
more GC/CC
[Checked in: Comment 62]

I'm kind of lost as to what the goal is here. Have we determined that the window leaks that this patch "fixes" are not real issues that come up outside of the test harness (e.g. a problem with our GC/CC heuristics)? Test-harness only changes that hide them are only useful if we can say with confidence that they're not "real" issues, and I'm not sure that we can do that.

Comment 60

6 years ago
The goal is to get the relatively minor "requires multiple GCs/CCs to clean up" bugs out of our way so we can start ratcheting down on *something*.

I'd like these bugs to be noted in the log.  But they probably shouldn't turn the tree orange, because they would be intermittently caught (due to heuristics such as GC timing, CC timing, and conservative scanning).

Also, winutils.garbageCollect triggers a CC, while schedulePreciseGC does not. So we clearly need at least one of the added calls.
Comment on attachment 547413 [details] [diff] [review]
more GC/CC
[Checked in: Comment 62]

OK, that makes sense. The comment before this code should probably be adjusted - this doesn't really result in a "more accurate" window count, just a different one (omitting windows that would have eventually been picked up anyways - i.e. we're hiding less serious issues so that we can focus on more serious ones).
Attachment #547413 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 62

6 years ago
Comment on attachment 547413 [details] [diff] [review]
more GC/CC
[Checked in: Comment 62]

http://hg.mozilla.org/mozilla-central/rev/b3149b080c65
Attachment #547413 - Attachment description: more GC/CC → more GC/CC (checked in)
(Assignee)

Comment 63

6 years ago
Created attachment 548179 [details]
URIs
Attachment #544183 - Attachment is obsolete: true
(Assignee)

Comment 64

6 years ago
Created attachment 548180 [details]
URIs
Attachment #548179 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
No longer depends on: 664472
(Assignee)

Updated

6 years ago
Depends on: 674486
(Assignee)

Updated

6 years ago
No longer depends on: 670140
(Assignee)

Updated

6 years ago
Depends on: 675412
(Assignee)

Comment 65

6 years ago
about:addons leak was due to an undeclared variable:
http://hg.mozilla.org/mozilla-central/rev/040446311029
(Assignee)

Comment 66

6 years ago
Created attachment 549675 [details]
URIs
Attachment #548180 - Attachment is obsolete: true
(Assignee)

Comment 67

6 years ago
Created attachment 550047 [details]
URIs
Attachment #549675 - Attachment is obsolete: true
(Assignee)

Comment 68

6 years ago
Created attachment 551409 [details]
URIs

Something somewhere made a huge difference. about:blank went from 100 to 31, browser.xul from 78 to 8.

test-console.html and test-console-api.html are new, regressed by <http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=eef25ec2d58e>.
Attachment #550047 - Attachment is obsolete: true
(Assignee)

Comment 69

6 years ago
> Something somewhere made a huge difference. about:blank went from 100 to 31,
> browser.xul from 78 to 8.

Something in http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=0fbdefb0a9b5
(Assignee)

Comment 70

6 years ago
(In reply to Dão Gottwald [:dao] from comment #69)
> > Something somewhere made a huge difference. about:blank went from 100 to 31,
> > browser.xul from 78 to 8.
> 
> Something in
> http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=0fbdefb0a9b5

<http://hg.mozilla.org/integration/fx-team/pushloghtml?changeset=6f1affa4bb5e>, so I guess bug 651643.
Depends on: 651643
(Assignee)

Comment 71

6 years ago
(In reply to Dão Gottwald [:dao] from comment #70)
> (In reply to Dão Gottwald [:dao] from comment #69)
> > > Something somewhere made a huge difference. about:blank went from 100 to 31,
> > > browser.xul from 78 to 8.
> > 
> > Something in
> > http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=0fbdefb0a9b5
> 
> <http://hg.mozilla.org/integration/fx-team/pushloghtml?changeset=6f1affa4bb5e>,
> so I guess bug 651643.

After trying to verify this, I found out that bug 666475 did it.
Depends on: 666475
No longer depends on: 651643
(Assignee)

Comment 72

6 years ago
(In reply to Dão Gottwald [:dao] from comment #71)
> (In reply to Dão Gottwald [:dao] from comment #70)
> > (In reply to Dão Gottwald [:dao] from comment #69)
> > > > Something somewhere made a huge difference. about:blank went from 100 to 31,
> > > > browser.xul from 78 to 8.
> > > 
> > > Something in
> > > http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=0fbdefb0a9b5
> > 
> > <http://hg.mozilla.org/integration/fx-team/pushloghtml?changeset=6f1affa4bb5e>,
> > so I guess bug 651643.
> 
> After trying to verify this, I found out that bug 666475 did it.

Err, I mixed up the logs.
Depends on: 651643
No longer depends on: 666475
(Assignee)

Updated

6 years ago
Depends on: 677996
(Assignee)

Comment 73

6 years ago
Created attachment 553428 [details]
script for building the URI list
Attachment #541165 - Attachment is obsolete: true
Created attachment 554470 [details]
leak statistics (before backout)

I wrote a little script to narrow down which tests are leaking DOMWindows and docShells. It parses the build log, records created DOMWindows and docShells per test and counts them as "leaked" if they get removed after the test suite shutdown.

I compared the builds log from before and after the backout of bug 662812. I hope that can help a bit finding the leak sources.
Created attachment 554471 [details]
leak statistics (after backout)
(Assignee)

Comment 76

6 years ago
(In reply to Tim Taubert [:ttaubert] from comment #74)
> Created attachment 554470 [details]
> leak statistics (before backout)
> 
> I wrote a little script to narrow down which tests are leaking DOMWindows
> and docShells. It parses the build log, records created DOMWindows and
> docShells per test and counts them as "leaked" if they get removed after the
> test suite shutdown.

Note that some windows aren't leaked, they just exist as part of the browser window that all tests run in. This is probably the case for browser_bug597218.js, for instance, which I guess causes panorama to load its iframe.
Created attachment 554493 [details]
leak statistics (before backout)

assigned urls to the leaked windows
Attachment #554470 - Attachment is obsolete: true
Created attachment 554494 [details]
leak statistics (after backout)

assigned urls to the leaked windows
Attachment #554471 - Attachment is obsolete: true
(In reply to Dão Gottwald [:dao] from comment #76)
> Note that some windows aren't leaked, they just exist as part of the browser
> window that all tests run in. This is probably the case for
> browser_bug597218.js, for instance, which I guess causes panorama to load
> its iframe.

Yeah, I already noticed that. Thanks for the hint!
Depends on: 680647
Depends on: 680649
Depends on: 680686
(Assignee)

Comment 80

6 years ago
Created attachment 554804 [details]
URIs
Attachment #551409 - Attachment is obsolete: true
Attachment #554493 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Depends on: 683128
(Assignee)

Updated

6 years ago
Depends on: 683320
Depends on: 683953
(Assignee)

Updated

6 years ago
Depends on: 684803
(Assignee)

Comment 81

6 years ago
Created attachment 559090 [details]
URIs
Attachment #554804 - Attachment is obsolete: true
(Assignee)

Comment 82

6 years ago
Created attachment 562034 [details]
URIs

Since the previous update, places.xul and bookmarkProperties.xul disappeared, the browser.xul count grew by 1.
Attachment #559090 - Attachment is obsolete: true
(Assignee)

Comment 83

6 years ago
Created attachment 563078 [details]
URIs
Attachment #562034 - Attachment is obsolete: true
(Assignee)

Comment 84

6 years ago
(In reply to Dão Gottwald [:dao] from comment #83)
> Created attachment 563078 [details]
> URIs

For the record, this is still up to date.
Dão, what's the status of this bug?  Looks like there are a few dependent bugs still, but not that many.  Are you working on them?  Maybe this should be downgraded to a MemShrink:P2?

Comment 86

6 years ago
Well, how important is it for the tree to turn orange when we introduce a big new leak bug?  Getting the leaks to zero is the most straightforward way to do that, but not the only way.
(Assignee)

Comment 87

6 years ago
I don't really care if this is a P1 -- I think bug 633670 and/or bug 683953 should be, though, so we can stop monitoring this manually.

The biggest remaining offender is bug 675412, by the way, which seems to be stalled.
(Assignee)

Updated

6 years ago
Depends on: 695480
(Assignee)

Updated

6 years ago
Blocks: 668871
(Assignee)

Comment 88

6 years ago
Created attachment 583272 [details]
URIs

We've gone from 127 to 615 windows. Most of the new ones are about:addons instances.
Attachment #563078 - Attachment is obsolete: true
(Assignee)

Comment 89

6 years ago
(In reply to Dão Gottwald [:dao] from comment #88)
> Created attachment 583272 [details]
> URIs
> 
> We've gone from 127 to 615 windows. Most of the new ones are about:addons
> instances.

I've narrowed down the regression range to cbb0233c7ba8 (good) and be0fffc6b0d1 (bad).
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cbb0233c7ba8&tochange=be0fffc6b0d1
The hotfix addon work is in that range.
I confirmed from mozilla-inbound tinderbox logs that https://tbpl.mozilla.org/?branch=mozilla-inbound&rev=0ab6831c77bc is good (before the regression) based on the Win debug log at https://tbpl.mozilla.org/php/getParsedLog.php?id=8074372&tree=Mozilla-Inbound .  Still looking for the nearest non-deleted debug mochitest-other tinderbox log after the push khuey suspects (which is https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=e5d8d2fb987d ).

Maybe we should have a separate bug on the regression, though?
(Assignee)

Comment 92

6 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #90)
> The hotfix addon work is in that range.

Further narrowed down to 61a46d539c69 (good) and be0fffc6b0d1 (bad):

> be0fffc6b0d1 Michael Wu — Bug 710029 - Assorted build system tweaks for B2G/Gonk. r=khuey
> 26cce2f95d49 Brian Hackett — Fix lingering uses of int32 types, no bug.
> c5026431768d Doug Sherk — Bug 711579: fix WGL context creation without robustness r=Bas
> b895916227b2 Doug Sherk — Bug 711226: separate desktop and ES 2.0 symbol loading r=bjacob Symbols were amalgamated into a single table which worked up until recently. This patch separates them so that there is a common list, then two separate ones for desktop and OpenGL ES 2.0.
> 9939bcd259a3 Doug Sherk — Bug 708207: implement WebGL's getShaderPrecisionFormat r=bjacob
> de66e7bd2b98 Brian Hackett — Add interface for accessing PC counter information from chrome code, bug 687134. r=sfink,waldo
> 5db46b0c2f14 Jonas Sicking — Back out fb4d12d2a2da, bug 701772, due to leaks.
> fbf296760b0f Steffen Wilberg — Bug 710064: Make the 'Update Add-ons Automatically' checkbox state depend on the extensions.update.enabled pref, in addition to the existing extensions.update.autoUpdateDefault pref. r=mossop

Bug 710064 / http://hg.mozilla.org/mozilla-central/rev/fbf296760b0f added a pref observer to extensions.js without ever removing it, so I guess that's the culprit.
(Assignee)

Comment 93

6 years ago
(In reply to David Baron [:dbaron] from comment #91)
> I confirmed from mozilla-inbound tinderbox logs that
> https://tbpl.mozilla.org/?branch=mozilla-inbound&rev=0ab6831c77bc is good
> (before the regression) based on the Win debug log at
> https://tbpl.mozilla.org/php/getParsedLog.php?id=8074372&tree=Mozilla-
> Inbound .  Still looking for the nearest non-deleted debug mochitest-other
> tinderbox log after the push khuey suspects (which is
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?changeset=e5d8d2fb987d ).
> 
> Maybe we should have a separate bug on the regression, though?

filed bug 712614
Depends on: 712614
(In reply to Dão Gottwald [:dao] from comment #88)

> We've gone from 127 to 615 windows. Most of the new ones are about:addons
> instances.

Can we add a leaked-window threshold check (eg, < 130) to prevent further slips like this? Experience from previous "drive X to 0" efforts seems to be that it (1) takes a long time to actually reach 0 and (2) in the meantime more regressions will slip in.
(Assignee)

Comment 95

6 years ago
Created attachment 587055 [details]
URIs

194 times browser.xul (instead of 8)
Attachment #583272 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Depends on: 716692
(Assignee)

Comment 96

6 years ago
Created attachment 587629 [details]
URIs
Attachment #587055 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Depends on: 717191
(In reply to Justin Dolske [:Dolske] from comment #94)
> Can we add a leaked-window threshold check (eg, < 130) to prevent further
> slips like this? Experience from previous "drive X to 0" efforts seems to be
> that it (1) takes a long time to actually reach 0 and (2) in the meantime
> more regressions will slip in.

I just uploaded a new patch to bug 683953 which implements a threshold and makes the tree go orange if we leak more than that. Unfortunately, the counts vary heavily on different platforms, see bug 683953 comment #13 for some stats.
(In reply to Tim Taubert [:ttaubert] from comment #97)
> Unfortunately, the
> counts vary heavily on different platforms

not a big deal, having a threshold per platform is better than not having one.
(Assignee)

Comment 99

6 years ago
Created attachment 589056 [details]
URIs
Attachment #587629 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Depends on: 718536
(Assignee)

Comment 100

6 years ago
Created attachment 589821 [details]
URIs
Attachment #589056 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Depends on: 719849
(Assignee)

Comment 101

6 years ago
Created attachment 591119 [details]
URIs
Attachment #589821 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Depends on: 720985
(Assignee)

Updated

6 years ago
Depends on: 722479
http://hg.mozilla.org/mozilla-central/rev/b2d50f779d6c
Bug 658738 - split browser_394759.js into three tests
Blocks: 726521
Attachment #537771 - Attachment description: remove event listeners in tests (checked in) → remove event listeners in tests [Checked in: Comment 17]
Attachment #538233 - Attachment description: clean out each test's scope (checked in) → clean out each test's scope [Checked in: Comment 27]
Attachment #538835 - Attachment description: reset TabContextMenu.contextTab when manually calling TabContextMenu.updateContextMenu (checked in) → reset TabContextMenu.contextTab when manually calling TabContextMenu.updateContextMenu [Checked in: Comment 33]
Attachment #544046 - Attachment description: schedule final GC (checked in) → schedule final GC [Checked in: Comment 43]
Attachment #547413 - Attachment description: more GC/CC (checked in) → more GC/CC [Checked in: Comment 62]
Version: unspecified → Trunk
Depends on: 728541
(Assignee)

Comment 103

5 years ago
Created attachment 598580 [details]
URIs
Attachment #591119 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Depends on: 728606
Blocks: 728638
(Assignee)

Updated

5 years ago
Depends on: 728426
Would it be possible (maybe via adding something to nsIWindowUtils) to track the number of DOM windows that exist, so that the test harness can directly implicate each test as it runs? At some point tests would default to not leaking, but old tests could possibly be grandfathered in with some sort of limited opt-out?
(Assignee)

Updated

5 years ago
Attachment #554494 - Attachment is obsolete: true
(Assignee)

Comment 105

5 years ago
Created attachment 604090 [details]
URIs
Attachment #598580 - Attachment is obsolete: true
Depends on: 734148
Depends on: 734155
Blocks: 734148
No longer depends on: 734148
Depends on: 734172
(Assignee)

Updated

5 years ago
Depends on: 749361
(Assignee)

Updated

5 years ago
Depends on: 734554
(Assignee)

Updated

5 years ago
Depends on: 750269
(Assignee)

Updated

5 years ago
Depends on: 751086
(Assignee)

Updated

5 years ago
Depends on: 751100
(Assignee)

Updated

5 years ago
Depends on: 751085
(Assignee)

Updated

5 years ago
Depends on: 751334
Depends on: 741070
(Assignee)

Updated

5 years ago
No longer depends on: 741070
(Assignee)

Updated

5 years ago
Depends on: 754800
(Assignee)

Updated

5 years ago
Depends on: 754801
(Assignee)

Updated

5 years ago
Depends on: 754803

Updated

5 years ago
Depends on: 759707

Updated

5 years ago
Depends on: 759708

Updated

5 years ago
Depends on: 759709

Updated

5 years ago
Depends on: 759711

Updated

5 years ago
Depends on: 759713

Updated

5 years ago
Depends on: 759715

Updated

5 years ago
Depends on: 759716

Updated

5 years ago
Depends on: 759717

Updated

5 years ago
Depends on: 759718

Updated

5 years ago
Depends on: 759720
(Assignee)

Updated

5 years ago
Depends on: 759754
Ed, I'm curious what spurred this large list of new bugs.  Are these new leaks, or have they just not been filed before?
They are the tests that are regularly causing us to go over the threshold and thus cause bug 754804. They just haven't been filed until now. Dão has been driving the shutdown leaks threshold related bugs for a while now, thought he could do with a second pair of hands :-)
Depends on: 766291

Updated

5 years ago
Depends on: 766506

Updated

5 years ago
Depends on: 767896

Updated

5 years ago
Depends on: 768422

Updated

5 years ago
Depends on: 768935

Updated

5 years ago
Depends on: 768936

Updated

5 years ago
Depends on: 769646

Updated

5 years ago
Depends on: 770542

Updated

5 years ago
Depends on: 772382

Updated

5 years ago
Depends on: 772383

Updated

5 years ago
Depends on: 772384

Updated

5 years ago
Depends on: 772385

Updated

5 years ago
No longer depends on: 751085

Updated

5 years ago
Depends on: 772778

Updated

5 years ago
Depends on: 773216

Updated

5 years ago
Depends on: 773387

Updated

5 years ago
Depends on: 773653

Updated

5 years ago
Depends on: 773661

Updated

5 years ago
Depends on: 775380

Updated

5 years ago
Depends on: 775393

Updated

5 years ago
Depends on: 751085

Updated

5 years ago
Depends on: 776553

Updated

5 years ago
Depends on: 776554

Updated

5 years ago
Depends on: 778433

Updated

5 years ago
No longer depends on: 776553

Updated

5 years ago
Depends on: 782269

Updated

5 years ago
Depends on: 782858
Whiteboard: [MemShrink:P1] → [MemShrink:P2]
Depends on: 932880
Depends on: 932867
Depends on: 932891
Depends on: 932900
Depends on: 932898
Depends on: 933551
(Assignee)

Comment 108

3 years ago
I think this has outlived its usefulness as a tracking bug let alone as a bug where actual work happens. New leaky tests can be (and have been) dealt with in other bugs that don't need to block this one.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.