1513615 - clean up timer event initialization

Status: RESOLVED FIXED

Description

[Nathan Froyd [:froydnj] [away through 21 July]]

Inspired by the crashes seen in bug 1489953.  It's possible that these cleanups
will just move the mysterious crashes someplace else, but they will also make
the code slightly cleaner.

Comment 1

[Nathan Froyd [:froydnj] [away through 21 July]]

Attachment: part 1 - remove null check on nsTimerEvent allocation

Infallible new means that this check was pointless.

We add the parentheses on the `new` expression because I can never remember
what the rules are when you leave off the parentheses, and because
approx. 99.99% of the codebase uses the parentheses anyway.

Comment 2

[Nathan Froyd [:froydnj] [away through 21 July]]

Attachment: part 2 - move some code around in PostTimerEvent

Doing this code movement separately will ideally make the next part of
this work easier to review.  The idea is that we want to extract all the
necessary information from `timer` before we pass ownership of it into
the newly-allocated nsTimerEvent.

Comment 3

[Nathan Froyd [:froydnj] [away through 21 July]]

Attachment: part 3 - do more work in nsTimerEvent's constructor

nsTimerEvent goes through a multi-step initialization for reasons that
are lost to time.  We are also seeing peculiar crashes in
`nsTimerEvent::SetTimer()` that are only explainable by `SetTimer`
finding a non-null pointer where there should have been a null pointer.
The compiler ought to have been able to optimize those bits away, but no
matter: we can do the job ourselves and make the code clearer.

Since we only call `SetTimer` once, we should just move its work into
nsTimerEvent's constructor.

Comment 4

[Nathan Froyd [:froydnj] [away through 21 July]]

Comment on attachment 9030815 [details] [diff] [review]
part 1 - remove null check on nsTimerEvent allocation

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

