clean up timer event initialization

RESOLVED FIXED in Firefox 66

Status

()

enhancement
RESOLVED FIXED
7 months ago
2 months ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla66
Points:
---

Firefox Tracking Flags

(firefox66 fixed)

Details

(crash signature)

Attachments

(3 attachments, 6 obsolete attachments)

2.44 KB, patch
glandium
: review+
Details | Diff | Splinter Review
2.22 KB, patch
glandium
: review+
Details | Diff | Splinter Review
3.65 KB, patch
glandium
: review+
Details | Diff | Splinter Review
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.
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.
Attachment #9030815 - Flags: review?(mh+mozilla)
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.
Attachment #9030816 - Flags: review?(mh+mozilla)
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.
Attachment #9030817 - Flags: review?(mh+mozilla)
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.
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.
Attachment #9030858 - Flags: review?(mh+mozilla)
Attachment #9030815 - Attachment is obsolete: true
Attachment #9030815 - Flags: review?(mh+mozilla)
Same as before, just rebased.
Attachment #9030859 - Flags: review?(mh+mozilla)
Attachment #9030816 - Attachment is obsolete: true
Attachment #9030816 - Flags: review?(mh+mozilla)
Same as before, just rebased.
Attachment #9030860 - Flags: review?(mh+mozilla)
Attachment #9030817 - Attachment is obsolete: true
Attachment #9030817 - Flags: review?(mh+mozilla)
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)
Attachment #9030858 - Flags: review?(mh+mozilla)
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?
Attachment #9030859 - Flags: review?(mh+mozilla)
Attachment #9030860 - Flags: review?(mh+mozilla) → review+
(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`.
Flags: needinfo?(mh+mozilla)
I think we addressed all the questions raised here on IRC. Can you refresh the patches?
Flags: needinfo?(mh+mozilla)
Somehow, removing the static_cast from the previous version works now; I'm not
sure what I was doing before.
Attachment #9034233 - Flags: review?(mh+mozilla)
Attachment #9030858 - Attachment is obsolete: true
This leaves the `mInitTime` move for part 3, where I think it makes a little
more sense.
Attachment #9034234 - Flags: review?(mh+mozilla)
Attachment #9030859 - Attachment is obsolete: true
Attachment #9030860 - Attachment is obsolete: true
Attachment #9034233 - Flags: review?(mh+mozilla) → review+
Attachment #9034234 - Flags: review?(mh+mozilla) → review+
Attachment #9034235 - Flags: review?(mh+mozilla) → review+
Pushed by nfroyd@mozilla.com:
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
Duplicate of this bug: 1489953
Crash Signature: [@ nsTimerImpl::Release]
You need to log in before you can comment on or make changes to this bug.