Closed
Bug 885966
Opened 11 years ago
Closed 11 years ago
Intermittent devtools/shared/test/browser_telemetry_toolboxtabs.js | Test timed out | lots of bytes leaked leaked (AsyncStatement, AtomImpl, BackstagePass, BodyRule, CalculateFrecencyFunction, ...)
Categories
(DevTools :: General, defect)
Tracking
(firefox24 fixed, firefox25 fixed)
RESOLVED
FIXED
Firefox 25
People
(Reporter: ttaubert, Assigned: miker)
References
Details
(Keywords: intermittent-failure, memory-leak)
Attachments
(2 files, 1 obsolete file)
1.35 MB,
image/png
|
Details | |
27.22 KB,
patch
|
jwalker
:
review+
|
Details | Diff | Splinter Review |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 2•11 years ago
|
||
Test introduced by bug 866642 - please can you take a look at this? :-)
Blocks: 866642
Flags: needinfo?(mratcliffe)
Assignee | ||
Comment 3•11 years ago
|
||
The opens each devtool multiple times. GC is clearly having trouble keeping up. I can trigger GC a few times during the test.
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
Flags: needinfo?(mratcliffe)
Comment 4•11 years ago
|
||
Thank you :-)
Assignee | ||
Comment 5•11 years ago
|
||
This seems to have been fixed as part of some other bug ... maybe bug 885751.
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 8•11 years ago
|
||
This patch forces GC each time a tool is destroyed and just previous to test finish. It also adds a delay after each tool is destroyed to allow Fx to gather his thoughts. I have also taken the liberty of fixing a bunch of strict warnings and actual errors that were only visible from a debug build (the test logs are now a little shorter for debug builds). Some of these errors would have caused destructors to terminate early. Try: https://tbpl.mozilla.org/?tree=Try&rev=14878cce202b
Attachment #768266 -
Flags: review?(jwalker)
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 10•11 years ago
|
||
Comment on attachment 768266 [details] [diff] [review] Patch Review of attachment 768266 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/framework/toolbox.js @@ +486,5 @@ > tab.setAttribute("selected", "true"); > > let prevToolId = this._currentToolId; > > + if (typeof this._currentToolId != "undefined" && this._currentToolId == id) { Confused as to why this is needed? (Here an elsewhere) ::: browser/devtools/inspector/breadcrumbs.js @@ +604,5 @@ > let element = this.nodeHierarchy[this.currentIndex].button; > > // Repeated calls to ensureElementIsVisible would interfere with each other > // and may sometimes result in incorrect scroll positions. > + if (typeof this._ensureVisibleTimeout != "undefined") { In general, doing (typeof foo != "undefined") seems long winded doesn't it? I think (foo) is more common in Moz code? ::: browser/devtools/shared/test/browser_telemetry_toolboxtabs.js @@ +49,5 @@ > toolbox.once("destroyed", function() { > + // Force GC (Bug 885966) > + SpecialPowers.DOMWindowUtils.garbageCollect(); > + > + setTimeout(function() { I'm unclear why we need the timeout. If we've done a force GC isn't that enough? At the very least we should comment this, but does removing it actually retain the problem?
Attachment #768266 -
Flags: review?(jwalker)
Assignee | ||
Comment 11•11 years ago
|
||
Screenshot shows that this seems like this is caused by a GCLI issue. At least with the current patch we have some consistency. I have logged bug 887831 to disable this test until we find the cause of the problem.
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 19•11 years ago
|
||
I had to rebuild my PC over the weekend in order to get tests running again. Splitting the test into seven stops all leaks and ensures that the test will no longer time out.
Attachment #768266 -
Attachment is obsolete: true
Attachment #769640 -
Flags: review?(jwalker)
Assignee | ||
Comment 20•11 years ago
|
||
Try: https://tbpl.mozilla.org/?tree=Try&rev=3ff47e6ab579
Comment hidden (Legacy TBPL/Treeherder Robot) |
Updated•11 years ago
|
Attachment #769640 -
Flags: review?(jwalker) → review+
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 24•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d175faa55da7
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 26•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/d175faa55da7
Comment 27•11 years ago
|
||
Comment on attachment 769640 [details] [diff] [review] Patch v2 Review of attachment 769640 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/shared/test/Makefile.in @@ +11,5 @@ > > include $(DEPTH)/config/autoconf.mk > > MOCHITEST_BROWSER_FILES = \ > + browser_eventemitter_basic.js \ Can you please not change code from a correct style (two spaces) to an incorrect style (two tabs)?
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 29•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/f3cccda58421
status-firefox24:
--- → fixed
status-firefox25:
--- → fixed
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•