Closed
Bug 798165
Opened 12 years ago
Closed 12 years ago
Purple buffer support for nsTimeout
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
Attachments
(1 file, 3 obsolete files)
10.33 KB,
patch
|
smaug
:
review+
froydnj
:
feedback+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•12 years ago
|
||
Assignee: nobody → continuation
Assignee | ||
Comment 2•12 years ago
|
||
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. ;)
Attachment #668268 -
Flags: feedback?(bugs)
Comment 3•12 years ago
|
||
Surely you meant "beautiful ain't it". Right? Right? \o/
Assignee | ||
Comment 4•12 years ago
|
||
Once more with null checks...
https://tbpl.mozilla.org/?tree=Try&rev=8d5fdb302200
Comment 5•12 years ago
|
||
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.
Attachment #668268 -
Flags: feedback?(bugs) → feedback+
Comment 6•12 years ago
|
||
Also, would be good to add a comment the traverse and unlink
Assignee | ||
Comment 7•12 years ago
|
||
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.
Comment 8•12 years ago
|
||
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.
Assignee | ||
Updated•12 years ago
|
Blocks: IncrementalCC
Assignee | ||
Comment 10•12 years ago
|
||
Maybe this will work...
https://tbpl.mozilla.org/?tree=Try&rev=1e4cc6a5c287
Attachment #668268 -
Attachment is obsolete: true
Assignee | ||
Comment 11•12 years ago
|
||
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
Attachment #742738 -
Attachment is obsolete: true
Assignee | ||
Comment 12•12 years ago
|
||
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
Attachment #742817 -
Attachment is obsolete: true
Assignee | ||
Comment 13•12 years ago
|
||
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.
Attachment #742839 -
Flags: review?(bugs)
Attachment #742839 -
Flags: feedback?(nfroyd)
Assignee | ||
Comment 14•12 years ago
|
||
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 15•12 years ago
|
||
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...
Attachment #742839 -
Flags: feedback?(nfroyd) → feedback+
Comment 16•12 years ago
|
||
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?
Assignee | ||
Comment 17•12 years ago
|
||
(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.)
Comment 18•12 years ago
|
||
(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 19•12 years ago
|
||
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.)
Attachment #742839 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 20•12 years ago
|
||
Comment 21•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•