Closed
Bug 1513615
Opened 7 years ago
Closed 6 years ago
clean up timer event initialization
Categories
(Core :: XPCOM, enhancement)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla66
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: froydnj, Assigned: froydnj)
References
Details
Crash Data
Attachments
(3 files, 6 obsolete files)
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.
![]() |
Assignee | |
Comment 1•7 years ago
|
||
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)
![]() |
Assignee | |
Comment 2•7 years ago
|
||
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)
![]() |
Assignee | |
Comment 3•7 years ago
|
||
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)
![]() |
Assignee | |
Comment 4•7 years ago
|
||
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.
Updated•7 years ago
|
status-firefox66:
--- → affected
See Also: → 1489953
![]() |
Assignee | |
Comment 5•7 years ago
|
||
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)
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #9030815 -
Attachment is obsolete: true
Attachment #9030815 -
Flags: review?(mh+mozilla)
![]() |
Assignee | |
Comment 6•7 years ago
|
||
Same as before, just rebased.
Attachment #9030859 -
Flags: review?(mh+mozilla)
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #9030816 -
Attachment is obsolete: true
Attachment #9030816 -
Flags: review?(mh+mozilla)
![]() |
Assignee | |
Comment 7•7 years ago
|
||
Same as before, just rebased.
Attachment #9030860 -
Flags: review?(mh+mozilla)
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #9030817 -
Attachment is obsolete: true
Attachment #9030817 -
Flags: review?(mh+mozilla)
Comment 8•7 years ago
|
||
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 9•7 years ago
|
||
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)
Updated•7 years ago
|
Attachment #9030860 -
Flags: review?(mh+mozilla) → review+
![]() |
Assignee | |
Comment 10•7 years ago
|
||
(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)
Comment 11•7 years ago
|
||
I think we addressed all the questions raised here on IRC. Can you refresh the patches?
Flags: needinfo?(mh+mozilla)
![]() |
Assignee | |
Comment 12•6 years ago
|
||
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)
![]() |
Assignee | |
Updated•6 years ago
|
Attachment #9030858 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 13•6 years ago
|
||
This leaves the `mInitTime` move for part 3, where I think it makes a little
more sense.
Attachment #9034234 -
Flags: review?(mh+mozilla)
![]() |
Assignee | |
Updated•6 years ago
|
Attachment #9030859 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 14•6 years ago
|
||
Attachment #9034235 -
Flags: review?(mh+mozilla)
![]() |
Assignee | |
Updated•6 years ago
|
Attachment #9030860 -
Attachment is obsolete: true
Updated•6 years ago
|
Attachment #9034233 -
Flags: review?(mh+mozilla) → review+
Updated•6 years ago
|
Attachment #9034234 -
Flags: review?(mh+mozilla) → review+
Updated•6 years ago
|
Attachment #9034235 -
Flags: review?(mh+mozilla) → review+
Comment 15•6 years ago
|
||
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
Comment 16•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/57ded7ecade7
https://hg.mozilla.org/mozilla-central/rev/7adf8f337782
https://hg.mozilla.org/mozilla-central/rev/9ea519d6b3c8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Updated•6 years ago
|
Crash Signature: [@ nsTimerImpl::Release]
You need to log in
before you can comment on or make changes to this bug.
Description
•