This makes mozilla::PrintfTarget consistent across all locales (not
printing e.g. "," instead of "." for the decimal point in floats in some
locales)
This implementation passes all the glibc tests in stdio-common/tfformat.c
except two tests because of the difference in how values like e.g 0.25 are
rounded. Printf in glibc and on MacOS, as well as Rust std::fmt, round to
nearest, ties to even. Double-conversion, as well as printf on Windows
and conversion functions in ECMAScript round to nearest, ties away from
zero.
The standard for printf says rounding is implementation-defined so
either way is technically correct.
Differential Revision: https://phabricator.services.mozilla.com/D102699
While at this point PrintfTarget doesn't use double-conversion, add a
test that it can (and thus will) handle the largest double output possible
with the default %f precision.
Differential Revision: https://phabricator.services.mozilla.com/D103431
It is worth noting that some of these tests fail on Windows for rounding
difference reasons (see later commit from this bug for more details),
and on both Windows and mac for differences in formatting for INFINITY
and NAN. All the tests pass on Linux (since the underlying printf is
currently glibc's).
Differential Revision: https://phabricator.services.mozilla.com/D103133
The standard for printf says that for integers, the result of converting
zero with an explicit precision of zero shall be no characters. But
flags and width still need to apply.
Differential Revision: https://phabricator.services.mozilla.com/D102696
%F and %G are the same as %f and %g, but using caps for the exponent
indicator, and for "inf"/"nan" for infinity and NaN.
%n$E is the same as %E, but taking the nth argument.
Differential Revision: https://phabricator.services.mozilla.com/D102695
This makes mozilla::PrintfTarget consistent across all locales (not
printing e.g. "," instead of "." for the decimal point in floats in some
locales)
This implementation passes all the glibc tests in stdio-common/tfformat.c
except two tests because of the difference in how values like e.g 0.25 are
rounded. Printf in glibc and on MacOS, as well as Rust std::fmt, round to
nearest, ties to even. Double-conversion, as well as printf on Windows
and conversion functions in ECMAScript round to nearest, ties away from
zero.
The standard for printf says rounding is implementation-defined so
either way is technically correct.
Differential Revision: https://phabricator.services.mozilla.com/D102699
It is worth noting that some of these tests fail on Windows for rounding
difference reasons (see later commit from this bug for more details),
and on both Windows and mac for differences in formatting for INFINITY
and NAN. All the tests pass on Linux (since the underlying printf is
currently glibc's).
Differential Revision: https://phabricator.services.mozilla.com/D103133
The standard for printf says that for integers, the result of converting
zero with an explicit precision of zero shall be no characters. But
flags and width still need to apply.
Differential Revision: https://phabricator.services.mozilla.com/D102696
%F and %G are the same as %f and %g, but using caps for the exponent
indicator, and for "inf"/"nan" for infinity and NaN.
%n$E is the same as %E, but taking the nth argument.
Differential Revision: https://phabricator.services.mozilla.com/D102695
This will disable the skeleton UI if we crash while setting it up, and further
the error will propagate up and get reported via telemetry on the next run.
This is important because we don't have anything like the crash reporter set up
by the time we want to start showing the skeleton UI.
Differential Revision: https://phabricator.services.mozilla.com/D102355
This is, for the most part, just a large refactor of the skeleton UI stuff to
support coarse-grained error reporting via telemetry. There are a few slight
changes in how we handle some errors. For example, if CreateWindow fails, we
will now bail out and report the failure. The flow for the happy path, however,
should remain unchanged.
Differential Revision: https://phabricator.services.mozilla.com/D102098
This patch displays the new telemetry field "loadStatus", which was introduced
by the earlier patch, on about:support. With this information, a user can see
each of injected modules was really loaded or blocked by our DLL blocklist.
Differential Revision: https://phabricator.services.mozilla.com/D102409
We used to record a DLL loading event only when a module was loaded.
With this patch, we record an event for a module blocked by our DLL
blocklist as well as a loaded module. It is achieved by calling
to `ModuleLoadFrame::NotifySectionMap` in `patched_NtMapViewOfSection`
regardless of the block action.
This patch introduces a new member `ModuleLoadInfo::mStatus` and
`ProcessedModuleLoadEvent::mLoadStatus` to keep the DLL loading status,
which will be added to the third-party-modules ping by a following patch.
Differential Revision: https://phabricator.services.mozilla.com/D102407
This patch displays the new telemetry field "loadStatus", which was introduced
by the earlier patch, on about:support. With this information, a user can see
each of injected modules was really loaded or blocked by our DLL blocklist.
Differential Revision: https://phabricator.services.mozilla.com/D102409
We used to record a DLL loading event only when a module was loaded.
With this patch, we record an event for a module blocked by our DLL
blocklist as well as a loaded module. It is achieved by calling
to `ModuleLoadFrame::NotifySectionMap` in `patched_NtMapViewOfSection`
regardless of the block action.
This patch introduces a new member `ModuleLoadInfo::mStatus` and
`ProcessedModuleLoadEvent::mLoadStatus` to keep the DLL loading status,
which will be added to the third-party-modules ping by a following patch.
Differential Revision: https://phabricator.services.mozilla.com/D102407
This is no longer going to be detected by tsan. I figure with full shutdown enabled we should be able to see if there were other issues around this one.
Differential Revision: https://phabricator.services.mozilla.com/D102487
Thunderbird does not yet have the same blocklist initialization as Firefox, so we skip calling InitDllBlocklistOOP to avoid a MOZ_RELEASE_ASSERT.
Differential Revision: https://phabricator.services.mozilla.com/D99173
This patch is to improve the way to detect an injected dependent module for
automatic DLL blocking (bug 1659438).
In the previous version, we created a list of dependent modules in the launcher
process and shared it with other processes via the shared section. However, it
was not compatible with third-party applications who tamper the Import Table and
revert it in the injected module's DllMain (bug 1682834) because we parsed the
Import Table in the launcher process after it was reverted.
With this patch, we check the Import Table in `patched_NtMapViewOfSection`,
so we can see tampering before it's reverted. More specifically, we create
a list of dependent modules in the browser process as below.
1. The launcher process creates a section object and initializes
the kernel32.dll's functions in it.
2. The launcher process transfers a writable handle of the shared
section to the browser process.
3. In the browser process, if an injected dependent module is being
mapped by `NtMapViewOfSection`, we add its NT path to the shared
section and block it with `REDIRECT_TO_NOOP_ENTRYPOINT`.
4. The `main` function of the browser process converts the writable
handle of the shared section into a readonly handle.
5. The browser process transfers a readonly handle of the shared
section to a sandbox process.
Since automatic DLL blocking may still cause a compat issue like bug 1682304,
we activate it only in Nightly for now.
Differential Revision: https://phabricator.services.mozilla.com/D101460
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 you'd
expect we'd be able to go back to the straightforward implemention that works even
if stderr has been redirected. Unforunately, Windows takes not buffering
stderr very seriously and fprintf will write out the results character
by character. This can cause log output lines to be intermixed which
breaks log parsing in CI. We keep using fdopen to create a new FILE*
that's buffered but instead of hard coding fd 2, we get the actual fd
that corresponds to stderr using fileno.
The mozglue implementation was cargo culted from xpcom, so we update it
as well.
Differential Revision: https://phabricator.services.mozilla.com/D98550
CLOSED TREE
Backed out changeset f82f5070bee5 (bug 1205985)
Backed out changeset 89b03879ce7d (bug 1205985)
Backed out changeset 9ba60febbcf8 (bug 1205985)
This eliminates all of the thread leaks we had on record, while also increasing
our coverage on the code that's used for testing firefox.
Differential Revision: https://phabricator.services.mozilla.com/D100264
`ImageBridgeChild` was turned into a one-off thread due to a deadlock, but when bug 1672255 is finished we can revert the changes back to a background task queue without any issues and get rid of these suppressions.
Differential Revision: https://phabricator.services.mozilla.com/D100257
The `RunningTimes` class stores CPU measurements. It may seem overkill for only one value, but in the future more measurements will be added.
During sampling, CPU measurements are collected by platform-specific code. This patch doesn't produce anything yet, see later patches.
These are stored with the samples.
Note that for duplicated samples (when a thread is known to be "asleep"), we still need to collect new measurements, because there could potentially be some activity happening, e.g. in system calls.
Finally the measurements are output as extra "samples" values.
Units for these values may platform-specific, so they are stored in the top-level JSON "meta" object.
We don't collect running times in the Base Profiler (yet), but we still need to add the appropriate field names in the samples' "schema", as expected by profiler.firefox.com.
Differential Revision: https://phabricator.services.mozilla.com/D99413
This patch adds "CPU Utilization" ("cpu" for short) as a new feature that will control the upcoming still-experimental CPU measurements.
Differential Revision: https://phabricator.services.mozilla.com/D99054
Instead of only capturing one feature (NoStackSampling), the sampler thread now stores all features so that any feature can be quickly looked at during sampling.
Currently this is still limited to NoStackSampling, a later patch will start using another feature.
Differential Revision: https://phabricator.services.mozilla.com/D99053
Now is already supported when CLOCK_MONOTONIC is supported, but
ComputeProcessUptime is not. This shares the code with other BSDs, and
makes it look like the implementation in Timestamp_darwin.cpp.
Eventually, we'll remove the one from Timestamp_darwin.cpp.
Differential Revision: https://phabricator.services.mozilla.com/D100069
Some sites do have stacks that require more than 64KB to store in the profiler buffer.
Note that this only affects one semi-permanent buffer per process during profiling, and short-lived buffers when capturing stacks in markers.
Differential Revision: https://phabricator.services.mozilla.com/D99981
The "expected maximum stack size" (currently 64KiB) value was present in multiple places.
Now it's accessible from everywhere as ProfileBufferChunkManager::scExpectedMaximumStackSize, so it's easier to modify as needed.
Differential Revision: https://phabricator.services.mozilla.com/D100222
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
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
Florian Quèze
Yaron Tausky
smolnar
Alexis Beingessner
Mike Hommey
Csoregi Natalia
Toshihito Kikuchi
Doug Thayer
Kris Wright
Emma Malysz
Butkovits Atila
Greg Tatum
Geoff Lankow
Molly Howell
Razvan Maries
Brindusan Cristian
Gerald Squelart
Paul Adenot
Jeff Muizelaar
Mihai Alexandru Michis
Narcis Beleuzu
Masatoshi Kimura
Jon Coppeard
Hiroyuki Ikezoe
Andreea Pavel
Steve Fink
Ted Campbell
Andrew McCreight
Alessio Placitelli
Bogdan Tara
David Major
Simon Giesecke
Kevin Jacobs
Geoff Brown
Sylvestre Ledru
Christian Holler
Gabriele Svelto
Sebastian Hengst
Byron Campen [:bwc]
Ricky Stewart
sanketh
Dorel Luca