::: xpcom/threads/TimerThread.cpp
@@ -715,5 @@
>    // canceled.
>  
> -  RefPtr<nsTimerEvent> event = new nsTimerEvent;
> -  if (!event) {
> -    return timer.forget();

Argh, we're not using the system `new` here, but the timer event allocator, which *can* actually fail.  Bother.  Will have to rethink this patch.

Comment 5

[Nathan Froyd [:froydnj] [away through 21 July]]

Attachment: part 1 - tweak nsTimerEvent allocation

Unlike many of our uses of `new`, nsTimerEvent has its own definition of
`operator new`, to ensure instances are allocated through
TimerEventAllocator.  And allocating with TimerEventAllocator can fail.
Later changes, however, want to assume that constructing an nsTimerEvent
can't fail, which is difficult to guarantee with the current structure.

To make that guarantee, we need to make explicit what calling `new`
does: there's an "allocate memory" step and a "construct the object"
step.  The first part can fail, and that's what we care about here.
Once we have a chunk of memory, we can construct the object as normal,
secure in the knowledge that calling (placement) `new` is now guaranteed
to succeed.

Comment 6

[Nathan Froyd [:froydnj] [away through 21 July]]

Attachment: part 2 - move some code around in PostTimerEvent

Same as before, just rebased.

Comment 7

[Nathan Froyd [:froydnj] [away through 21 July]]

Attachment: part 3 - do more work in nsTimerEvent's constructor

Same as before, just rebased.

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 9030858 [details] [diff] [review]
part 1 - tweak nsTimerEvent allocation

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

::: xpcom/threads/TimerThread.cpp
@@ -713,5 @@
>    // the caller. We need to copy the generation number from this timer into the
>    // event, so we can avoid firing a timer that was re-initialized after being
>    // canceled.
>  
> -  RefPtr<nsTimerEvent> event = new nsTimerEvent;

dxr says this is the only caller of nsTimerEvent::nsTimerEvent. Instead of doing some placement-new hacks, why not use an Init method, like we use in many other places? (and while here, new nsTimerEvent() rather than new nsTimerEvent would make for less confusion)

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 9030859 [details] [diff] [review]
part 2 - move some code around in PostTimerEvent

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

::: xpcom/threads/TimerThread.cpp
@@ +732,5 @@
> +  RefPtr<nsTimerEvent> event =
> +    static_cast<nsTimerEvent*>(::new (KnownNotNull, p) nsTimerEvent());
> +
> +  if (MOZ_LOG_TEST(GetTimerLog(), LogLevel::Debug)) {
> +    event->mInitTime = TimeStamp::Now();

It strikes me this could be in nsTimerEvent::Init. Does it matter whether this happens before or after the traceinfo override above?

Comment 10

[Nathan Froyd [:froydnj] [away through 21 July]]

(In reply to Mike Hommey [:glandium] from comment #8)
> ::: xpcom/threads/TimerThread.cpp
> @@ -713,5 @@
> >    // the caller. We need to copy the generation number from this timer into the
> >    // event, so we can avoid firing a timer that was re-initialized after being
> >    // canceled.
> >  
> > -  RefPtr<nsTimerEvent> event = new nsTimerEvent;
> 
> dxr says this is the only caller of nsTimerEvent::nsTimerEvent. Instead of
> doing some placement-new hacks, why not use an Init method, like we use in
> many other places? (and while here, new nsTimerEvent() rather than new
> nsTimerEvent would make for less confusion)

I understand this as saying you are suggesting:

RefPtr<nsTimerEvent> event = new nsTimerEvent();
if (!event) {
  return timer.forget();
}

event->Init(timer.forget());
 
This is basically the same code we have now; we're just renaming SetTimer() to Init() instead.  And we want to move all the work to the constructor (cf part 3) because we want to initialize the RefPtr<nsTimerImpl> that nsTimerEvent is holding once in the constructor, rather than initializing it and then assigning to it.

So we really do need placement new hacks, because we want to separate the fallible-ness of the allocation from the actual construction of the object.  The current code (and the Init variant) presumably exists because the author realized that you can't pass timer.forget() into the constructor if the allocation is fallible: if the allocation fails, then you're dropping the timer reference on the floor.  But the current code is not efficient and not ideal.  WDYT?

I think `new nsTimerEvent()` is much better, too, hence the use of it in the current patch. ;)

(In reply to Mike Hommey [:glandium] from comment #9)
> Comment on attachment 9030859 [details] [diff] [review]
> part 2 - move some code around in PostTimerEvent
> 
> Review of attachment 9030859 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/threads/TimerThread.cpp
> @@ +732,5 @@
> > +  RefPtr<nsTimerEvent> event =
> > +    static_cast<nsTimerEvent*>(::new (KnownNotNull, p) nsTimerEvent());
> > +
> > +  if (MOZ_LOG_TEST(GetTimerLog(), LogLevel::Debug)) {
> > +    event->mInitTime = TimeStamp::Now();
> 
> It strikes me this could be in nsTimerEvent::Init. Does it matter whether
> this happens before or after the traceinfo override above?

That's a good point, this code could be moved into the constructor regardless.  I don't believe it matters where the initialization happens relative to `AutoSaveCurTraceInfo`.

Comment 11

[Mike Hommey [:glandium]]

I think we addressed all the questions raised here on IRC. Can you refresh the patches?

Comment 12

[Nathan Froyd [:froydnj] [away through 21 July]]

Attachment: part 1 - tweak nsTimerEvent allocation

Somehow, removing the static_cast from the previous version works now; I'm not
sure what I was doing before.

Comment 13

[Nathan Froyd [:froydnj] [away through 21 July]]

Attachment: part 2 - move some code around in PostTimerEvent

This leaves the `mInitTime` move for part 3, where I think it makes a little
more sense.

Comment 14

[Nathan Froyd [:froydnj] [away through 21 July]]

Attachment: part 3 - do more work in nsTimerEvent's constructor

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57ded7ecade7
part 1 - tweak nsTimerEvent allocation; r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/7adf8f337782
part 2 - move some code around in PostTimerEvent; r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ea519d6b3c8
part 3 - do more work in nsTimerEvent's constructor; r=glandium

Comment 16

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/57ded7ecade7
https://hg.mozilla.org/mozilla-central/rev/7adf8f337782
https://hg.mozilla.org/mozilla-central/rev/9ea519d6b3c8