Last Comment Bug 557120 - [SeaMonkey 2.1] mochitest-plain-1: it seems test_bug457672.html "leaks the world"
: [SeaMonkey 2.1] mochitest-plain-1: it seems test_bug457672.html "leaks the wo...
Status: RESOLVED FIXED
: mlk
Product: SeaMonkey
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
-- major (vote)
: seamonkey2.1b1
Assigned To: neil@parkwaycc.co.uk
:
:
Mentors:
http://mxr.mozilla.org/mozilla-centra...
: 576299 (view as bug list)
Depends on: 457672 530735 541235 557060
Blocks: SmTestLeak
  Show dependency treegraph
 
Reported: 2010-04-04 11:14 PDT by Serge Gautherie (:sgautherie)
Modified: 2010-08-26 15:50 PDT (History)
6 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
only close when focusing worked [Checkin: Comment 9] (454 bytes, patch)
2010-06-12 15:17 PDT, Robert Kaiser
bugs: review+
Details | Diff | Splinter Review
Proposed patch (2.15 KB, patch)
2010-08-18 13:34 PDT, neil@parkwaycc.co.uk
bugspam.Callek: review+
iann_bugzilla: review+
misak.bugzilla: feedback+
Details | Diff | Splinter Review
Alternative patch (770 bytes, patch)
2010-08-18 16:30 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review-
bugspam.Callek: feedback-
Details | Diff | Splinter Review
Addressed review comments [Checkin: Comment 31] (3.30 KB, patch)
2010-08-19 02:17 PDT, neil@parkwaycc.co.uk
bugspam.Callek: review+
Details | Diff | Splinter Review

Description User image Serge Gautherie (:sgautherie) 2010-04-04 11:14:38 PDT
{
Leaked URLs:
  https://bugzilla.mozilla.org/show_bug.cgi?id=457672
  http://mochi.test:8888/tests/content/events/test/test_bug457672.html
  http://mochi.test:8888/tests/content/events/test/test_bug457672.html#

TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1263904 bytes during test execution
}
Comment 1 User image Robert Kaiser 2010-06-12 14:38:59 PDT
For testing, I have replaced |window.close();| with |window.setTimeout('window.close()', 10);| in bug457672.html and that seems to fix the leaks.
It's been taken me a long run of trial and error to get there, but setTimeout() is a bad thing in tests, is there a good alternative?
Comment 2 User image Robert Kaiser 2010-06-12 15:17:14 PDT
Created attachment 450885 [details] [diff] [review]
only close when focusing worked
[Checkin: Comment 9]

