774874 - Clean up nsTimeout a little

Status: RESOLVED DUPLICATE of bug 798165

(Core :: DOM: Core & HTML, defect)

Description

[Andrew McCreight [:mccr8]]

This class has a few weird aspects that make it hard to convert to a suspected native cycle collected class in bug 750570.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: clean up constructor, destructor, other stuff

- Explicitly initialize fields instead of memset.  This is pretty questionable, blocks turning this into a normal refcounted class, and jst said he didn't think it would affect performance.
- Move code to cancel the time from Release() to the destructor.  Seems weird to have here, and this blocks turning the class into a normal refcounted class.
- For some reason the definition of nsTimeout::AddRef() and Release() are far away from the rest of the definitions needed for nsTimeout, so I moved them closer.
- Move the class into nsGlobalWindow.cpp, as it isn't used anywhere else. To do this, I had to move the definition of nsGlobalWindow::FirstTimeout() and LastTimeout() into the .cpp file, with the "inline" keyword.  This part is optional, but it seems nicer to me.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: part 2: make nsTimeout use normal ref counting declarations

This make makes nsTimeout use the normal IMPL_ADDREF and RELEASE stuff.  This changes the behavior slightly, in that it is now stabilized during destruction.  It also adds thread safety checks, ref count logging, and some sanity assertions.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Andrew McCreight [:mccr8] from comment #1)
> Created attachment 643154 [details] [diff] [review]
> clean up constructor, destructor, other stuff
> 
> - Move the class into nsGlobalWindow.cpp, as it isn't used anywhere else.

I'd rather you didn't, nsGlobalWindow.cpp is huge already.

Comment 4

[Andrew McCreight [:mccr8]]

(In reply to :Ms2ger from comment #3)
> I'd rather you didn't, nsGlobalWindow.cpp is huge already.

I could make a new nsTimeout.h and nsTimeout.cpp if that would be an improvement over the current situation, then include the header in nsGlobalWindow.cpp.

Also, this patch appears to leak thousands of timeouts...

Comment 5

[Andrew McCreight [:mccr8]]

Looking at some ref count logs, the problem with the leaking nsTimeouts is dummy_timeout, from nsGlobalWindow::RunTimeout.  It is stack allocated, but then it is manually addref'ed twice, apparently to keep "ClearWindowTimeouts" (I assume this means ClearAllTimeouts) from throwing the dummy away.  At the end of the function, we run the destructor, but never release it.  This only doesn't show up right now because we don't currently log the addrefs or releases at all.  I'm not really sure what the best way to fix this is.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 1: move nsTimeout to its own file

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: part 2: clean up nsTimeout constructor, destructor, release, unlink.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: part 1: move nsTimeout to its own file

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: part 3: malloc dummy_timeout

This may be controversial, but it seems sketchy to me to allocate a ref counted object on the stack.  This patch changes dummy_timeout to use new instead.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: part 4: use normal ref counting declarations

Comment 11

[Andrew McCreight [:mccr8]]

To switch nsTimeout to be usable in the purple buffer, we either have to heap allocate it, or add a flag to it that indicates whether it is allocated on the stack or on the heap and then create a modified version of the cycle collecting release that checks that flag and does not add it to the purple buffer if it is on the stack.

Of course, I could also just not convert it to use the purple buffer, which would leave open the possibility of leaks and require some simple custom macros for native cycle collected but not purple buffered objects.

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 647991 [details] [diff] [review]
part 3: malloc dummy_timeout

Review of attachment 647991 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsGlobalWindow.cpp
@@ +9713,5 @@
> +  dummy_timeout->mFiringDepth = firingDepth;
> +  dummy_timeout->mWhen = now;
> +
> +  PR_INSERT_AFTER(dummy_timeout, last_expired_timeout);
> +  dummy_timeout->AddRef();

I hate manual AddRefs; can this just be nsRefPtr<nsTimeout> kungFuDeathGrip = dummy_timeout;? Also, please keep the pointer to ClearWindowTimeouts.

Comment 13

[Andrew McCreight [:mccr8]]

(In reply to :Ms2ger (Back and backlogged) from comment #12)
> I hate manual AddRefs; can this just be nsRefPtr<nsTimeout> kungFuDeathGrip
> = dummy_timeout;? Also, please keep the pointer to ClearWindowTimeouts.
Can do.  I was just matching the existing "style".  There's also bug 306598, but that is a bit more involved than I want to deal with here.

Comment 14

[Andrew McCreight [:mccr8]]

Attachment: part 3: malloc dummy_timeout

I incorporated Ms2ger's suggestion, and added a PR_REMOVE_LINK(dummy_timeout) along an early return path, though I'm not sure if that is quite right.

Comment 15

[Andrew McCreight [:mccr8]]

Attachment: part 3: malloc dummy_timeout

Oops, forgot to remove the Release() when I switched over to kungFuDeathGrip.

I'm not 100% sure about the PR_REMOVE_LINK(dummy_timeout) that I added on the early return path. That changes the existing behavior, so maybe I should just leave it alone.

Comment 16

[Nathan Froyd [:froydnj]]

Comment on attachment 649284 [details] [diff] [review]
part 3: malloc dummy_timeout

Review of attachment 649284 [details] [diff] [review]:
-----------------------------------------------------------------

This seems subtly wrong with respect to clearing timeouts: when dummy_timeout gets cleared, it's going to have a manual Release called on it.  Suddenly the destructors for nsRefPtr are going to Release one too many times, causing trouble.

I think the kungFuDeathGrip needs to be a manual AddRef, with a manual Release in the normal exit path (i.e. *not* in the timeout_was_cleared path).  Still hate the manual reference counting, but small steps to using nsRefPtr everywhere...

Comment 17

[Andrew McCreight [:mccr8]]

Hmm. Okay. Something was definitely wrong with one of these patches, and I didn't really understand what all was happening here.

Comment 18

[Andrew McCreight [:mccr8]]

I don't think I'm going to work on this any more.  Looking at nsTimeout makes my head hurt.  I will try to figure out how to make it a proper CC class.  Hopefully that will work...

Comment 19

[Andrew McCreight [:mccr8]]

Part of this is that froydnj cleaned up the worst of the non-CC weirdness here.  Of course, there's still plenty of weirdness to go around...