Instead of discarding released chunks, we keep them as "next" chunks, and we make sure there's a valid "current" chunk when possible.
Recycling released chunks means that when using the `ProfileBufferChunkManagerSingle`, the one chunk, in whichever state it may be, will be kept alive and reused.
Differential Revision: https://phabricator.services.mozilla.com/D99979
When a "Put" operation fails (most probably because no chunk was available to store the data), we remember the number of bytes that couldn't be stored.
This can be useful to give an indication of how much more memory would have been needed for successful puts.
Differential Revision: https://phabricator.services.mozilla.com/D99977
This bug has revealed some issues when the single chunk gets filled, and there are different paths depending on whether the chunk is filled right to the end, or past it.
Later patches will fix these issues and update these tests accordingly.
Differential Revision: https://phabricator.services.mozilla.com/D99976
In DEBUG builds, instead of only testing for the expected chunk state(s), there are now separate assertions for each *un*expected state, which makes it much easier to track the source of failures.
Differential Revision: https://phabricator.services.mozilla.com/D99974
So this is an ugly solution, but it was the best I could come up with. We do
not want to show the skeleton UI if we're going to show the profile manager,
and we *will* show the profile manager if StartWithLastProfile=0 is under
[General] in profiles.ini. Accordingly the only ways to do the correct thing
here are to try to mirror edits that firefox makes to the profiles.ini file
to the registry, or to simply read the profiles.ini file ourselves. There are
many ways that profiles.ini could get out of sync with the registry if we
tried to mirror its state there, so going straight to the source of truth
seemed the best option.
There is one case which is still not covered here: if there is no profile for
our install marked as Default=1, then we will show the profile manager. This
should only be possible if the user manually edits their profiles.ini file,
however, and then it should resolve itself after one run, so I don't consider
it a significant enough problem to jump through all the hoops we would need
to jump through to solve it.
Depends on D98525
Differential Revision: https://phabricator.services.mozilla.com/D98936
Note that we can probably use mLastRefreshDriverTime directly in
DocumentTimeline::GetCurrentTimeStamp(), i.e. we don't need to use the refresh
driver there, but I'd preserve the current behavior.
Differential Revision: https://phabricator.services.mozilla.com/D97823
Note that we can probably use mLastRefreshDriverTime directly in
DocumentTimeline::GetCurrentTimeStamp(), i.e. we don't need to use the refresh
driver there, but I'd preserve the current behavior.
Differential Revision: https://phabricator.services.mozilla.com/D97823
Add `mWaitingForDecode` to indicate that main-thread is blocked on the
Monitor and should be woken. This avoids generating some of the unused
Runnables. Also ensure `mFinishDecodeRunnablePending` is only accessed while
holding the lock.
Depends on D99403
Differential Revision: https://phabricator.services.mozilla.com/D99404
The `mToken` is used across threads without full locking so it must be marked
as atomic. At the same time, we can use `exchange` operations to read or
write outside of the mMonitor lock.
Depends on D99402
Differential Revision: https://phabricator.services.mozilla.com/D99403
Be consistent about always calling `MaybeFinishOffThreadDecode` without
holding the lock to simplify code. This lets us remove a TSAN deadlock
exception.
Differential Revision: https://phabricator.services.mozilla.com/D99402
The menubar is permanently shown if autohide is false. If that is the case, we
insert space above the tab and ensure the tab does not have a left margin.
It's height can change, so we store the height in our registry.
Differential Revision: https://phabricator.services.mozilla.com/D97195
Some env vars have effects similar to command line arguments which present
problems for the skeleton UI, and we want to treat these env vars similarly.
Differential Revision: https://phabricator.services.mozilla.com/D98475
Previously, we implemented arg checking with `marionette` just carrying a free
pass, so we could let the arguments which typically come when running tests.
However, some marionette tests do like to play with arguments which we do not
want to get a free pass, such as -safe-mode. These changes allow just the
-profile argument through, as that is necessary for running tests.
Differential Revision: https://phabricator.services.mozilla.com/D98474
Some env vars have effects similar to command line arguments which present
problems for the skeleton UI, and we want to treat these env vars similarly.
Differential Revision: https://phabricator.services.mozilla.com/D98475
Previously, we implemented arg checking with `marionette` just carrying a free
pass, so we could let the arguments which typically come when running tests.
However, some marionette tests do like to play with arguments which we do not
want to get a free pass, such as -safe-mode. These changes allow just the
-profile argument through, as that is necessary for running tests.
Differential Revision: https://phabricator.services.mozilla.com/D98474
Even though the Glean dispatcher was shutting down, the SDK was not
properly joining on the thread, triggering a TSAN error. This has been
addressed in a new version of the Glean SDK.
Differential Revision: https://phabricator.services.mozilla.com/D98889
We don't want to show the skeleton UI if there is already an instance of
Firefox running for that install. Accordingly, we implement something
similar to the profile lock, acquiring exclusive access to
~/AppData/Local/Mozilla/Firefox/SkeletonUILock-<installHash>. If we do not do
this, then when a user clicks firefox.exe while an existing instance is
running, under default conditions we will open the skeleton UI, then
almost immediately terminate and send a message to the existing instance to
open a new window.
Differential Revision: https://phabricator.services.mozilla.com/D98525
TSan found races on these flags that are ostensibly benign but this way there's no UB.
This code is a bit weird. These bitfields are seemingly pointless as
they're squeezed between a uint64_t and an Atomic<bool>. There's plenty
of space for 2 more bools there.
Also the Atomic<bool> could theoretically be merged into the flags. For
now, here's the version of this patch that preserves the semantics of
this code as closely as possible, for review by the code owners.
Differential Revision: https://phabricator.services.mozilla.com/D93416
TSan found races between mResolveAgain and mGetTtl. This makes them non-UB.
The code is a bit weird. Although the values are typed as uint16_t's, they're
used as bools (even assigned true/false). In addition, the atomic bool mTRRUsed
could be folded into these fields, as there is a spare bit. I decided not to
change these things, as network code can have weird representation requirements
that I'd prefer the owners of the code chime in on first.
Differential Revision: https://phabricator.services.mozilla.com/D93417
TSan found races on these flags that are ostensibly benign but this way there's no UB.
This code is a bit weird. These bitfields are seemingly pointless as
they're squeezed between a uint64_t and an Atomic<bool>. There's plenty
of space for 2 more bools there.
Also the Atomic<bool> could theoretically be merged into the flags. For
now, here's the version of this patch that preserves the semantics of
this code as closely as possible, for review by the code owners.
Differential Revision: https://phabricator.services.mozilla.com/D93416
TSan found races between mResolveAgain and mGetTtl. This makes them non-UB.
The code is a bit weird. Although the values are typed as uint16_t's, they're
used as bools (even assigned true/false). In addition, the atomic bool mTRRUsed
could be folded into these fields, as there is a spare bit. I decided not to
change these things, as network code can have weird representation requirements
that I'd prefer the owners of the code chime in on first.
Differential Revision: https://phabricator.services.mozilla.com/D93417
clang 12 (specifically https://reviews.llvm.org/D91379) made some refactorings to libc++ that exposed a problem in the MinGW headers. That has now been fixed upstream.
In the meantime the headers also gained definitions for ProcessPayloadRestrictionPolicy, so we can remove our workaround for that.
Differential Revision: https://phabricator.services.mozilla.com/D98945
No idea why this didn't show up in earlier try runs, but this is a known false-positive that we need to suppress in the newly-added EncryptedClientHelloServer.
Differential Revision: https://phabricator.services.mozilla.com/D98771
Currently, printf_stderr doesn't show up when running with ./mach run.
This is because we run with -attach-console and that redirects stderr
to a different file descriptor using freopen in UseParentConsole.
The change from just using stderr directly happened in bug 340443 and was done
to avoid some linking issues. That problem doesn't seem to apply anymore so we
should be able to go back to the straightforward implemention that works even
if stderr has been redirected. The mozglue implementation was cargo culted from
xpcom, and there wasn't a reason other than that for the fdopen(dup()) there.
Differential Revision: https://phabricator.services.mozilla.com/D98550
TimeStamps in markers must now be streamed through `SpliceableJSONWriter::TimeProperty(name, timestamp)`.
This is consistent with all other JSON-writing functions being in `SpliceableJSONWriter` (and base class `JSONWriter`).
Depends on D97556
Differential Revision: https://phabricator.services.mozilla.com/D97557
Expand `profiler_is_locked_on_current_thread()` to also return true if the ProfilerChild mutex is locked on the current thread.
This will prevent some profiler-re-entrant operations (like native allocation markers) from running.
In particular, this removes the potential deadlock found in bug 1671403:
- SamplerThread: In the sampling loop, lock the main profiler mutex, run a ProfilerChild update, which attempts to lock the ProfilerChild mutex.
- ProfilerChild thread: While processing an IPC message with an update, lock the ProfilerChild mutex, then resolve the update, which records a native allocation with a backtrace that attempts to lock the main profiler mutex.
With this patch, the native allocation won't record a marker while the ProfilerChild mutex is locked.
Differential Revision: https://phabricator.services.mozilla.com/D96969
In this case, the same marker type "CONTENT_FULL_PAINT_TIME" is used in separate places, so it makes sense to put the marker type definition in a common location.
Differential Revision: https://phabricator.services.mozilla.com/D96042
To be able to deserialize raw pointers (when streaming markers), `ProfileBufferRawPointer` must be used (to prevent uncontrolled raw pointers), but with the new API implementation the deserializer must accept references to `ProfilerBufferRawPointer` instead of just a raw pointer.
Differential Revision: https://phabricator.services.mozilla.com/D96032
In this case, the same marker type "CONTENT_FULL_PAINT_TIME" is used in separate places, so it makes sense to put the marker type definition in a common location.
Differential Revision: https://phabricator.services.mozilla.com/D96042
To be able to deserialize raw pointers (when streaming markers), `ProfileBufferRawPointer` must be used (to prevent uncontrolled raw pointers), but with the new API implementation the deserializer must accept references to `ProfilerBufferRawPointer` instead of just a raw pointer.
Differential Revision: https://phabricator.services.mozilla.com/D96032
In this case, the same marker type "CONTENT_FULL_PAINT_TIME" is used in separate places, so it makes sense to put the marker type definition in a common location.
Differential Revision: https://phabricator.services.mozilla.com/D96042
To be able to deserialize raw pointers (when streaming markers), `ProfileBufferRawPointer` must be used (to prevent uncontrolled raw pointers), but with the new API implementation the deserializer must accept references to `ProfilerBufferRawPointer` instead of just a raw pointer.
Differential Revision: https://phabricator.services.mozilla.com/D96032
* Bumps the tsan toolchain to rust-nightly-2020-11-14 that has my patches to make -Zbuild-std work in vendored environments:
* https://github.com/rust-lang/cargo/pull/8834
* https://github.com/rust-lang/rust/pull/78790
* Passes -Zbuild-std to cargo when MOZ_TSAN is defined (mk_add_options --enable-thread-sanitizer)
* Removes generic Rust supressions and adds much more specific ones
* One presumed upstream false positive from tsan not understanding the code
* One actual upstream bug tsan found (yay!)
* One new real issue uncovered
* One issue that probably already existed intermittently but I happened to hit
Differential Revision: https://phabricator.services.mozilla.com/D97165
I'm hoping that the comments in the code are sufficient documentation of this,
but here is a quick overview. The mockup we got from design has rounded rects
in it, and bordered rounded rects in it, so we need to support drawing those.
Initially we tried using the RoundRect function in GDI. However, 1) GDI doesn't
support anti-aliasing, which is unfortunately noticeable, 2) we were
observing strange issues with only some of the corners being rounded with
RoundRect at higher radii / stroke widths. 3) drawing on top of things drawn
with RoundRect would be complicated and inefficient unless we switched
everything over to GDI calls.
As it stands this drawing code is platform agnostic, assuming we have a way of
blitting a raw bitmap to the screen on a given platform, which is a nice trait
to have and makes me think twice about switching all of the drawing over to
direct GDI calls.
Differential Revision: https://phabricator.services.mozilla.com/D95317
I'm hoping that the comments in the code are sufficient documentation of this,
but here is a quick overview. The mockup we got from design has rounded rects
in it, and bordered rounded rects in it, so we need to support drawing those.
Initially we tried using the RoundRect function in GDI. However, 1) GDI doesn't
support anti-aliasing, which is unfortunately noticeable, 2) we were
observing strange issues with only some of the corners being rounded with
RoundRect at higher radii / stroke widths. 3) drawing on top of things drawn
with RoundRect would be complicated and inefficient unless we switched
everything over to GDI calls.
As it stands this drawing code is platform agnostic, assuming we have a way of
blitting a raw bitmap to the screen on a given platform, which is a nice trait
to have and makes me think twice about switching all of the drawing over to
direct GDI calls.
Differential Revision: https://phabricator.services.mozilla.com/D95317
This patch supports a skeleton UI for default, light, and dark themes.
It is not enabled for apenglow or any custom themes.
This also takes into account the system theme. If the user has the default
theme selected and is in dark mode, we override the theme and present the
dark theme skeleton UI.
Differential Revision: https://phabricator.services.mozilla.com/D96230
This patch supports a skeleton UI for default, light, and dark themes.
It is not enabled for apenglow or any custom themes.
This also takes into account the system theme. If the user has the default
theme selected and is in dark mode, we override the theme and present the
dark theme skeleton UI.
Differential Revision: https://phabricator.services.mozilla.com/D96230
This patch adds a list of the executable's dependent module's path to SharedSection
as an array of the offset to a string and a string buffer. A following patch will
use this data from the browser process and the sandboxed processes.
Depends on D96283
Differential Revision: https://phabricator.services.mozilla.com/D96284
We transfer several ntdll's function addresses to a child process directly via
`WriteProcessMemory`. This patch changes the way to transfer data to using
a section object as Chromium sandbox does, so that we can transfer more data
with the same cost as transferring a single handle value.
Depends on D96282
Differential Revision: https://phabricator.services.mozilla.com/D96283
If a searchbar is present, draw that similar to a urlbar.
This also changes how we store the urlbar, as we know will use our custom struct
to cleanly store the width and height.
Differential Revision: https://phabricator.services.mozilla.com/D95029
Unique strings are used to encode all markers' 'name' field, SpliceableJSONWriter::UniqueStringElement can be used for that (instead of a caller-provided callback, which was necessary before UniqueJSONStrings was de-duplicated).
Differential Revision: https://phabricator.services.mozilla.com/D95513
Some markers (e.g., Screenshot) use unique strings in their data.
The UniqueJSONStrings used during streaming is attached to the SpliceableJSONWriter, and StreamJSONMarkerData can use pass-through functions UniqueStringProperty() and UniqueStringElement().
Differential Revision: https://phabricator.services.mozilla.com/D95512
Some markers (e.g., GC major/minor/slice) need to splice JSON strings in their data.
So now, instead of JSONWriter, StreamJSONMarkerData functions will be given a mozilla::baseprofiler::SpliceableJSONWriter.
Differential Revision: https://phabricator.services.mozilla.com/D95511
`UniqueJSONStrings` constructors now accept a `JSONWriter::CollectionStyle` to pass to the internal JSON writer.
The default won't change how the unique string list is output (with friendly indented lines), but tests can use `JSONWriter::SingleLineStyle` to make comparison strings more readable and maintainable.
Differential Revision: https://phabricator.services.mozilla.com/D95832
The shutdown code of BackgroundReadFiles races with BeginBackgroundRead.
This is paritally obfuscated by the usage of the initialEvent convenience
of NS_NewNamedThread, so this change also removes that in favour of explicit
dispatch. (xpcom devs want to remove the convenience anyway)
Differential Revision: https://phabricator.services.mozilla.com/D94877
The previous patch removed the only two uses of ProfilerStringView::String().
Since it can be potentially expensive (creating a `std::string` object, sometimes allocating a buffer, and copying the string contents), it's best to remove it completely.
Differential Revision: https://phabricator.services.mozilla.com/D95116
For consistency with `JSONWriter` (which UniqueJSONStrings' functions use), and for added safety and some efficiency, UniqueJSONStrings now takes `Span<const char`> arguments instead of raw pointers to null-terminated strings.
Differential Revision: https://phabricator.services.mozilla.com/D95114
Document the class and methods.
`GetOrAddIndex` is only used internally, so it can be private.
`SpliceStringTableElements` can now only work on rvalue UniqueJSONStrings, this emphasizes that it shouldn't be used anymore after this call.
Differential Revision: https://phabricator.services.mozilla.com/D95113
Most of this patch is a dance to avoid size flickering of the skeleton UI
window. We change all Resize/Move/SetSizeMode calls from before the first
nsWindow::Show call. Normally those have no effect, since the window isn't
shown yet, and if the window is not maximized, they typically match the
sizes we've gotten out of the registry anyway. However, if we are maximized,
then they produce a lot of visual noise. We can however achieve the desired
effect by just calling SetWindowPlacement.
Similarly, we switch the window styles of the skeleton UI window to match those
of the toplevel Windows window, and adjust the client rect from our window proc
in a way that matches the adjustments in nsWindow in the WM_NCCALCSIZE handler.
We do this because otherwise we get a flicker as soon as we change the styles
and nonclient margins as the fake chrome pops up and then back down.
Lastly we also change the extended window styles so that they match. We
historically added WS_EX_TOOLWINDOW here to hide the toolbar entry, because it
would otherwise switch out to a new toolbar entry when we changed the window
styles. However since our new styles match, we no longer need to do this. It
was also causing the maximized window to paint over the Windows taskbar.
Differential Revision: https://phabricator.services.mozilla.com/D93534
For a better user experience during slow startups (and to match the design
by Markus Jaritz), we want to animate the placeholder elements (the grey
rectangles which hold the place of text and icons) with a light moving
gradient.
A question may arise regarding whether the performance cost of running this
animation is worth the improved user experience. My claim is yes, hinging
mostly on the observation that the performance cost is small.
On my machine, one frame of the animation takes at most 0.15ms, and runs
every 16.15ms. This means we eat less than 1% CPU time on one core of the
system. It should also be noted that this animation runs primarily during
the window wherein we are prefetching dlls, AKA while we are blocked on IO
and the CPU is more likely to be idle.
On slower systems, one frame may take longer - however, on slower systems
we should also be blocked *more* by IO, making the trade favorable.
For further anecdotal evidence of this being okay, when I run this on slow
reference hardware shortly after OS startup, I do not see any dropped frames,
indicating that this has very little trouble completing, and is thus quite
cheap.
Regarding testing, I will invoke the same logic as for the rest of the
skeleton UI patches - it would involve quite a bit of work to test this in
automation given our existing frameworks. It may be worth it at some point,
but certainly not while this is a feature that we are just experimenting
with and which is not enabled by default.
Differential Revision: https://phabricator.services.mozilla.com/D91453
One of the scenarios in TestMMPolicy.exe is to reserve all regions but one free
region, and then FindRegion can find that free region. However, it intermittently
failed to find a free region on 32bit. Probably a different thread invoked by
the system reserved it before the main thread does. A proposed fix is to limit
the address range to reserve.
Differential Revision: https://phabricator.services.mozilla.com/D95188
Most of this patch is a dance to avoid size flickering of the skeleton UI
window. We change all Resize/Move/SetSizeMode calls from before the first
nsWindow::Show call. Normally those have no effect, since the window isn't
shown yet, and if the window is not maximized, they typically match the
sizes we've gotten out of the registry anyway. However, if we are maximized,
then they produce a lot of visual noise. We can however achieve the desired
effect by just calling SetWindowPlacement.
Similarly, we switch the window styles of the skeleton UI window to match those
of the toplevel Windows window, and adjust the client rect from our window proc
in a way that matches the adjustments in nsWindow in the WM_NCCALCSIZE handler.
We do this because otherwise we get a flicker as soon as we change the styles
and nonclient margins as the fake chrome pops up and then back down.
Lastly we also change the extended window styles so that they match. We
historically added WS_EX_TOOLWINDOW here to hide the toolbar entry, because it
would otherwise switch out to a new toolbar entry when we changed the window
styles. However since our new styles match, we no longer need to do this. It
was also causing the maximized window to paint over the Windows taskbar.
Differential Revision: https://phabricator.services.mozilla.com/D93534
For a better user experience during slow startups (and to match the design
by Markus Jaritz), we want to animate the placeholder elements (the grey
rectangles which hold the place of text and icons) with a light moving
gradient.
A question may arise regarding whether the performance cost of running this
animation is worth the improved user experience. My claim is yes, hinging
mostly on the observation that the performance cost is small.
On my machine, one frame of the animation takes at most 0.15ms, and runs
every 16.15ms. This means we eat less than 1% CPU time on one core of the
system. It should also be noted that this animation runs primarily during
the window wherein we are prefetching dlls, AKA while we are blocked on IO
and the CPU is more likely to be idle.
On slower systems, one frame may take longer - however, on slower systems
we should also be blocked *more* by IO, making the trade favorable.
For further anecdotal evidence of this being okay, when I run this on slow
reference hardware shortly after OS startup, I do not see any dropped frames,
indicating that this has very little trouble completing, and is thus quite
cheap.
Regarding testing, I will invoke the same logic as for the rest of the
skeleton UI patches - it would involve quite a bit of work to test this in
automation given our existing frameworks. It may be worth it at some point,
but certainly not while this is a feature that we are just experimenting
with and which is not enabled by default.
Differential Revision: https://phabricator.services.mozilla.com/D91453
Poison was setup at the start of xpcom init when that was assumed to be early enough.
Since then, Poison was added to Maybe, and Maybe has been used everywhere, including in
our channel implementation. As a result, poison was being used before it was initialized.
This basically meant our poison pointers were being replaced with null instead, which
dances into some more UB than accessing a page we have actually allocated. Also, tsan
noticed that accesses to the value were racing with the initializer actually being
called!
A (dynamic) static initializer forces the poison initialization as we can reasonably
hope without getting CallOnce or singleton patterns involved.
Other changes:
* Cleaned up the outdated documentation for mozWritePoison (the alignment
restriction was removed in Bug 1414901)
* Removed the poison supression from TSan
Differential Revision: https://phabricator.services.mozilla.com/D94251
Gerald Squelart
Razvan Maries
Alexis Beingessner
Jon Coppeard
Doug Thayer
smolnar
Hiroyuki Ikezoe
Andreea Pavel
Steve Fink
Ted Campbell
Andrew McCreight
Emma Malysz
Mihai Alexandru Michis
Alessio Placitelli
Bogdan Tara
David Major
Simon Giesecke
Kevin Jacobs
Jeff Muizelaar
Geoff Brown
Florian Quèze
Csoregi Natalia
Sylvestre Ledru
Christian Holler
Toshihito Kikuchi
Narcis Beleuzu
Gabriele Svelto
Butkovits Atila
Sebastian Hengst
Byron Campen [:bwc]