Closed Bug 1513615 Opened 7 years ago Closed 6 years ago

clean up timer event initialization

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Crash Data

Attachments

(3 files, 6 obsolete files)

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
Crash Signature: [@ nsTimerImpl::Release]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: