Closed Bug 1293324 Opened 8 years ago Closed 8 years ago

Intermittent leakcheck | default process: 10248 bytes leaked (BasicContainerLayer, BasicImplData, BasicLayerManager, BasicPaintedLayer, CompositableClient, ...)

Categories

(Core :: Graphics: Layers, defect, P3)

Product:
Core
Component:
Graphics: Layers
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox49 --- wontfix
firefox50 --- fixed
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: mstange)

References

Details

(Keywords: intermittent-failure, memory-leak, Whiteboard: [gfx-noted])

Attachments

(3 files, 2 obsolete files)

refcount log
2.31 MB, application/zip
Details
refcount balance tree
442.01 KB, text/plain
Details
Bug 1293324 - Release widgets from a runnable instead of from an objective C 'performAfterDelay' timer.
58 bytes, text/x-review-board-request
ehsan.akhgari
: review+
ritu
: approval-mozilla-aurora+
ritu
: approval-mozilla-beta+
Details
Filed by: wkocher [at] mozilla.com https://treeherder.mozilla.org/logviewer.html#?job_id=1525670&repo=autoland http://archive.mozilla.org/pub/firefox/tinderbox-builds/autoland-macosx64-debug/1470554272/autoland_yosemite_r7-debug_test-mochitest-clipboard-bm136-tests1-macosx-build67.txt.gz This keeps getting starred to a resolved bug 1216343, but this seems like it's exclusively on OSX (primarily 10.10, but that might just be because we don't run as much stuff on 10.6 these days), whereas the original bug seemed mostly to be happening on Windows.
Flags: needinfo?(nical.bugzilla)
See Also: → 1216343
I took note on the needinfo, but I'm pretty busy so it'll be a while before I come back to this.
The leak threshold is 10000 bytes, so probably the leak was just slowly increasing, and now it is intermittently passing it.
Flags: needinfo?(nical.bugzilla)
Whiteboard: [gfx-noted]
Keywords: mlk
Depends on: 1299871
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
I actual misread this bug. It is not a small increase over the existing content process leaks, it is a new main process leak. Our current main process leak is 0 byes. This needs a graphics person to fix it. Maybe bug 942688 caused this? That looks like it involves Skia and Cocoa widget, which are in the set of leaking objects.
Mason, could you look into figuring out if this is a regression from bug 942688? Thanks.
Flags: needinfo?(mchang)
(In reply to Andrew McCreight [:mccr8] from comment #32) > Mason, could you look into figuring out if this is a regression from bug > 942688? Thanks. Yeah I'll take a look.
Flags: needinfo?(mchang)
Here are two pushes: w/ patch - https://treeherder.mozilla.org/#/jobs?repo=try&revision=02a77cea04ad w/ backout - https://treeherder.mozilla.org/#/jobs?repo=try&revision=952a6cf31a52&selectedJob=27187031 The main pushes to look at at debug Mochitest-2 and Mochitest e10s-5. Both still have the leak check hitting quite a bit, so I don't think it's bug 942688
This reproduces in non-e10s mochitest-clipboard on 10.10 debug. https://archive.mozilla.org/pub/firefox/try-builds/mstange@mozilla.com-7e21d0a2b5e4c96ed1b9f8c83b2520cd8f2d58ad/try-macosx64-debug/try_yosemite_r7-debug_test-mochitest-clipboard-bm132-tests1-macosx-build726.txt.gz In this log the leaked window was opened during test_bug1012662_noeditor.html. It's the "Would you like Nightly to remember this login?" arrow panel. I haven't yet managed to convince the tryserver to print call stacks for the AddRef and Release calls to nsCocoaWindow.
Retriggers on autoland indicate that this is a regression from bug 1284687. More convincingly, a Try push of that backed out (it does so cleanly) is showing no OSX mochitest leaks. https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c611dbf897c5f013d14c53f0cd5a17e548eeae1 Mike, I don't know what the implications of leaving bug 1284687 unfixed are, but this is the #1 non-infra failure across multiple branches, so I'm leaning in the direction of backing out at this point unless we can figure out how to make these leaks go away ASAP.
Blocks: 1284687
Flags: needinfo?(mconley)
Assignee: nobody → mconley
Attachment #8797618 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8797618 [details] Bug 1293324 - Disable PopupNotification panel animations in mochitests. https://reviewboard.mozilla.org/r/83288/#review81924 Clearing review while mconley looks at this more. If this is killing the sheriffs, r=me to land temporarily but we should ideally aim for a more permanent fix that is less wallpapery. Note that we might want a pref for other reasons, but in that case it should likely be more general... (cf. bug 984589)
Attachment #8797618 - Flags: review?(gijskruitbosch+bugs)
Attached file refcount log —
This is the refcount log that we got from a leaking run of this test.
Attached file refcount balance tree —
And here's the refcount balance tree of the leaked window.
If I run $ perl tools/rb/make-tree.pl --object 0x12112b600 --ignore-balanced --exclude ignorestacks.txt < ~/Downloads/refcount-log.log > ~/Downloads/balance-tree.txt with an "ignorestacks.txt" file containing this: #00: -[ChildView releaseWidgets:][/builds/slave/test/build/application/NightlyDebug.app/Contents/MacOS/XUL +0x2f091ff] #00: -[ChildView viewWillDraw][/builds/slave/test/build/application/NightlyDebug.app/Contents/MacOS/XUL +0x2f092b1] then I get a balanced tree. This suggests that the imbalance is between viewWillDraw and releaseWidgets; i.e. that our problem is that there is no call to releaseWidgets between the last viewWillDraw and shutdown.
Credit where it's due.
Assignee: mconley → mstange
Flags: needinfo?(mconley)
Try push looks mostly green (the timeout is unrelated). Is this ready for review?
Flags: needinfo?(mstange)
Looks like it is. I've triggered a bunch more tests to be sure, but I'm going to put this up for review now.
Flags: needinfo?(mstange)
Attachment #8793594 - Attachment is obsolete: true
Comment on attachment 8798243 [details] Bug 1293324 - Release widgets from a runnable instead of from an objective C 'performAfterDelay' timer. https://reviewboard.mozilla.org/r/83768/#review82524 ::: widget/cocoa/nsChildView.mm:3144 (Diff revision 1) > { > mGLContext->SwapBuffers(); > mGLContext->fBindBuffer(LOCAL_GL_ARRAY_BUFFER, 0); > } > > +class WidgetsReleaserRunnable : public mozilla::Runnable Nit: please make this final. ::: widget/cocoa/nsChildView.mm:3157 (Diff revision 1) > + NS_IMETHOD > + Run() override > + { > + // Do nothing; all this runnable does is hold a reference the widgets in > + // mWidgetArray, and those references will be dropped when this runnable > + // is destroyed. This isn't doing anything that Runnable::Run() is not, so you can just replace this whole method with the comment. ::: widget/cocoa/nsChildView.mm:3991 (Diff revision 1) > while (parent) { > - NS_ADDREF(parent); > + widgetArray.AppendElement(parent); > - [widgetArray addObject:[NSNumber numberWithUnsignedInteger:(NSUInteger)parent]]; > parent = parent->GetParent(); > } > - NS_ADDREF(mGeckoChild); > + widgetArray.AppendElement(mGeckoChild); How about rewriting this more concisely, like this? nsCOMPtr<nsIWidget> widget = mGeckoChild; while (widget) { widgetArray.AppendElement(Move(widget)); widget = widget->GetParent(); }
Comment on attachment 8798243 [details] Bug 1293324 - Release widgets from a runnable instead of from an objective C 'performAfterDelay' timer. https://reviewboard.mozilla.org/r/83768/#review82528 r=me with or without the suggestion addressed. Up to you!
Attachment #8798243 - Flags: review?(ehsan) → review+
That is, the last comment!
(In reply to :Ehsan Akhgari from comment #84) > ::: widget/cocoa/nsChildView.mm:3991 > (Diff revision 1) > > while (parent) { > > - NS_ADDREF(parent); > > + widgetArray.AppendElement(parent); > > - [widgetArray addObject:[NSNumber numberWithUnsignedInteger:(NSUInteger)parent]]; > > parent = parent->GetParent(); > > } > > - NS_ADDREF(mGeckoChild); > > + widgetArray.AppendElement(mGeckoChild); > > How about rewriting this more concisely, like this? > > nsCOMPtr<nsIWidget> widget = mGeckoChild; > while (widget) { > widgetArray.AppendElement(Move(widget)); > widget = widget->GetParent(); > } The old code isn't doing this if parent is null, so I'm going to keep that behavior. I think only popup windows have parents, and those are the windows we're worried about accidentally deleting during viewWillDraw.
Pushed by mstange@themasta.com: https://hg.mozilla.org/integration/autoland/rev/04bbd8b53f9d Release widgets from a runnable instead of from an objective C 'performAfterDelay' timer. r=Ehsan
https://hg.mozilla.org/mozilla-central/rev/04bbd8b53f9d
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Soooooooooooo green! Thanks for hunting this down, Markus and Mike! Please do request Aurora/Beta approval on this when you feel it has sufficiently baked.
Flags: needinfo?(mstange)
Comment on attachment 8798243 [details] Bug 1293324 - Release widgets from a runnable instead of from an objective C 'performAfterDelay' timer. I think being green on central is all the baking it needs. Approval Request Comment [Feature/regressing bug #]: bug 550392 - This bug has existed for a long time. But something must have changed recently that affected how close to shutdown we paint a panel. [User impact if declined]: none, the leak that this fixes only occurs during shutdown, so it only matters for automatic leak detection and not to users [Describe test coverage new/current, TreeHerder]: yes [Risks and why]: very low, just uses a different event loop to stop holding on to popup widgets [String/UUID change made/needed]: none
Flags: needinfo?(mstange)
Attachment #8798243 - Flags: approval-mozilla-beta?
Attachment #8798243 - Flags: approval-mozilla-aurora?
Comment on attachment 8798243 [details] Bug 1293324 - Release widgets from a runnable instead of from an objective C 'performAfterDelay' timer. Fixes an intermittent, Aurora51+, Beta50+
Attachment #8798243 - Flags: approval-mozilla-beta?
Attachment #8798243 - Flags: approval-mozilla-beta+
Attachment #8798243 - Flags: approval-mozilla-aurora?
Attachment #8798243 - Flags: approval-mozilla-aurora+
Attachment #8797618 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: