This avoids the small overhead and potential for bugs from the previous
approach of assuming all `const char*` arguments passed to `nsITimer`
initializers are string literals, by instead requiring the argument to be a
`const nsACString&`. This involves changing all callsites to pass a `""_ns`
string literal down instead of a bare string literal.
It appears that while there were currently no buggy callers which
would've provided a dynamic string, there was at least one piece of code
(nsRepeatService::Start) which did pass down a raw char* which may be
dynamically allocated (all callers pass down `nsLiteralCString` so we're
actually OK here right now, but this was a footgun).
Differential Revision: https://phabricator.services.mozilla.com/D261032
This avoids risks which can be caused by reading the name for things
such as the profiler. By fetching the name from the callback when the
nsITimer is initialized, we avoid potential race conditions, and also
avoid the potential for re-entrancy when reading the name from a timer.
As part of this change, timer names are now always stored as a
`nsCString` (as they may be dynamic due to JS or custom nsINamed
implementations). The `const char*` arguments passed to function and closure
based constructors are assumed to be string literals, and passed to
`AssignLiteral`.
Differential Revision: https://phabricator.services.mozilla.com/D261031
This reverts commit 8c929cc018.
Revert "Bug 1979013 - Part 2: Require nsITimer API callers to provide a nsACString, r=xpcom-reviewers,media-playback-reviewers,webrtc-reviewers,profiler-reviewers,dom-storage-reviewers,toolkit-telemetry-reviewers,firefox-style-system-reviewers,mstange,emz,bwc,janv,emilio,padenot"
This reverts commit e01c0a6a32.
Revert "Bug 1979013 - Part 1: Eagerly fetch name for timer callbacks, r=xpcom-reviewers,mccr8"
This reverts commit 9f8e73c163.
This avoids the small overhead and potential for bugs from the previous
approach of assuming all `const char*` arguments passed to `nsITimer`
initializers are string literals, by instead requiring the argument to be a
`const nsACString&`. This involves changing all callsites to pass a `""_ns`
string literal down instead of a bare string literal.
It appears that while there were currently no buggy callers which
would've provided a dynamic string, there was at least one piece of code
(nsRepeatService::Start) which did pass down a raw char* which may be
dynamically allocated (all callers pass down `nsLiteralCString` so we're
actually OK here right now, but this was a footgun).
Differential Revision: https://phabricator.services.mozilla.com/D261032
This avoids risks which can be caused by reading the name for things
such as the profiler. By fetching the name from the callback when the
nsITimer is initialized, we avoid potential race conditions, and also
avoid the potential for re-entrancy when reading the name from a timer.
As part of this change, timer names are now always stored as a
`nsCString` (as they may be dynamic due to JS or custom nsINamed
implementations). The `const char*` arguments passed to function and closure
based constructors are assumed to be string literals, and passed to
`AssignLiteral`.
Differential Revision: https://phabricator.services.mozilla.com/D261031
While the recommendation to not use SetDelay at all with one-shot timers
remains, there are too many rare usages to be easily audited.
But we can at least increase the sequence number, which ensures for
one-shot timers:
- in case the timer event was not yet executed we will reschedule the timer,
even if the event was already in flight.
- in case the payload was already executed, we will still bail with error.
For repeating timers nothing changes.
Differential Revision: https://phabricator.services.mozilla.com/D256191
This will help us later to craft stable and unique sort conditions for TimerThread::mTimers, enabling binary search on the array.
Differential Revision: https://phabricator.services.mozilla.com/D249899
This will help us later to craft stable and unique sort conditions for TimerThread::mTimers, enabling binary search on the array.
Differential Revision: https://phabricator.services.mozilla.com/D249899
On certain devices we have seen that taking frequent timestamps can be
expensive, therefore it makes sense to reduce the number of timestamps
taken in hot paths. nsTimerImpl::Fire() currently takes a timestamp
which only gets used in logging. This patch therefore makes it so the
timestamp is only taken when logging is enabled.
Differential Revision: https://phabricator.services.mozilla.com/D185010
We'll probably want to do something more accurate in the future with a
custom clang static analysis pass which validates that XPIDL interfaces
have the expected vtable and struct layout, however doing so would be
more involved than the string matching done in this patch.
In addition to checking for extra virtual methods, we'll likely also
want to check for data members on interfaces, and reject them unless the
class is marked as `[builtinclass]` in addition to some other attribute
which we'll need to add to prevent them from being implemented in Rust
(as c++ data members will not be reflected by the rust macro).
There were 2 instances of a comment which contained the word 'virtual'
within a CDATA block. These comments were moved out of the CDATA block
to avoid triggering the error.
Differential Revision: https://phabricator.services.mozilla.com/D151068
Covers the case where we finish late because the callback took a long time,
and the case where we fired really late (eg; because of sleep).
Differential Revision: https://phabricator.services.mozilla.com/D123814
Covers the case where we finish late because the callback took a long time,
and the case where we fired really late (eg; because of sleep).
Differential Revision: https://phabricator.services.mozilla.com/D123814
This particular race is a tricky one - there's no perfect solution to protecting the timer thread from being called in `cancel` while being dereferenced. This ensures that we won't run into that problem by locking all of our TimerThread calls behind a mutex inside a wrapper class. Then we hold onto the wrapper class until after we shutdown `nsThreadManager`, in which case no background thread pools should be active anymore to call `nsTimerImpl::Cancel`.
For reference, the worst-case scenario happens when another thread dereferences `gThread` [between these two calls](https://searchfox.org/mozilla-central/rev/98a9257ca2847fad9a19631ac76199474516b31e/xpcom/threads/nsTimerImpl.cpp#402-403), in which case we will get stuck on a dereferenced mutex. By putting the check and the actual call into `gThread` behind a mutex maybe we can prevent this issue.
Differential Revision: https://phabricator.services.mozilla.com/D115453
Sylvestre Ledru