I've got a bad feeling that the XP-only Moth leak in bug 792215 is caused by nsTimeout being our last non-purple-buffered cycle-collected class. My attempt to fix this the right way lives over in bug 774874, but that's kind of died on the shoals of weird problems due to nsTimeout being an old crusty class. But... I just thought of a hacky way we could work around this problem: any time we Release() an nsTimeout, instead of adding it to the purple buffer (and having to deal with adding all the support that this requires), we could AddRef() and Release() every cycle collected thing the nsTimeout holds onto, which will add them to the purple buffer. Probably not an awesome real solution but if this stops the leaks in XP, then we at least know what is going on... https://tbpl.mozilla.org/?tree=Try&rev=441748420832

Comment on attachment 668268 [details] [diff] [review] beautiful it ain't I'm not looking for feedback on whether this is nice, but rather if you think it will fix a possible leak caused by nsTimeout not being a purple buffered class. ;)

Surely you meant "beautiful ain't it". Right? Right? \o/

Once more with null checks... https://tbpl.mozilla.org/?tree=Try&rev=8d5fdb302200

Comment on attachment 668268 [details] [diff] [review] beautiful it ain't Lovely. Perhaps the comment should say something about the crazy nsTimeout usage as a stack variable.

Also, would be good to add a comment the traverse and unlink

Unfortunately, this didn't fix the leak in bug 792215. The leak may be a few MB smaller, but it is still really huge, and that could just be random. We may want to land something like this anyways.

I think I may need to fix nsTimeout to work like the rest of cycle collectable objects before doing the planned addref/release changes. But that won't happen for FF18, so maybe we could land this for FF18.

Maybe this will work... https://tbpl.mozilla.org/?tree=Try&rev=1e4cc6a5c287

Well, that was a total failure. The problem is that dummy_timeout gets put into the purple buffer, dies when we exit that method, then later the CC touches the purple buffer entry for dummy_timeout and badness ensues. I tried a few unsuccessful approaches, but this one at least passes the document navigation mochitests, and seems like the cleanest. I define a new StackRelease() method that basically does nsTimeout::Release() over and over again until the refcount drops to zero, except for the delete, which is naturally handled when we leave the function. This is nice because it doesn't have to be updated if the definition of decr() changes. It isn't pretty, but on the whole I think the patch still makes nsTimeout a less odd class. Then I wrap it up in an RAII class to ensure that it is called when dummy_timeout goes out of scope. my new less ambitious try run: https://tbpl.mozilla.org/?tree=Try&rev=1da28e4ab7f3

Actually, come to think of it, it is my impression that the two addrefs are for 1. keep it alive in the method 2. putting it in the list In the early return case, it is not in the list, so we have exactly one reference, and in the regular case, we have exactly two references. Thus we can just turn dummy_timeout into a normal heap allocated class kept alive with an nsRefPtr, and avoid bizarre (though, in my humble opinion, awesome) machinery for a stack allocated CCed class. Try run was green: https://tbpl.mozilla.org/?tree=Try&rev=301de2f3e865

Comment on attachment 742839 [details] [diff] [review] make nsTimeout a more normal CCed class Review of attachment 742839 [details] [diff] [review]: ----------------------------------------------------------------- Nathan, if you could check that the AddRef/Release stuff for dummy_timeout is doing, I'd appreciate it, as I'm really just implementing your suggestion from bug 774874.

I made timeout a for-scoped variable, because the existing setup was confusing me, but I can change that back if you'd like, Olli.

Comment on attachment 742839 [details] [diff] [review] make nsTimeout a more normal CCed class I think this patch has the correct releasing behavior. Someday maybe the list management calls will automagically DTRT and we can banish the manual AddRef/Release bits...

Comment on attachment 742839 [details] [diff] [review] make nsTimeout a more normal CCed class Review of attachment 742839 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsGlobalWindow.cpp @@ +10368,5 @@ > + nsRefPtr<nsTimeout> dummy_timeout = new nsTimeout(); > + dummy_timeout->mFiringDepth = firingDepth; > + dummy_timeout->mWhen = now; > + last_expired_timeout->setNext(dummy_timeout); > + dummy_timeout->AddRef(); Use a second nsRefPtr?

(In reply to :Ms2ger from comment #16) > Use a second nsRefPtr? That's what I had first when I tried this, a few months ago, but Nathan pointed out there's an early return that doesn't do the release, so you can't use a smart pointer. We really just need something akin to LinkedList<RefPtr<Foo>>, as that's all the AddRef() and Release() stuff is doing. (Obviously that won't literally work, but that's the idea.)

(In reply to :Ms2ger from comment #16) > ::: dom/base/nsGlobalWindow.cpp > @@ +10368,5 @@ > > + nsRefPtr<nsTimeout> dummy_timeout = new nsTimeout(); > > + dummy_timeout->mFiringDepth = firingDepth; > > + dummy_timeout->mWhen = now; > > + last_expired_timeout->setNext(dummy_timeout); > > + dummy_timeout->AddRef(); > > Use a second nsRefPtr? That doesn't work, because this AddRef is not necessarily matched by the Release in this method. The matching Release is whenever dummy_timeout is removed from the timeout list, which might be done by ClearAllTimeouts. At least, that's how I'm reading the code...

Comment on attachment 742839 [details] [diff] [review] make nsTimeout a more normal CCed class Could you add an assertion to if (timeout_was_cleared) that refcnt is 1. (You may need to add some method to nsTimeout to give back refcnt.)