The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in seamonkey2.1b1

Status

SeaMonkey
Tabbed Browser
--
major
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: sgautherie, Assigned: neil@parkwaycc.co.uk)

Tracking

(Blocks: 1 bug, {mlk})

Trunk
seamonkey2.1b1
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

7 years ago
{
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
}
(Reporter)

Updated

7 years ago
Blocks: 541235
Depends on: 557060
(Reporter)

Updated

7 years ago
Blocks: 448125
No longer blocks: 452942

Comment 1

7 years ago
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

7 years ago
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.
Assignee: nobody → kairo
Status: NEW → ASSIGNED
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.

Comment 6

7 years ago
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+

Updated

7 years ago
Blocks: 576299

Comment 8

7 years ago
I filed the followup for creating the test and investigating the SeaMonkey leak as bug 576299.
http://hg.mozilla.org/mozilla-central/rev/bccbd4148ea1
Status: ASSIGNED → RESOLVED
Last Resolved: 7 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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 11

7 years ago
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.

Updated

7 years ago
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

Comment 13

7 years ago
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

7 years ago
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 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
(Assignee)

Comment 16

7 years ago
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 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

7 years ago
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.
Depends on: 530735
(Assignee)

Comment 21

7 years ago
(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.
(Assignee)

Comment 22

7 years ago
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...
(Assignee)

Comment 23

7 years ago
Created attachment 467139 [details] [diff] [review]
Proposed patch
Attachment #467139 - Flags: review?(iann_bugzilla)
Attachment #467139 - Flags: feedback?
(Assignee)

Comment 24

7 years ago
I failed to guess Misak's bugmail address again, so anyone can feed back ;-)
(Reporter)

Updated

7 years ago
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+
(Assignee)

Comment 26

7 years ago
Created attachment 467224 [details] [diff] [review]
Alternative patch
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-

Updated

7 years ago
Attachment #467139 - Flags: review+

Comment 28

7 years ago
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 29

7 years ago
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)

Comment 30

7 years ago
Created attachment 467352 [details] [diff] [review]
Addressed review comments
[Checkin: Comment 31]
Assignee: nobody → neil
Attachment #467139 - Attachment is obsolete: true
Attachment #467224 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Attachment #467352 - Flags: review?(bugspam.Callek)
Attachment #467224 - Flags: feedback?(misak.bugzilla)

Updated

7 years ago
Attachment #467352 - Flags: review?(bugspam.Callek) → review+
(Assignee)

Comment 31

7 years ago
Pushed changeset 6cdb4d209658 to comm-central.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
(Reporter)

Updated

7 years ago
Attachment #450885 - Attachment description: only close when focusing worked → only close when focusing worked [Checkin: Comment 9]
(Reporter)

Updated

7 years ago
Attachment #467352 - Attachment description: Addressed review comments → Addressed review comments [Checkin: Comment 31]
(Reporter)

Updated

7 years ago
No longer blocks: 541235
Depends on: 541235
(Reporter)

Updated

7 years ago
Flags: in-testsuite-
Target Milestone: --- → seamonkey2.1b1

Updated

7 years ago
Duplicate of this bug: 576299

Updated

7 years ago
No longer blocks: 576299
You need to log in before you can comment on or make changes to this bug.