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 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 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 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 Justin Wood (:Callek) 2010-06-19 21:09:18 PDT
smaug, ping?
Comment 4 Olli Pettay [:smaug] (TPAC) 2010-06-20 01:31:50 PDT
Is there a bug filed to fix the leak properly?
Comment 5 Justin Wood (:Callek) 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 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 Justin Wood (:Callek) 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 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 Justin Wood (:Callek) 2010-07-01 09:02:39 PDT
http://hg.mozilla.org/mozilla-central/rev/bccbd4148ea1
Comment 10 Justin Wood (:Callek) 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 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 Justin Wood (:Callek) 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 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 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 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 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 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 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 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 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 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 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 neil@parkwaycc.co.uk 2010-08-18 13:34:42 PDT
Created attachment 467139 [details] [diff] [review]
Proposed patch
Comment 24 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 Justin Wood (:Callek) 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 neil@parkwaycc.co.uk 2010-08-18 16:30:54 PDT
Created attachment 467224 [details] [diff] [review]
Alternative patch
Comment 27 Justin Wood (:Callek) 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 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 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 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 neil@parkwaycc.co.uk 2010-08-19 16:35:50 PDT
Pushed changeset 6cdb4d209658 to comm-central.
Comment 32 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.