Closed Bug 557120 Opened 11 years ago Closed 10 years ago

[SeaMonkey 2.1] mochitest-plain-1: it seems test_bug457672.html "leaks the world"


(SeaMonkey :: Tabbed Browser, defect)

Not set


(Not tracked)



(Reporter: sgautherie, Assigned: neil)


(Blocks 1 open bug, )


(Keywords: memory-leak)


(2 files, 2 obsolete files)

Blocks: 541235
Depends on: 557060
Blocks: SmTestLeak
No longer blocks: SmTestFail
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?
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.
Assignee: nobody → kairo
Attachment #450885 - Flags: review?(Olli.Pettay)
smaug, ping?
Is there a bug filed to fix the leak properly?
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.
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. :(
I just ran this patch through try today; and it passed all tests (after accounting for known intermittent; and our predictable Mac64 Bd issue)
Attachment #450885 - Flags: review?(Olli.Pettay) → review+
Blocks: 576299
I filed the followup for creating the test and investigating the SeaMonkey leak as bug 576299.
Closed: 11 years ago
Resolution: --- → FIXED
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.
Resolution: FIXED → ---
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.
Assignee: kairo → nobody
Component: DOM: Events → Tabbed Browser
Product: Core → SeaMonkey
QA Contact: events → tabbed-browser
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
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.
Sorry, the pref is browser.tabs.max_tabs_undo on trunk, I looked at the wrong tree.
I tested it with a debug build (Mozilla/5.0 (X11; Linux x86_64; rv:2.0b5pre) Gecko/20100818 SeaMonkey/2.1b1pre, built from 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
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.
If browser.sessionstore.max_tabs_undo is > 0 I get a leak, regardless of the value browser.tabs.max_tabs_undo has.
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).
Replacing with 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.
It's best to start from here:

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.
Depends on: 530735
(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.
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...
Attached patch Proposed patch (obsolete) — Splinter Review
Attachment #467139 - Flags: review?(iann_bugzilla)
Attachment #467139 - Flags: feedback?
I failed to guess Misak's bugmail address again, so anyone can feed back ;-)
Attachment #467139 - Flags: feedback? → feedback?(misak.bugzilla)
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.
Attachment #467139 - Flags: review?(iann_bugzilla) → review+
Attached patch Alternative patch (obsolete) — Splinter Review
Attachment #467224 - Flags: review?(iann_bugzilla)
Attachment #467224 - Flags: feedback?(misak.bugzilla)
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.
Attachment #467224 - Flags: feedback-
Attachment #467139 - Flags: review+
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?
Attachment #467224 - Flags: review?(iann_bugzilla) → review-
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.
Attachment #467139 - Flags: feedback?(misak.bugzilla) → feedback+
Assignee: nobody → neil
Attachment #467139 - Attachment is obsolete: true
Attachment #467224 - Attachment is obsolete: true
Attachment #467352 - Flags: review?(bugspam.Callek)
Attachment #467224 - Flags: feedback?(misak.bugzilla)
Attachment #467352 - Flags: review?(bugspam.Callek) → review+
Pushed changeset 6cdb4d209658 to comm-central.
Closed: 11 years ago10 years ago
Resolution: --- → FIXED
Attachment #450885 - Attachment description: only close when focusing worked → only close when focusing worked [Checkin: Comment 9]
Attachment #467352 - Attachment description: Addressed review comments → Addressed review comments [Checkin: Comment 31]
No longer blocks: 541235
Depends on: 541235
Flags: in-testsuite-
Target Milestone: --- → seamonkey2.1b1
Duplicate of this bug: 576299
No longer blocks: 576299
You need to log in before you can comment on or make changes to this bug.