664472 - tabview tests may leak

Status: RESOLVED WORKSFORME

(Firefox Graveyard :: Panorama, defect)

Description

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

Attachment: patch

I'm not happy with this patch, but I'm not familiar with nsSessionStore.js
Feel free to fix the problem in nsSessionStore.js ;)

Comment 1

[Dão Gottwald [:dao]]

This very much sounds like it needs to be fixed in nsSessionStore.js.

Comment 2

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

Dão, will bug 658738 fix this one too?

Comment 3

[Dão Gottwald [:dao]]

I hope it's the other way around: that fixing this bug kills some of the leaked windows logged in bug 658738.

Comment 4

[Dão Gottwald [:dao]]

Hm, I don't see how this patch would make the timer fire (or why it wouldn't fire in the first place) nor why it would keep things alive if it didn't fire.

Comment 5

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

The patch just makes sure the timer has time to fire before the testing page
goes away.
Pure hack, but I'm not at all familiar what all nsSessionStore.js tries to
do and where it keeps references to which elements or windows.

Comment 6

[Dão Gottwald [:dao]]

It doesn't keep references... The timer firing after the page going away really shouldn't be a problem.

Comment 7

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

Then I've misinterpret the code.
Somehow the timer in the patch in this bug makes the tests pass without leaking.

Comment 8

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

And btw, the only leaking test, when parentNode is strong ref, is 
browser_tabview_firstrun_pref.js

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

We have _lots_ of windows opened and closed in tabview tests and it's not clear to me why only browser_tabview_firstrun_pref.js is leaking. I don't know why the setTimeout() fixes the leak but IMHO it doesn't look like the right solution.

Comment 10

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

Comment on attachment 539571 [details] [diff] [review]
patch

Ok, someone (maybe me) needs to debug the leak some more.