This patch fixes it for me. The change is to only close the window/tab once it actually got the focus event from the done() function.
Comment 3 User image Justin Wood (:Callek) [away until Feb 27] 2010-06-19 21:09:18 PDT
smaug, ping?
Comment 4 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2010-06-20 01:31:50 PDT
Is there a bug filed to fix the leak properly?
Comment 5 User image Justin Wood (:Callek) [away until Feb 27] 2010-06-20 01:36:08 PDT
Since this is only leaking on SeaMonkey, I'm not certain on enough information to file a suitable bug. Or if it is purely a SM-Side code leak. But if this test fix fixes it I'd love to take it.
Comment 6 User image Robert Kaiser 2010-06-20 05:17:32 PDT
It may be a good idea to file a bug, but I'm not completely sure if it is an actual SeaMonkey bug or where it might be (I'd suspect tabbrowser, but I have no clue how that one works or if it could be a problem).
My target is first to get all tests to go green, which we only have been working on since about half a year now, and because of that, I really start getting tired of those perma-oranges. :(
Comment 7 User image Justin Wood (:Callek) [away until Feb 27] 2010-06-29 19:26:17 PDT
I just ran this patch through try today; and it passed all tests (after accounting for known intermittent; and our predictable Mac64 Bd issue)
Comment 8 User image Robert Kaiser 2010-07-01 08:47:30 PDT
I filed the followup for creating the test and investigating the SeaMonkey leak as bug 576299.
Comment 9 User image Justin Wood (:Callek) [away until Feb 27] 2010-07-01 09:02:39 PDT
http://hg.mozilla.org/mozilla-central/rev/bccbd4148ea1
Comment 10 User image Justin Wood (:Callek) [away until Feb 27] 2010-07-01 12:50:47 PDT
In the brief window before the Component Registration changes landed, I was able to verify that this patch did not fix the leak. I did not choose to backout the test change though.
Comment 11 User image Robert Kaiser 2010-07-27 12:04:04 PDT
Seems like the leaking expanded to other tests now with the switch to tabbed browsing by default. I suspect that the same leak is now spreading to more places.
Comment 12 User image Justin Wood (:Callek) [away until Feb 27] 2010-08-18 00:51:41 PDT
Ok of note:

Load SM->Ctrl+t->Close SeaMonkey ==> Does NOT leak
Load SM->Ctrl+t->Close the new tab->Close SeaMonkey ==> LEAKS
Load SM->Ctrl+t->Close tab->Try to undo-closed-tab->Close SeaMonkey ==> Does NOT leak
Comment 13 User image Robert Kaiser 2010-08-18 04:29:24 PDT
Callek: if you disable saving tabs for undo (i.e. set browser.sessionstore.max_tabs_undo to 0), does it still leak? I think I tested once and it did, but just want to make sure.
Comment 14 User image Robert Kaiser 2010-08-18 04:52:44 PDT
Sorry, the pref is browser.tabs.max_tabs_undo on trunk, I looked at the wrong tree.
Comment 15 User image Bruno 'Aqualon' Escherl 2010-08-18 06:14:48 PDT
I tested it with a debug build (Mozilla/5.0 (X11; Linux x86_64; rv:2.0b5pre) Gecko/20100818 SeaMonkey/2.1b1pre, built from http://hg.mozilla.org/mozilla-central/rev/ca457b5758e0). STR were load SM, open new tab, load website, close tab, close SM

browser.sessionstore.max_tabs_undo = 10

 => mAllocCount:          77912
 => mReallocCount:         6641
 => mFreeCount:           71033  --  LEAKED 6879 !!!
 => mShareCount:          82009
 => mAdoptCount:           5073
 => mAdoptFreeCount:       5071  --  LEAKED 2 !!!

browser.sessionstore.max_tabs_undo = 0

 => mAllocCount:          52051
 => mReallocCount:         5582
 => mFreeCount:           52050  --  LEAKED 1 !!!
 => mShareCount:          63576
 => mAdoptCount:           4864
 => mAdoptFreeCount:       4864
Comment 16 User image neil@parkwaycc.co.uk 2010-08-18 07:12:10 PDT
Actually we have two prefs, so we need to check with them a) both set to default b) each set to 0 individually c) both set to 0.
Comment 17 User image Bruno 'Aqualon' Escherl 2010-08-18 07:32:25 PDT
If browser.sessionstore.max_tabs_undo is > 0 I get a leak, regardless of the value browser.tabs.max_tabs_undo has.
Comment 18 User image Bruno 'Aqualon' Escherl 2010-08-18 07:35:00 PDT
Oh, and if browser.sessionstore.max_tabs_undo is 0 I don't get a leak (tested with browser.tabs.max_tabs_undo = 3 (default) and browser.tabs.max_tabs_undo = 0).
Comment 19 User image Bruno 'Aqualon' Escherl 2010-08-18 08:59:13 PDT
Replacing http://mxr.mozilla.org/comm-central/source/suite/common/src/nsSessionStore.js?mark=689-699#689 with http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js?mark=817-833#817 gets rid of the leak, but changes the behavior of undo, opening the tabs as first tab not as last.

CCing misak to the bug since he should know sessionstore best.
Comment 20 User image Misak Khachatryan 2010-08-18 09:22:01 PDT
It's best to start from here:
https://bugzilla.mozilla.org/show_bug.cgi?id=530735#c14

