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

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Description

[Serge Gautherie (:sgautherie)]

{
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]

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]

Attachment: 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)]

smaug, ping?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Is there a bug filed to fix the leak properly?

Comment 5

[Justin Wood (:Callek)]

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]

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)]

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]

I filed the followup for creating the test and investigating the SeaMonkey leak as bug 576299.

Comment 9

[Justin Wood (:Callek)]

http://hg.mozilla.org/mozilla-central/rev/bccbd4148ea1

Comment 10

[Justin Wood (:Callek)]

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]

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)]

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]

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]

Sorry, the pref is browser.tabs.max_tabs_undo on trunk, I looked at the wrong tree.

Comment 15

[Bruno 'Aqualon' Escherl]

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

[[e-mail]]

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]

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]

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]

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]

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

[[e-mail]]

(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

[[e-mail]]

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

[[e-mail]]

Attachment: Proposed patch

Comment 24

[[e-mail]]

I failed to guess Misak's bugmail address again, so anyone can feed back ;-)

Comment 25

[Justin Wood (:Callek)]

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

[[e-mail]]

Attachment: Alternative patch

Comment 27

[Justin Wood (:Callek)]

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]

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]

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

[[e-mail]]

Attachment: Addressed review comments [Checkin: Comment 31]

Comment 31

[[e-mail]]

Pushed changeset 6cdb4d209658 to comm-central.