Closed Bug 838069 Opened 12 years ago Closed 12 years ago

Intermittent browser_toolbox_window_shortcuts.js | leaked until shutdown [nsGlobalWindow #9377 about:blank] and 11 more

Categories

(DevTools :: Framework, defect)

Product:
DevTools
Component:
Framework
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 22

People

(Reporter: philor, Assigned: Optimizer)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 2 obsolete files)

disabling this time for sure.
(deleted), patch
Details | Diff | Splinter Review 
https://tbpl.mozilla.org/php/getParsedLog.php?id=19441140&tree=Mozilla-Inbound
Rev3 WINNT 6.1 mozilla-inbound opt test mochitest-browser-chrome on 2013-02-04 20:51:29 PST for push 687a4154ebaf
slave: talos-r3-w7-033

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_window_shortcuts.js | leaked until shutdown [nsGlobalWindow #9377 about:blank]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_window_shortcuts.js | leaked until shutdown [nsGlobalWindow #9375 about:blank]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_window_shortcuts.js | leaked until shutdown [nsGlobalWindow #9370 chrome://browser/content/devtools/framework/toolbox.xul]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_window_shortcuts.js | leaked until shutdown [nsGlobalWindow #9384 about:blank]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_window_shortcuts.js | leaked until shutdown [nsGlobalWindow #9368 about:blank]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_window_shortcuts.js | leaked until shutdown [nsGlobalWindow #9386 about:blank]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_window_shortcuts.js | leaked until shutdown [nsGlobalWindow #9378 about:blank]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_window_shortcuts.js | leaked until shutdown [nsGlobalWindow #9376 about:blank]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_window_shortcuts.js | leaked until shutdown [nsGlobalWindow #9372 chrome://browser/content/devtools/framework/toolbox.xul]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_window_shortcuts.js | leaked until shutdown [nsGlobalWindow #9385 about:blank]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_window_shortcuts.js | leaked until shutdown [nsGlobalWindow #9369 about:blank]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/framework/test/browser_toolbox_window_shortcuts.js | leaked until shutdown [nsGlobalWindow #9387 about:blank]
Component: Developer Tools → Developer Tools: Framework
OS: Windows 7 → All
Assignee: nobody → scrapmachines
Seems like the leak is not there[1] when 'Profiler' is not checked for shortcut.

This could mean that the leak is in profiler opening and closing very quickly.

@Paul, should I go ahead and land this change, which will mean that we will not test profiler shortcuts ..

@Anton, can you help me with these leaks here ?

[1] https://tbpl.mozilla.org/?tree=Try&rev=0454d58a8f17
(In reply to Girish Sharma [:Optimizer] from comment #13)
> @Paul, should I go ahead and land this change, which will mean that we will
> not test profiler shortcuts ..

Which change?
(In reply to Paul Rouget [:paul] from comment #14)
> (In reply to Girish Sharma [:Optimizer] from comment #13)
> > @Paul, should I go ahead and land this change, which will mean that we will
> > not test profiler shortcuts ..
> 
> Which change?

Removing the profiler from the list of tools for which the shortcut will be checked. re: https://hg.mozilla.org/try/rev/bc88ce7a203e

When I removed the profiler from the list, the profiler was not opened during the test and the leaks stopped.
(In reply to Girish Sharma [:Optimizer] from comment #15)
> (In reply to Paul Rouget [:paul] from comment #14)
> > (In reply to Girish Sharma [:Optimizer] from comment #13)
> > > @Paul, should I go ahead and land this change, which will mean that we will
> > > not test profiler shortcuts ..
> > 
> > Which change?
> 
> Removing the profiler from the list of tools for which the shortcut will be
> checked. re: https://hg.mozilla.org/try/rev/bc88ce7a203e
> 
> When I removed the profiler from the list, the profiler was not opened
> during the test and the leaks stopped.

Works for me. Please file a bug to re-introduce the profiler to this test (and cc Anton).

Thank you!
Filed bug 845752.
Blocks: 845752
Attached patch remove profiler. (obsolete) (deleted) — — Splinter Review 
Removed Profiler from the list of tools to test. Added comments directing to this and the bug which re-enables it.
Attachment #719012 - Flags: review?(paul)
Attachment #719012 - Flags: review?(paul) → review?(jwalker)
Attachment #719012 - Flags: review?(jwalker) → review+
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/a15f45e617a4
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a15f45e617a4
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 22
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Hmm, so maybe the leak is not *just* profiler related, but the whole toolbox, or the whole remote protocol related.

bug 839280 seems to target hunting down leaks, lets see if it results to something good. Because doing some workarounds in this test to avoid leak is *not* the right way, and this test on its own does not seem to be the cause of the leak.
Joe, or anyone else that might have a clue ..

Should I wait for some time between each shortcut press, so that everything initiates correctly, and then when I close the toolbox, I wait some more, so that everything gets destroyed correctly ? (to try to avoid this leaking)
Flags: needinfo?
(In reply to Girish Sharma [:Optimizer] from comment #33)
> Joe, or anyone else that might have a clue ..
> 
> Should I wait for some time between each shortcut press, so that everything
> initiates correctly, and then when I close the toolbox, I wait some more, so
> that everything gets destroyed correctly ? (to try to avoid this leaking)

(a needinfo without anyone specified is automatically cancelled once the next person replies - which in this case was the ever so helpful tbplbot)
Flags: needinfo?(jwalker)
Previously, it used to happen on many different kind of machines, but now it is opnly happening on OSX Lion 10.8 opt.

Is that due to the frequency, or there is some pattern now ?
Depends on: 833429
I am going to go ahead and bypass this test on mac OSX 10.8 . Which I will re-enable once the toolbox leak is fixed (being tracked in bug 839280, bug 845752 and bug 833429)
Attached patch disable test on windows xp and mac os x 10.8 (obsolete) (deleted) — — Splinter Review 
need a quick review to calm down the orange :)
Attachment #719012 - Attachment is obsolete: true
Attachment #724041 - Flags: review?(paul)
Attachment #724041 - Flags: review?(jwalker)
Comment on attachment 724041 [details] [diff] [review]
disable test on windows xp and mac os x 10.8

Can you do that in the Makefile.in instead? (see browser/base/tests/Makefile.in for examples).
Attachment #724041 - Flags: review?(paul)
Attachment #724041 - Flags: review?(jwalker)
Maybe Ed knows better what the right way to disable this.
Flags: needinfo?(emorley)
You can't do that in the makefile, because the makefile/build/packaging of the tests happens on 10.7 and, um, Windows Server 2008 or something odd like that. If you want anything other than "all Windows" or "all OS X," you have to do it like that patch does.
Flags: needinfo?(emorley)
Attachment #724041 - Flags: review+
Can someone land this directly on inbound ?
Flags: needinfo?(jwalker)
Whiteboard: [leave-open]
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/564ddd109940
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/564ddd109940
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Okay, then we will re-enable this test for lion and xp in another bug :)
Whiteboard: [leave-open]
(In reply to Girish Sharma [:Optimizer] from comment #86)
> Okay, then we will re-enable this test for lion and xp in another bug :)

Filed that bug?
Seems like the check is not working on mac osx 10.8, but it is working on windows xp and the test is being bypassed on xp.
Even though I asked for a real navigator.oscpu from a person on mac osx 10.8 :/
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Girish Sharma [:Optimizer] from comment #97)
> Seems like the check is not working on mac osx 10.8, but it is working on
> windows xp and the test is being bypassed on xp.
> Even though I asked for a real navigator.oscpu from a person on mac osx 10.8
> :/

I believe there should be a space between the 'os' and 'x'.

However, looking in the tree, a more commonly used pattern for things like this is:
http://mxr.mozilla.org/mozilla-central/search?string=navigator.userAgent.indexOf
Ok, I will update the patch soon.
Attached patch disabling this time for sure. (deleted) — — Splinter Review 
Fixed the matching condition, as per what other tests are doing.
Attachment #724041 - Attachment is obsolete: true
Depends on: 851129
Keywords: checkin-needed
In the previous comment, the bypass happened on windows XP, but not on mac again. I is bypassing on other trees and at other times, why did it not bypass this time ?
(In reply to Girish Sharma [:Optimizer] from comment #109)
> In the previous comment, the bypass happened on windows XP, but not on mac
> again. I is bypassing on other trees and at other times, why did it not
> bypass this time ?

The patch is working on inbound, it just needs to merge to the other trees. (Though from IRC I gather you've since spotted this :-))
https://hg.mozilla.org/mozilla-central/rev/6347b649cc74
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Does this need a beta uplift ? or otherwise, release branch will show these leaks in next cycle . (Should probably have thought of aurora uplift in last cycle).
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: