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
This saves 1 byte when serializing each marker (and removes all the code that was related to the 2nd byte).
Also it will be easier to use it in legacy code that only knows about the category pair.
Added unit tests for the whole of MarkerCategory.
Differential Revision: https://phabricator.services.mozilla.com/D91110
This patch uses the prefs relevant to the pre-xul skeleton UI to control the
registry value. There are three prefs:
- A new pref intended to be the global toggle once all themes can be handled.
- browser.startup.blankWindow - we currently depend on the code in BrowserGlue.jsm,
gated on this pref, which immediately specifies the width and height of the
window and ultimately causes XUL to consume and take ownership of our pre-xul
window. Without this, we resize to fit a small content area, and then resize
again back to the correct size.
- extensions.activeThemeID - Given that we have hardcoded layout and colors, we
need to ensure that we're only presenting the skeleton UI for the default theme.
We're hoping to not need to gate on any prefs other than the new pref that we're
adding, but this allows us to provide a simple direction for users who may want
to dogfood our changes in the mean time: use the default theme, ensure the
browser.startup.blankWindow pref is set, and turn our new pref on.
Differential Revision: https://phabricator.services.mozilla.com/D90884
This patch bypasses `-Wunused-lambda-capture` warnings by using `Unused` as
Bug 1375386 did. Having two definitions with `#ifdef` confuses clang-format.
Using `Unused` seems like the easiest approach.
Differential Revision: https://phabricator.services.mozilla.com/D90610
Many instances of the launcher failure ping indicate hooking NtMapViewOfSection
or LdrLoadDll failed. This is most likely caused by a third-party application
applying a hook onto the same target earlier than we do.
This patch is to add a new field "detour_orig_bytes" in the laucnher failure ping
to collect the first sixteen bytes of a detour target function. With this,
we can know whether those detour failures were caused by a third-party hook or not,
and if yes, what was the actual binary pattern.
Differential Revision: https://phabricator.services.mozilla.com/D89836
This is currently failing the tier 2 MinGW-Clang builds, complaining
that DPI_AWARENESS_CONTEXT is not defined. Accordingly I pulled out the
logic from WinUtils.h which ensures it is defined into a shared header.
Differential Revision: https://phabricator.services.mozilla.com/D90467
Many instances of the launcher failure ping indicate hooking NtMapViewOfSection
or LdrLoadDll failed. This is most likely caused by a third-party application
applying a hook onto the same target earlier than we do.
This patch is to add a new field "detour_orig_bytes" in the laucnher failure ping
to collect the first sixteen bytes of a detour target function. With this,
we can know whether those detour failures were caused by a third-party hook or not,
and if yes, what was the actual binary pattern.
Differential Revision: https://phabricator.services.mozilla.com/D89836
The earlier fix ea452bb92e6a proved the executable's imagebase in a child
process is not always the same as the local imagebase. This patch applies
the new approach to retieve the imagebase from a handle to all channels.
Interestingly, we observed the launcher failures at `VirtualProtectEx` only
when launching a sandboxed process, not when launching the browser process.
In the long term, we may need to take care of all `WriteProcessMemory` calls
for a child process for greater safety, but given that observation, this
patch only updates `RestoreImportDirectory` and `InitializeDllBlocklistOOP`.
Differential Revision: https://phabricator.services.mozilla.com/D90316
We need this because otherwise we load user32, which fails the check in
WindowsDllBlocklist.cpp (line 649). It sounds like this check is non-
negotiable, so this is the only solution I can come up with. Obviously
please let me know if there is some reason we cannot do this, but it
seems to function fine.
Depends on D89670
Differential Revision: https://phabricator.services.mozilla.com/D90271
Hopefully the comments in the actual code are enough to explain what is going
on here.
NOTE: this patch does not represent a finished skeleton UI. There are some
questions in comments within the code, and generally I'm seeking feedback on
whether the overall approach seems sane or not. Gijs, I'm including you for
feedback on whether you think this is maintainable by more frontend-oriented
folks, and Molly, I'm including you for feedback on whether the justification
for writing to a raw pixel buffer seems sound or not, and a general review of
the approach.
Differential Revision: https://phabricator.services.mozilla.com/D86447
See bug for justification. This patch aims to display a blank window prior to
loading/prefetching xul.dll. It also has a placeholder for drawing a
skeleton UI into that window. Note that this is disabled by default based on
a registry value, as there are still kinks to work out (for instance, what
happens if we aren't actually going to display a window, because, say, Firefox
is already running.) This just gives a basic implementation to dogfood, and
facilitates distributing work across multiple contributors.
Onto the details. The patch achieves its goal by creating a window and
assigning its handle to a static variable, which will be consumed inside
nsWindow::Create by the first toplevel window we want to make. nsWindow::Create
will take ownership of the window handle, restyle it to its own liking, and
then proceed as if everything is normal and it had created the window itself.
Differential Revision: https://phabricator.services.mozilla.com/D86263
In most situations, JSONWriter users already know string lengths (either directly, or through `nsCString` and friends), so we should keep this information through JSONWriter and not recompute it again.
This also allows using JSONWriter with sub-strings (e.g., from a bigger buffer), without having to create null-terminated strings.
Public JSONWriter functions have overloads that accept literal strings.
Differential Revision: https://phabricator.services.mozilla.com/D86192
Hopefully the comments in the actual code are enough to explain what is going
on here.
NOTE: this patch does not represent a finished skeleton UI. There are some
questions in comments within the code, and generally I'm seeking feedback on
whether the overall approach seems sane or not. Gijs, I'm including you for
feedback on whether you think this is maintainable by more frontend-oriented
folks, and Molly, I'm including you for feedback on whether the justification
for writing to a raw pixel buffer seems sound or not, and a general review of
the approach.
Differential Revision: https://phabricator.services.mozilla.com/D86447
See bug for justification. This patch aims to display a blank window prior to
loading/prefetching xul.dll. It also has a placeholder for drawing a
skeleton UI into that window. Note that this is disabled by default based on
a registry value, as there are still kinks to work out (for instance, what
happens if we aren't actually going to display a window, because, say, Firefox
is already running.) This just gives a basic implementation to dogfood, and
facilitates distributing work across multiple contributors.
Onto the details. The patch achieves its goal by creating a window and
assigning its handle to a static variable, which will be consumed inside
nsWindow::Create by the first toplevel window we want to make. nsWindow::Create
will take ownership of the window handle, restyle it to its own liking, and
then proceed as if everything is normal and it had created the window itself.
Differential Revision: https://phabricator.services.mozilla.com/D86263
`MarkerOptions::Set()` returns the same reference type as the object it's invoked on, i.e.: & -> &, and && -> &&.
`MarkerOptions::NAME()` now always returns a reference to a `const` member, so it's clear it cannot be modified (even if the object at hand is not `const`).
`MarkerOptions::NAMERef()` must be used when non-`const` access is needed.
Differential Revision: https://phabricator.services.mozilla.com/D89715
The name `AUTO_PROFILER_MARKER_TEXT` is more consistent with the equivalent non-`AUTO` macro, and similarly arguments have been re-ordered to be the same, i.e.: Name, category&options, text.
The different macros with different argument sets can now be collapsed into one macro, and the optional arguments (timing, inner window id, backtrace) can easily be added to the `MarkerOptions` where needed.
As a bonus, a specific start time can optionally be provided at construction time.
Differential Revision: https://phabricator.services.mozilla.com/D89588
Mostly mechanical changes, with some work needed to convert the different payloads (with optional timestamps, inner window id, and/or backtrace) to the equivalent MarkerOptions.
Differential Revision: https://phabricator.services.mozilla.com/D89587
Mostly mechanical change, with some extra work where non-literal names are provided.
Also, when this is the only profiler call in a file, `#include "GeckoProfiler.h"` can be changed to `#include "mozilla/ProfilerMarkers.h"`.
Differential Revision: https://phabricator.services.mozilla.com/D89415
Our data indicates when the browser process populates a newly-created child process,
`VirtualProtectEx` may fail with `ERROR_INVALID_ADDRESS` for some unknown reason.
One possible cause is the parameter `aRemoteExeImage` of `RestoreImportDirectory`
was wrong i.e. pointing to an invalid address. We simply pass the local process's
imagebase as `aRemoteExeImage` based on the assumption that the same executable is
mapped onto the same address in a different process, but it may not be guaranteed.
To deal with that potential case, we could retrieve a correct imagebase from the handle
of a remote process as we do for the plugin process. Since we're not so sure about
the root cause or the effectiveness of this fix, we run it only when the first
attempt to `VirtualProtectEx` failed in Nightly. Once it's confirmed, we promote this
to a permanent fix.
Differential Revision: https://phabricator.services.mozilla.com/D89502
`ProfileChunkedBuffer` needed to be fully defined, because its destructor is needed to define `UniquePtr<ProfileChunkedBuffer>`.
It can just be empty, because it won't actually be used anyway.
Added non-`MOZ_GECKO_PROFILER` tests around this.
Differential Revision: https://phabricator.services.mozilla.com/D89351
This patch ports existing ProfilerMarkerPayload types to draft struct definitions that may be used with the new markers API.
This is just a starting point, they may be changed later on as needed, see meta-bug 1661394.
Differential Revision: https://phabricator.services.mozilla.com/D87259
This is the main public marker API:
- `AddMarkerToBuffer` can be used to store a marker to any buffer. This could be useful to code that wants to store markers outside of the default profiler buffers.
- `baseprofiler::AddMarker`/`profiler_add_marker` store a marker in the appropriate profiler buffer.
- BASE_PROFILER_MARKER and PROFILER_MARKER do the same, but are also defined (and empty) when MOZ_GECKO_PROFILER is not #defined.
All these take a name, marker options, a marker type, and the type's expected arguments if any (as expected by the `StreamJSONMarkerData` function).
Extra helpers for the most common types:
- BASE_PROFILER_MARKER_UNTYPED and PROFILER_MARKER_UNTYPED store a marker with no data payload.
- BASE_PROFILER_MARKER_TEXT and PROFILER_MARKER_TEXT store a text marker. `baseprofiler::markers::Text` is an example of how new marker types can be defined.
Differential Revision: https://phabricator.services.mozilla.com/D87257
Toshihito Kikuchi
Gerald Squelart
David Parks
Florian Quèze
Alexis Beingessner
Bogdan Tara
Markus Stange
Doug Thayer
Jon Coppeard
Dorel Luca