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)

x86_64
macOS
defect
Not set
normal

Tracking

(firefox24 fixed, firefox25 fixed)

RESOLVED FIXED
Firefox 25
Tracking Status
firefox24 --- fixed
firefox25 --- fixed

People

(Reporter: ttaubert, Assigned: miker)

References

Details

(Keywords: intermittent-failure, memory-leak)

Attachments

(2 files, 1 obsolete file)

Test introduced by bug 866642 - please can you take a look at this? :-)
Blocks: 866642
Flags: needinfo?(mratcliffe)
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)
Thank you :-)
This seems to have been fixed as part of some other bug ... maybe bug 885751.
Attached patch Patch (obsolete) — Splinter Review
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 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)
Attached image Screenshot of timeout
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.
Attached patch Patch v2Splinter Review
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)
Attachment #769640 - Flags: review?(jwalker) → review+
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 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)?
Keywords: mlk
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: