This patch makes it so that ExpirationTrackerObserver now holds the
lock when issuing its callbacks to ensure the owner hasn't been
destroyed yet during shutdown races. PlaceholderLock does nothing,
so it is always safe to use, even if it has already been freed. The
StaticMutex is also always safe to use, because its lifetime now
exceeds that of ExpirationTrackerImpl and ExpirationTrackerObserver.
It also moves the timer callbacks into ExpirationTrackerObserver via
nsITimerCallback, as it can suffer from the same shutdown race as
the memory-pressure events.
Differential Revision: https://phabricator.services.mozilla.com/D303950
This patch makes it so that ExpirationTrackerObserver now holds the
lock when issuing its callbacks to ensure the owner hasn't been
destroyed yet during shutdown races. PlaceholderLock does nothing,
so it is always safe to use, even if it has already been freed. The
StaticMutex is also always safe to use, because its lifetime now
exceeds that of ExpirationTrackerImpl and ExpirationTrackerObserver.
It also moves the timer callbacks into ExpirationTrackerObserver via
nsITimerCallback, as it can suffer from the same shutdown race as
the memory-pressure events.
Differential Revision: https://phabricator.services.mozilla.com/D303950
Image decode tasks dispatched to TaskController pool threads can still
run after xpcom-shutdown-threads, because TaskController is only shut
down very late in the process teardown sequence. This causes
NotifyDecodeComplete and DropImageReference to attempt dispatching to the
main thread after it has stopped accepting events, triggering a
!mRawPtr assertion in already_AddRefed's destructor.
Check DecodePool::IsShuttingDown() in DecodingTask::Run() before running
the task, matching what AnimationSurfaceProvider already does internally.
Also update the comment in SurfaceCache::ReleaseImageOnMainThread to
clarify that the shutdown path is unreachable in release builds and the
(void)aImage cast is intentional to preserve the DEBUG assertion.
Differential Revision: https://phabricator.services.mozilla.com/D290391
The changeset has been generated automatically through:
./mach static-analysis check --output ./clang-tidy-bugprone.json --format json --fix
followed by manual adjustment when needed or when the auto-fixer bailed out.
This paves the way for Bug 2023527
Differential Revision: https://phabricator.services.mozilla.com/D287941
The changeset has been generated automatically through:
./mach static-analysis check --output ./clang-tidy-bugprone.json --format json --fix
followed by manual adjustment when needed or when the auto-fixer bailed out.
This paves the way for Bug 2023527
Differential Revision: https://phabricator.services.mozilla.com/D287941
It is replaced by a standard-conforming cast to (void) which is
understood the same by the compiler.
In some cases this could be replaced by `[[maybe_unused]]` but for
expression whose value is ignored, this requires storing to a temporary
variable first, so not going that way.
One notable change is that marking an object of type already_AddRefed as
unused used to implicitly leak memory. This is now made explicit.
Most of the heavy lifting was done through:
$ git grep -l '\<Unused\> <<' | xargs sed -i -e 's/Unused << /(void)/g'
$ git grep -l mozilla/Unused.h | xargs sed -i -e '/mozilla\/Unused.h/ d'
Differential Revision: https://phabricator.services.mozilla.com/D268706
It is replaced by a standard-conforming cast to (void) which is
understood the same by the compiler.
In some cases this could be replaced by `[[maybe_unused]]` but for
expression whose value is ignored, this requires storing to a temporary
variable first, so not going that way.
One notable change is that marking an object of type already_AddRefed as
unused used to implicitly leak memory. This is now made explicit.
Most of the heavy lifting was done through:
$ git grep -l '\<Unused\> <<' | xargs sed -i -e 's/Unused << /(void)/g'
$ git grep -l mozilla/Unused.h | xargs sed -i -e '/mozilla\/Unused.h/ d'
Differential Revision: https://phabricator.services.mozilla.com/D268706
It is replaced by a standard-conforming cast to (void) which is
understood the same by the compiler.
In some cases this could be replaced by `[[maybe_unused]]` but for
expression whose value is ignored, this requires storing to a temporary
variable first, so not going that way.
One notable change is that marking an object of type already_AddRefed as
unused used to implicitly leak memory. This is now made explicit.
Most of the heavy lifting was done through:
$ git grep -l '\<Unused\> <<' | xargs sed -i -e 's/Unused << /(void)/g'
$ git grep -l mozilla/Unused.h | xargs sed -i -e '/mozilla\/Unused.h/ d'
Differential Revision: https://phabricator.services.mozilla.com/D268706
It is replaced by a standard-conforming cast to (void) which is
understood the same by the compiler.
In some cases this could be replaced by `[[maybe_unused]]` but for
expression whose value is ignored, this requires storing to a temporary
variable first, so not going that way.
One notable change is that marking an object of type already_AddRefed as
unused used to implicitly leak memory. This is now made explicit.
Most of the heavy lifting was done through:
$ git grep -l '\<Unused\> <<' | xargs sed -i -e 's/Unused << /(void)/g'
$ git grep -l mozilla/Unused.h | xargs sed -i -e '/mozilla\/Unused.h/ d'
Differential Revision: https://phabricator.services.mozilla.com/D268706
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 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
AspectRatio already has a way of representing a null / invalid ratio,
and there are some cases (broken sizes or invalid SVGOuterSVGFrame
ratio) where we might get Some(AspectRatio()).
It's not a meaningfully different state, and IMO we shouldn't treat it
differently.
Differential Revision: https://phabricator.services.mozilla.com/D224212
AspectRatio already has a way of representing a null / invalid ratio,
and there are some cases (broken sizes or invalid SVGOuterSVGFrame
ratio) where we might get Some(AspectRatio()).
It's not a meaningfully different state, and IMO we shouldn't treat it
differently.
Differential Revision: https://phabricator.services.mozilla.com/D224212
When not using blob recordings for vector images we use simple surface providers. When we get an invalidation we will call SurfaceCache::InvalidateImage which will set a dirty bit on all blob recordings and remove all other surfaces from the surface cache. And if it finds a blob recording we will call ResumeHonoringInvalidations. This is not enough to invalidate in the case of a non-animated vector image using the webrender GetImageProvider path. Even though the surface isn't in the surface cache anymore it is still cached on the frame and when we tell it about the invalidation it will just has the image provider to update its key. That will call this code
https://searchfox.org/mozilla-central/rev/cdddec7fd690700efa4d6b48532cf70155e0386b/image/DecodedSurfaceProvider.cpp#222
which will just share the same surface as before the invalidation happened again. That will let us handle one invalidation but it still won't fix the bug, we need to call ResumeHonoringInvalidations so that we handle further invalidations.
When not using the image provider path, we don't save an image provider on the frame, that alone would avoid this problem. But we also call ResumeHonoringInvalidations for every successful call of VectorImage::Draw.
* * *
imported patch asvfref
Differential Revision: https://phabricator.services.mozilla.com/D174450
This pref exists only for fuzzers so they don't waste time on overly large images. The way fuzzers set the pref requires it to be live.
Differential Revision: https://phabricator.services.mozilla.com/D198498
This pref exists only for fuzzers so they don't waste time on overly large images. The way fuzzers set the pref requires it to be live.
Differential Revision: https://phabricator.services.mozilla.com/D198498
Large image sizes slow down fuzzing for webp and aren't likely to catch any more issues. We will still want to fuzz without this pref to catch any large image issues though.
Differential Revision: https://phabricator.services.mozilla.com/D188156
To release the images, `ClearReleasingImages` basically takes all the `RefPtr<Image>` out of `mReleasingImagesOnMainThread` under a lock and then clears them, all on the main thread. This can be safely done before as late as XPCOMShutdownFinal on the main thread in order to avoid leaks.
Differential Revision: https://phabricator.services.mozilla.com/D160624
If the surface cache entry is a placeholder (because we haven't decoded any frames yet) then a surface cache lookup will not return anything, so we can't get ahold of the AnimationSurfaceProvider to call reset on it.
I also looked at the other places we call Lookup on the surface cache and it seems correct to me for them to not get the surface if it's still a placeholder.
Differential Revision: https://phabricator.services.mozilla.com/D143894
Because these images are loaded as embeds/objects we can't get a hold of them to apply most usual methods of waiting for decode (img.decode, using a special powers image notifications observer). This should hopefully be enough to avoid hitting this in automation.
Backout the patch for bug 1722422 (the one that changed it from an assert to a leak) because I think it's easier to classify/recognize the assert than the leak in the logs.
Differential Revision: https://phabricator.services.mozilla.com/D139464
Destroying an already_AddRefed if it holds a non-null pointers asserts because it leaks. But we want the leak here, so leak in a way that doesn't assert.
Differential Revision: https://phabricator.services.mozilla.com/D139266
Destroying an already_AddRefed if it holds a non-null pointers asserts because it leaks. But we want the leak here, so leak in a way that doesn't assert.
Differential Revision: https://phabricator.services.mozilla.com/D139266
If a vector image does not have an intrinsic size as returned by
VectorImage::GetWidth and VectorImage::GetHeight, currently factor-of-2
scaling is disabled. With this patch, we just assume a default size of
100x100, adjusted by the intrinsic ratio if available, to get the
baseline size for determing appropriate factor-of-2 sizes.
Differential Revision: https://phabricator.services.mozilla.com/D133395
Now that we no longer have the extra layer of ImageContainers providing
a superficial level of caching/reuse of existing blob recordings, we
need some way to share recordings. This part adds support to
SurfaceCache to store BlobSurfaceProvider objects.
This includes the specialized code for invalidating SVG images. In
particular this is useful for animated SVG images. In general we want to
avoid changing the image key whenever possible so that we avoid
reallocating the underlying buffers in the compositor process for the
rasterized blob images.
We also need to track the ImageIntRegion used by the recording. If a
caller only wants a slice of the SVG image, then we need to track this
differentiation in our cache entries. At this time, we don't allow
substitutes for entries with a region exclusion.
Differential Revision: https://phabricator.services.mozilla.com/D126603
We no longer use SourceSurfaceMappedData because we only support
SourceSurfaceSharedData-backed imgFrame in the SurfaceCache now.
Differential Revision: https://phabricator.services.mozilla.com/D126595
Now that we no longer have the extra layer of ImageContainers providing
a superficial level of caching/reuse of existing blob recordings, we
need some way to share recordings. This part adds support to
SurfaceCache to store BlobSurfaceProvider objects.
This includes the specialized code for invalidating SVG images. In
particular this is useful for animated SVG images. In general we want to
avoid changing the image key whenever possible so that we avoid
reallocating the underlying buffers in the compositor process for the
rasterized blob images.
We also need to track the ImageIntRegion used by the recording. If a
caller only wants a slice of the SVG image, then we need to track this
differentiation in our cache entries. At this time, we don't allow
substitutes for entries with a region exclusion.
Differential Revision: https://phabricator.services.mozilla.com/D126603
We no longer use SourceSurfaceMappedData because we only support
SourceSurfaceSharedData-backed imgFrame in the SurfaceCache now.
Differential Revision: https://phabricator.services.mozilla.com/D126595
Andrew Osmond