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
If the urlbar is zoomed (defined by attribute breakout-extend), then we
scale it down to the unfocused coordinate values.
Differential Revision: https://phabricator.services.mozilla.com/D92303
The latest launcher process showed one of the top failures was `WriteProcessMemory` in
`CopyStubToChildProcess` failed with `ERROR_INVALID_ADDRESS` or `ERROR_NOACCESS`, that
is to store a trampoline address to the global variable of firefox.exe failed. Its root
cause should be the same as bug 1662560, the executable was loaded onto a different
address from the browser process.
The fix is to to expand the usage of `CrossExecTransferManager` to `FuncHookCrossProcess`
and `Kernel32ExportsSolver`.
Depends on D94652
Differential Revision: https://phabricator.services.mozilla.com/D94653
This patch introduces a class `CrossExecTransferManager` to manage the data
transfer from the current process to a remote process via `WriteProcessMemory`.
The class also encapsulates a logic to bridge the gap between two executable's
imagebase.
Differential Revision: https://phabricator.services.mozilla.com/D94652
This commit uses the new Markers 2.0 API for FileIO Markers. I had to
create another option for the MarkerStack class in order to conditionally
capture a backtrace inside of the Macro. Otherwise the macro invocation
failed.
Differential Revision: https://phabricator.services.mozilla.com/D93848
The new Markers 2.0 code had missed one detail:
Backtraces in markers were serialized as just the `ProfileChunkedBuffer`, which doesn't expose the original thread id like `ProfilerBacktrace` did.
Then when outputting the profile, the marker code would use the marker's thread id (where the marker should be displayed in the frontend, which *could* be different from where the backtrace came) to deserialize and stream the attached marker, and a special check in the streaming code meant that the mismatched id would ignore the stored sample, and the displayed marker would show no stack.
With this patch, when streaming stacks from markers, the given thread id is 0 (an impossible thread id), which indicates that whatever sample is present should be streamed.
`ProfilerBacktrace` doesn't need to store the thread id anymore.
This solves the above problem.
As a bonus, the streaming code now reports the original thread of the sample(s) it found. This could be used in the future, to better show in the frontend that some stacks may come from other threads.
Differential Revision: https://phabricator.services.mozilla.com/D94264
This commit uses the new Markers 2.0 API for FileIO Markers. I had to
create another option for the MarkerStack class in order to conditionally
capture a backtrace inside of the Macro. Otherwise the macro invocation
failed.
Differential Revision: https://phabricator.services.mozilla.com/D93848
Allow-list all Python code in tree for use with the black linter, and re-format all code in-tree accordingly.
To produce this patch I did all of the following:
1. Make changes to tools/lint/black.yml to remove include: stanza and update list of source extensions.
2. Run ./mach lint --linter black --fix
3. Make some ad-hoc manual updates to python/mozbuild/mozbuild/test/configure/test_configure.py -- it has some hard-coded line numbers that the reformat breaks.
4. Make some ad-hoc manual updates to `testing/marionette/client/setup.py`, `testing/marionette/harness/setup.py`, and `testing/firefox-ui/harness/setup.py`, which have hard-coded regexes that break after the reformat.
5. Add a set of exclusions to black.yml. These will be deleted in a follow-up bug (1672023).
# ignore-this-changeset
Differential Revision: https://phabricator.services.mozilla.com/D94045
Allow-list all Python code in tree for use with the black linter, and re-format all code in-tree accordingly.
To produce this patch I did all of the following:
1. Make changes to tools/lint/black.yml to remove include: stanza and update list of source extensions.
2. Run ./mach lint --linter black --fix
3. Make some ad-hoc manual updates to python/mozbuild/mozbuild/test/configure/test_configure.py -- it has some hard-coded line numbers that the reformat breaks.
4. Make some ad-hoc manual updates to `testing/marionette/client/setup.py`, `testing/marionette/harness/setup.py`, and `testing/firefox-ui/harness/setup.py`, which have hard-coded regexes that break after the reformat.
5. Add a set of exclusions to black.yml. These will be deleted in a follow-up bug (1672023).
# ignore-this-changeset
Differential Revision: https://phabricator.services.mozilla.com/D94045
Allow-list all Python code in tree for use with the black linter, and re-format all code in-tree accordingly.
To produce this patch I did all of the following:
1. Make changes to tools/lint/black.yml to remove include: stanza and update list of source extensions.
2. Run ./mach lint --linter black --fix
3. Make some ad-hoc manual updates to python/mozbuild/mozbuild/test/configure/test_configure.py -- it has some hard-coded line numbers that the reformat breaks.
4. Add a set of exclusions to black.yml. These will be deleted in a follow-up bug (1672023).
# ignore-this-changeset
Differential Revision: https://phabricator.services.mozilla.com/D94045
The latest launcher process ping showed one of the reasons why we failed to
detour `NtMapViewOfSection` is that `MMPolicyBase::FindRegion` failed to find
a free region. Inspecting the function carefully, there were three problems.
Firstly, `FindRegion` did not fully scan the given range. To randomize
the address of a free region we use, we start scanning from a random address
within the given range. The problem is we scan only addresses bigger than
that random address, without scanning smaller addresses. Probably this is
the reason why `FindRegion` fails.
Secondly, `FindRegion` may return an address not aligned with the allocation
granularity because `VirtualQueryEx` returns such an address. If that happens,
the subsequent mapping API fails with the alignment error.
Lastly, when we randomize an address to start scanning from, we divide a random
number by `maxOffset`, but with that, we never start scanning from the last
region. It does not affect the product's behavior, but to have fair randomization,
a divisor should be `maxOffset + 1`.
This patch fixes all of these three problems along with a new test program.
Differential Revision: https://phabricator.services.mozilla.com/D94110
Everything related to Base Profiler legacy markers can now be removed, only the new API from BaseProfilerMarkers.h should now be used.
Depends on D93737
Differential Revision: https://phabricator.services.mozilla.com/D93738
TextMarkerPayload is the only legacy marker type that is still used in the Base Profiler. We're converting it to the new BASE_PROFILER_MARKER_TEXT.
Depends on D93736
Differential Revision: https://phabricator.services.mozilla.com/D93737
Since the main thread is almost always being profiled, this makes it easy to direct important markers there (e.g., FileIO markers from unregistered threads).
Tech note: The `#include "BaseProfiler.h"` line was moved to BaseProfilerMarkersPrerequesites.h so that `MarkerThreadId::MainThread()` there can access `profiler_main_thread_id()`.
Depends on D93735
Differential Revision: https://phabricator.services.mozilla.com/D93736
These functions will be useful to get the main thread id, or check if we're in it, in some public code (e.g., markers).
Differential Revision: https://phabricator.services.mozilla.com/D93735
These unit tests were going through chunks very quickly in tight loops, which on some platforms could cause successive chunks to be marked "done" with the same timestamps.
Distinct timestamps are needed to uniquely identify chunks (in each process), and related assertions caught that. In real Firefox code, chunks are much bigger (around 1MB), and fill at a slower rate, so timestamps are always different as expected there.
To fix this artificial issue in our unit tests, delays are introduced before each `MarkDone()`, to ensure that they'll get a different timestamp every time.
Differential Revision: https://phabricator.services.mozilla.com/D93479
Along with the deserializer, we now also store other marker functions (that output the marker type name and schema).
These will be used in the following patch to output the combined schema list for all used marker types.
Differential Revision: https://phabricator.services.mozilla.com/D90659
Add `static mozilla::MarkerSchemaWriter MarkerTypeDisplay()` for each existing marker type.
Because all markers of a given type now must have the same payload and display schema, the DOM event marker has changed from type=tracing with category=DOMEvent, to its own type=DOMEvent, which is now accepted on the front-end.
Based on c9692715f2/src/profile-logic/marker-schema.js
Differential Revision: https://phabricator.services.mozilla.com/D90658
This class collects all the information necessary to stream the JSON schema that informs the front-end how to display a type of markers.
It will be created and populated in `MarkerTypeDisplay()`, functions in each marker type definition, with Add/Set functions (see following patch).
Based on c9692715f2/src/profile-logic/marker-schema.js
Differential Revision: https://phabricator.services.mozilla.com/D90657
`AutoArraySchemaWriter::FreeFormElement()` is never used, we can remove it.
When constructed, `AutoArraySchemaWriter` was optionally taking a `UniqueStrings` reference, which was stored as a pointer.
Then `AutoArraySchemaWriter::StringElement()` would do a dangerous `MOZ_RELEASE_ASSERT(mStrings);`.
The class is now split in two:
- `AutoArraySchemaWriter`, which does not deal with strings at all, so we don't need a pointer to `UniqueStrings`.
- `AutoArraySchemaWithStringsWriter` that can also deal with strings, in which we store a (non-null) `UniqueStrings` reference.
This is both:
- Safer, because it's not possible to instantiate the non-string writer and try to write a string.
- More efficient, because we don't need to pass&store a `UniqueStrings` reference/pointer when we don't deal with strings.
Differential Revision: https://phabricator.services.mozilla.com/D93191
This is the third attempt to investigate the launcher failure of our detour.
The previous commits d8315e4ed18d and 1b81ea85c43d added the assembly bytes
of a detour target and a special error code `DetourResultCode` to the launcher
failure ping.
In the latest telemetry data, however, the most common value of `hresult`
is still `ERROR_UNIDENTIFIED_ERROR`, meaning the previous commit missed to
set an error code in the common fallible codepath we wanted to know.
Besides `ERROR_UNIDENTIFIED_ERROR`, we're seeing `DETOUR_PATCHER_DO_RESERVE_ERROR`
in the telemetry, but having that code is not enough to pinpoint a falling
operation.
For further investigation, this patch adds ten more values to `DetourResultCode`.
`FUNCHOOKCROSSPROCESS_COPYSTUB_ERROR` is the last codepath we forgot to cover
in the previous commit. The values of `MMPOLICY_RESERVE_*` are to investigate
`DETOUR_PATCHER_DO_RESERVE_ERROR` in the MMPolicy level. In both cases, we add
the last Windows error code to `DetourError::mOrigBytes`.
Differential Revision: https://phabricator.services.mozilla.com/D92974
We blocked the older versions of database.dll as Bug 1566109 in 2019,
but the same crash is still happening with the newer versions.
We decided to block all versions because crashing in the middle of printing
or file uploading is not acceptable.
Differential Revision: https://phabricator.services.mozilla.com/D92988
It is possible to add options to a NoPayload marker, but stack and inner window ids would be lost because they are normally stored in the payload 'data' JSON object, which doesn't exist for NoPayload markers.
So in this case, we automatically change the marker to a new (internal) "NoPayloadUserData" type, which has a payload in which we can store options.
This is temporary, until bug 1646714 moves these options into the top-level marker JSON object.
Differential Revision: https://phabricator.services.mozilla.com/D93059
This is the third attempt to investigate the launcher failure of our detour.
The previous commits d8315e4ed18d and 1b81ea85c43d added the assembly bytes
of a detour target and a special error code `DetourResultCode` to the launcher
failure ping.
In the latest telemetry data, however, the most common value of `hresult`
is still `ERROR_UNIDENTIFIED_ERROR`, meaning the previous commit missed to
set an error code in the common fallible codepath we wanted to know.
Besides `ERROR_UNIDENTIFIED_ERROR`, we're seeing `DETOUR_PATCHER_DO_RESERVE_ERROR`
in the telemetry, but having that code is not enough to pinpoint a falling
operation.
For further investigation, this patch adds ten more values to `DetourResultCode`.
`FUNCHOOKCROSSPROCESS_COPYSTUB_ERROR` is the last codepath we forgot to cover
in the previous commit. The values of `MMPOLICY_RESERVE_*` are to investigate
`DETOUR_PATCHER_DO_RESERVE_ERROR` in the MMPolicy level. In both cases, we add
the last Windows error code to `DetourError::mOrigBytes`.
Differential Revision: https://phabricator.services.mozilla.com/D92974
The latest Windows Insider Preview (version 20226.1000) changes the machine code for BaseThreadInitThunk to have a preamble like the following:
00007FFDBF244C40 48 83 EC 28 sub rsp,28h
00007FFDBF244C44 85 C9 test ecx,ecx
00007FFDBF244C46 75 25 jne 00007FFDBF244C6D
00007FFDBF244C48 49 BA 70 A2 DC 12 6A 97 99 B0 mov r10,0B099976A12DCA270h
This patch adds "MOV r64, imm64" capability to the DLL interceptor so that we can hook this.
Differential Revision: https://phabricator.services.mozilla.com/D92146
This makes it clearer where marker-type-specific payload arguments start, just after the marker type object.
Also improved the main API documentation.
Differential Revision: https://phabricator.services.mozilla.com/D91681
The `category.WithOptions(...)` syntax was a bit strange and difficult to explain.
Now the category and options are separate parameters. Default options can be specified with `MarkerOptions{}` or just `{}`.
As a special case, defaulted-NoPayload functions don't need `<>`, and defaulted-NoPayload functions and macros don't even need `{}` for default options, e.g.:
`profiler_add_marker("name", OTHER); PROFILER_MARKER_UNTYPED("name", OTHER);`
Differential Revision: https://phabricator.services.mozilla.com/D91680
This makes it clearer where marker-type-specific payload arguments start, just after the marker type object.
Also improved the main API documentation.
Differential Revision: https://phabricator.services.mozilla.com/D91681
The `category.WithOptions(...)` syntax was a bit strange and difficult to explain.
Now the category and options are separate parameters. Default options can be specified with `MarkerOptions{}` or just `{}`.
As a special case, defaulted-NoPayload functions don't need `<>`, and defaulted-NoPayload functions and macros don't even need `{}` for default options, e.g.:
`profiler_add_marker("name", OTHER); PROFILER_MARKER_UNTYPED("name", OTHER);`
Differential Revision: https://phabricator.services.mozilla.com/D91680
The previous commit d8315e4ed18d introduced a new telemetry field
in the launcher process ping to collect the assembly pattern of
a target function on detour failure, but most of the crash instances
do not have a value in the field. This means the failure happens
before or after `CreateTrampoline`.
To narrow down the root cause, this patch puts a fine-grained error value
in the "hresult" field instead of the hardcoded ERROR_UNIDENTIFIED_ERROR.
This patch also adds `IsPageAccessible` check before fetching data from
a different process because fetching data from an invalid address hits
`MOZ_RELEASE_ASSERT` in `EnsureLimit`, resulting in crash without sending
the launcher process failure.
Differential Revision: https://phabricator.services.mozilla.com/D91881
The last Avast Antivirus's hook function contains `CALL [disp32]` instruction.
Our detour needs to be able to handle that pattern.
Differential Revision: https://phabricator.services.mozilla.com/D91155
This patch optimizes our detour's code handling Opcode 0xFF, expanding
its coverage to INC and DEC reg64 as well as PUSH and CALL.
Testcases for these scenarios are of course included.
Differential Revision: https://phabricator.services.mozilla.com/D91154
In the initial patches for bug 1656526, mhowell noticed that for startups which
take a very long time, if the user interacts with the skeleton UI window, the OS
will flag us as not responsive, which could be a poorer user experience than
seeing nothing. Since our UI is designed to look non-interactive anyway, we
assume that a better experience would be to simply squash the not responsive
response from the OS by trivially processing native events. It's not perfect in,
say, the event that startup is hung for some reason, but it's arguably preferable
to our old model of startup being hung, which was just nothing being displayed at
all.
Differential Revision: https://phabricator.services.mozilla.com/D91005
Alexis Beingessner
Ted Campbell
Andrew McCreight
Emma Malysz
Doug Thayer
Mihai Alexandru Michis
Alessio Placitelli
Bogdan Tara
David Major
Simon Giesecke
Kevin Jacobs
Razvan Maries
Jeff Muizelaar
Geoff Brown
Gerald Squelart
Florian Quèze
Csoregi Natalia
Sylvestre Ledru
Christian Holler
Toshihito Kikuchi
Narcis Beleuzu
Gabriele Svelto
Butkovits Atila
Sebastian Hengst
Byron Campen [:bwc]
Greg Tatum
Ricky Stewart
sanketh
Dorel Luca
Andreea Pavel
David Parks
Markus Stange