I was unsure that leaks are not local to my pc, so didn't insist. CCing Neil, as the author of idea of combined undoclosetab mechanism. He can suggest the way to go.
Comment 21 User image neil@parkwaycc.co.uk 2010-08-18 12:53:19 PDT
(In reply to comment #17)
> If browser.sessionstore.max_tabs_undo is > 0 I get a leak, regardless of the
> value browser.tabs.max_tabs_undo has.

(In reply to comment #18)
> Oh, and if browser.sessionstore.max_tabs_undo is 0 I don't get a leak (tested
> with browser.tabs.max_tabs_undo = 3 (default) and browser.tabs.max_tabs_undo =
> 0).

Well, this seems to suggest the problem lies entirely with session store, as setting browser.tabs.max_tabs_undo completely disables the bfundo mechanism.
Comment 22 User image neil@parkwaycc.co.uk 2010-08-18 13:26:38 PDT
OK, so it is my fault.

I create a tabData object in the tabbrowser to hold the title etc.

But session store persists the tabData so that it can undo the tab close.

And the tabData object's global is the window, so that persists too...
Comment 23 User image neil@parkwaycc.co.uk 2010-08-18 13:34:42 PDT
Created attachment 467139 [details] [diff] [review]
Proposed patch
Comment 24 User image neil@parkwaycc.co.uk 2010-08-18 13:39:12 PDT
I failed to guess Misak's bugmail address again, so anyone can feed back ;-)
Comment 25 User image Justin Wood (:Callek) [away until Feb 27] 2010-08-18 16:02:43 PDT
Comment on attachment 467139 [details] [diff] [review]
Proposed patch

>diff --git a/suite/browser/tabbrowser.xml b/suite/browser/tabbrowser.xml
>--- a/suite/browser/tabbrowser.xml
> 
>+            aTab.tabData = {};
>             event.initEvent("TabClose", true, false);
>             aTab.dispatchEvent(event);
>+            aTab.tabData.pos = this.getTabIndex(aTab);


Nit move the object init to below the dispatchEvent with an

if (aTab.tabData instanceof Object)
  aTab.tabData = {}; //create the object if Session Store did not

This way we can avoid the odd global-object issue and not make sessionstore pretend it has to override something, where its not doing anything bad.

>+++ b/suite/common/src/nsSessionStore.js

>+      // Overwrite the tabbrowser's tab data.
>+      aTab.tabData = { state: tabState };

With my nit above, no need for this comment at all.
Comment 26 User image neil@parkwaycc.co.uk 2010-08-18 16:30:54 PDT
Created attachment 467224 [details] [diff] [review]
Alternative patch
Comment 27 User image Justin Wood (:Callek) [away until Feb 27] 2010-08-18 16:36:00 PDT
Comment on attachment 467224 [details] [diff] [review]
Alternative patch

This looks too hacky to me, and too prone for a "hmm, why don't we _just_ add the state property here, rather than do this as a whole new object, since everything else is already on aTab.tabData" and thus too easy to recreate the leak.
Comment 28 User image Ian Neal 2010-08-18 18:15:19 PDT
Comment on attachment 467224 [details] [diff] [review]
Alternative patch

Having had discussions with Callek on IRC, I am persuaded the other patch is better, but if this is the one gone for then would using let rather than var be better here?
Comment 29 User image Misak Khachatryan 2010-08-18 23:55:51 PDT
Comment on attachment 467139 [details] [diff] [review]
Proposed patch

Unfortunately my build can't finish all the tests and report leaks, but all sessionstore related tests are passing with this patch, no functionality break noticed.
Comment 30 User image neil@parkwaycc.co.uk 2010-08-19 02:17:46 PDT
Created attachment 467352 [details] [diff] [review]
Addressed review comments
[Checkin: Comment 31]
Comment 31 User image neil@parkwaycc.co.uk 2010-08-19 16:35:50 PDT
Pushed changeset 6cdb4d209658 to comm-central.
Comment 32 User image Robert Kaiser 2010-08-26 15:49:56 PDT
*** Bug 576299 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.