fix(electron-ozone): NativeWindowViews leak on rohtmlwidget cycle OS-21158#138
Closed
gostoshbs wants to merge 116 commits into
Closed
fix(electron-ozone): NativeWindowViews leak on rohtmlwidget cycle OS-21158#138gostoshbs wants to merge 116 commits into
gostoshbs wants to merge 116 commits into
Conversation
* workflows: Copy across BrightSign CI workflows from dev. * workflows: Temp change to debug new auth method * workflows: Another temp hack to debug * workflows: Another temp hack to debug * workflows: Change to use Github App authentication instead of using a PAT token. * workflows: Use version of ec2 runner with a fix for authentication in stop case. * workflows: Update ec2 runner version again. * workflows: Update to an AMI image that has pkg-config installed. * workflows: Use new leave_ec2_instance_running debug parameter from the ec2 runner action * workflows: Temp hack to stop EC2 instances being terminated after test run to debug failures. * workflows: Update to the ami that has pkg-config installed. * workflows: Update to a newer ami image that has dependencies installed properly. * workflows: Remove hack that set leave_ec2_instance_running to true for debugging purposes. * workflows: Switch to correct AWS account. OS-14873: Switch to using chromium export_tarball.py (#10) * workflows: Switch to using chromium export_tarball.py to create src tarball for release * workflows: Use ELECTRON_VERSION env var when calling export_tarball.py * workflows: Add missing $ when accessing env.SOURCE_RELEASE_FILE_NAME * workflows: Set src tarball upload to be after gclient sync again * workflows: Fix electron_package_name from .tgz to .gz * workflows: Remove commented out line Cherry picked from: 23ffb1e: feat: OS-14592 Create BrightSign CI workflows 5027fb7: OS-14873: Switch to using chromium export_tarball.py (#10)
* transparent redirect: Make redirects from one file to another transparent * webRequest: Add new test case to api-web-request-spec for transparent and normal redirects fix: Clear content-type and last modified from file fetches: OS-16593 (#39) Clear content-type and last modified from file fetches: OS-16593 Cherry picked from: b84c835: feat: OS-14993 Make redirects from one file to another transparent (#12) 558b42d: fix: Clear content-type and last modified from file fetches: OS-16593 (#39)
Description of Change Allow the passing of a quota to the session.fromPartition() API. This patch allows partitions to have a quota size, which in turn persuades the storage manager to run a garbage collector(at specific times) to free reclaimable storage space. Checklist - [x] PR description included and stakeholders cc'd - [x] npm test passes - [x] tests are [changed or added](https://github.com/electron/electron/blob/main/docs/development/testing.md) - [x] relevant documentation is changed or added - [x] [PR release notes](https://github.com/electron/clerk/blob/master/README.md) describe the change in a way relevant to app developers, and are [capitalized, punctuated, and past tense](https://github.com/electron/clerk/blob/master/README.md#examples). Release Notes notes: Allow the passing of a quota to the session.fromPath() API. Cherry picked from: 9555dc9: feat : backport, Allow partitions to have app set quotas OS-14197 (#14)
* wayland: Add BrightSign custom set-z-index feature A custom feature has been added to BrightSign Weston server to enable a z-index to be set to top level xdg windows. This patch adds the functionality into third_party/wayland and chromium ui/ozone/platform/wayland. * wayland: Add fixup patch to add missing include * fixup: Fix for storage quota test case
OS-14345: Add a webpreference to disable pinch to zoom In chromium pinch to zoom can be disabled globally using the command line switch disable-pinch. BrightSign has a requirement to beable to disable pinch to zoom at a browser window level. This patch adds enable_pinch_zoom as a webpreference and sets its value in RenderWidgetHostViewEventHandler. The webpreference will present on all platforms, but the change to set it is only present in aura (linux). The webpreference will only be used if disable-pinch has not been set on the command line. Cherry picked from: c3ca6fb: feat: Add a webpreference to disable pinch to zoom (#24) Signed-off-by: Tariq Bashir <120014322+t-bashir-bs@users.noreply.github.com>
Weston backend refuses to send deletion requests outside selection area. Newly written text is in selection area, but old text is not. This casues old text to be not deletable. Deleting outside the selection area doesn't seem to be a problem as in EXO window manager mentioned in the code comment. Cherry picked from: f768e1f: fix: Fix weston-keyboard backspace problem with existing text
wayland: Retry display server connection before giving up Brightsign weston server instance might not be up when Electron tries to connect to it. Added a patch to force 5 retries one second apart before giving up on it. Co-authored-by: Caner Altinbasak <cal@brightsign.biz> Cherry picked from: 7678214: fix: Retry wayland display server connection before giving up (#29)
Added the Chromium patch for integrating Brightsign video player with Chromium. fix: Fix assertion with multiple video player instances. OS-15582 (#25) Renderer diffrentiates between video player instances using the DelegateId. Derive WebMediaPlayerBrightsign from WebMediaPlayerDelegate::Observer and initialise WebMediaPlayerDelegate instance. Provide DelegateId when Observer requests for it to avoid assertion. video: Compositor integration for Brightsign Video Player Brightsign video player uses a customised version of VideoHoleFrames, which sends video player factory and z-index of the video frames. These are handled as Overlay planes. Our custom BrightsignUnderlay strategy sends the geometry, opacity and visibility information via libvid. feat: WebMediaPlayerBrightsign: Set new is_video_tag param in vid_player_load (#32) WebMediaPlayerBrightsign: Set new is_video_tag param in vid_player_load feat: Add latest fixes to compositor integration for Brightsign VideoPlayer (#33) feat: Add latest fixes to compositor integration for Brightsign Video Player Update the existing BrightSign custom BrightsignUnderlay strategy code with the latest fixes that were made in the QtWebEngine version. fix: Pass attributes to video_player and correct conversion OS-16220 (#34) The html5 element attributes were not been set when passed to video_player_load. Also, some time parameters were being converted from std::chrono to int64 when passed to the c wrapper. However, they weren't being converted back correctly. Specifically the duration parameter when set to std::chrono::milliseconds::zero() was being treated as an int64 and therefore ending up with an incorrect negative value. fix: Add fix for single video with hwz cannot be hidden OS-16518 (#35) webmediaplayer_brightsign: Apply the fix for OnceCallback crash OS-16749 Applied the webmediaplayer_brightsign for the OnceCallback crash found in OS-16749. Cherry picked from: dc2f2de: feat: Add support for brightsign video player f4078f1: fix: Fix assertion with multiple video player instances. OS-15582 (#25) 8609bc5: video: Compositor integration for Brightsign Video Player 6e1d724: feat: WebMediaPlayerBrightsign: Set new is_video_tag param in vid_player_load (#32) 51d48c1: feat: Add latest fixes to compositor integration for Brightsign VideoPlayer (#33) 0febd19: fix: Pass attributes to video_player and correct conversion OS-16220 (#34) 55e3edd: fix: Add fix for single video with hwz cannot be hidden OS-16518 (#35) 7017af4: webmediaplayer_brightsign: Apply the fix for OnceCallback crash OS-16749
dunfell has clang14. The following changes are required to build Electron with clang14
If Electron is built using ozone, but not with X11 support, compilation fails. HostDisplayClient conditionally compiles DidCompleteSwapWithNewSize with OZONE_PLATFORM_X11 flag. Do the same for our override.
Electron introduces X11 dependencies in wayland ozone platform backend. This is undesired behaviour.
This patch uses a the BrightSign SetZOrderLevel custom Weston extension to enable setting an absolute z-order for a window. Upstream-Status: Unsuitable
This change adds a call to SetOpacity for OZONE_PLATFORM_WAYLAND. It requires patches to Chromium that are included in the BrightSign Electron repo which contain the Chromium changes for supporting SetOpacity in the ozone wayland code. Upstream-Status: Unsuitable
Electron provides no implementation for SetIgnoreMouseEvents when using Wayland. This patch provides uses the Chromium mouse lock functionality to achieve a way of ignoring mouse events when using Wayland. The change goes together with changes in our Electron App to listen to the "focus" event and trigger a call to setIgnoreMouseEvents. This allows us to workaround the issue of mouse lock being automatically disabled when a BrowserWindow gains focus. Using mouse lock for disabling mouse events works fine for our use case.
When I modified the patch for video player and re-created patches, it turned out that all patches required some massaging. This commit is a combines them all in a single commit.
Interfaces have changes and WebInbandTextTrack does not exist any more. This patch is to get Electron compile with new interface.
Brightsign sets the z-order of the window with Wayland extensions. BrowserWindow.setAlwaysOnTop doesn't move the window to the top as Electron expects after this change. squash this to "Add bs z-order absolute level"
Electron doesn't support setting the opacity of the BrowserWindow on Linux platforms. Brightsign added support for changing the opacity of the window via wayland extensions. This is now a valid test for Linux. squash this to Add support for changing opacity on ozone wayland platform
Fix MacOS compilation
Fix ignore mouse event code compilation flag error
Add SetOpacity to linux ozone wayland: OS-16623
* fix: Fix test specs that rely on chrome-error * fix: Disable the error page being displayed on navigation errors
Brightsign video player is capable of playing back HLS stream. Added m3u8 to known mimetypes.
Patched Chromium to receive --video-codecs-supported and --audio-codecs-supported flags for defining decode capabilities of the players. Brightsign process knows the decoder capabilities and should define these when launching Electron.
* build: enable perfetto in Chromium * refactor: delete TracingControllerImpl * fix: TraceObject isn't present when v8_use_perfetto is true * fix: update lib/internal/http for perfetto * chore: remove stray log Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Add memoryPressureMonitor singleton for main process that wraps Chromiums memory pressure system. Enables listening for OS memory pressure events and programmatically triggering pressure notifications to force cache drops and GC. Changes: - New C++ API: shell/browser/api/electron_api_memory_pressure_monitor.* - New TypeScript wrapper: lib/browser/api/memory-pressure-monitor.ts - Methods: getCurrentPressureLevel(), notifyMemoryPressure(level) - Event: memory-pressure emitted on OS pressure changes - Tests: spec/api-memory-pressure-monitor-spec.ts - Docs: docs/api/memory-pressure-monitor.md - Registration: module-list.ts, node_bindings.cc, filenames.gni, typings
…S-20753, OS-20757 Remove IS_ANDROID guards from Chromium's cross-process memory pressure IPC to enable notifyMemoryPressure() to reach all renderer processes on all platforms via content::mojom::ChildProcess::OnMemoryPressure. Iterate RenderProcessHost instances in the browser process and forward the pressure level to each live renderer, matching the pattern used by UserLevelMemoryPressureSignalGenerator on Android.
We will use the new electron.memoryPressureMonitor to trigger memory pressure events. Electron's memoryPressureMonitor propagates events to render processes too. Remove our previous attempt where we relied on Chromium's own LinuxMemoryPressureMonitor and launching the monitor on both main and renderer processes.
Signed-off-by: Tariq Bashir <120014322+t-bashir-bs@users.noreply.github.com>
* feat: Optimise window show time OS-20603 * nexus: Fix window activation/deactivation
…-20963 The built-in PDF viewer was not working on BrightSign devices, showing only a grey background. The root cause was a combination of missing process isolation for extensions and context mismatches between incognito and original sessions. When site isolation is disabled via --disable-site-isolation-trials, all sites share a default SiteInstance with URL http://unisolated.invalid/. The extension system could not identify extension processes from this generic URL, so ProcessMap was never populated and RendererStartupHelper never activated extensions in their renderers. Adding a DoesSiteRequireDedicatedProcess override that returns true for chrome-extension:// URLs ensures extensions always get their own process with the correct site URL regardless of site isolation settings. SiteInstanceGotProcess was guarded behind IsOffTheRecord(), preventing ProcessMap insertion for incognito contexts. Since ExtensionRegistry and ProcessMap factories redirect incognito contexts to the original context, removing this guard allows extensions loaded in the default session to be found and registered in the ProcessMap when running in incognito. IsSameContext did a pointer comparison that failed when comparing an incognito context with its original context. Since RendererStartupHelper is keyed to the original context but guest renderer processes use the incognito context, InitializeProcess skipped them entirely and the renderer never received LoadExtensions or ActivateExtension messages. IsSameContext now also compares original contexts, matching Chrome browser behavior. The extension system services (ProcessMap, ExtensionRegistry, etc.) must be initialized for in-memory sessions via CreateBrowserContextServices so that KeyedService lookups and factory redirects work correctly. However LoadComponentExtensions is still skipped for off-the-record contexts to avoid a DCHECK in task_queue_util.cc that prohibits extension task queue operations on OTR contexts. Component extensions loaded in the default session are accessible from OTR sessions through factory redirects. The DCHECK on extension_system() was removed since the accessor may be called on incognito contexts.
Signed-off-by: Tariq Bashir <120014322+t-bashir-bs@users.noreply.github.com>
* fix: Fix for multiple videos that are not fully opaque * fix: Fix for hwz on for a video that is not fully opaque --------- Signed-off-by: Tariq Bashir <120014322+t-bashir-bs@users.noreply.github.com>
… windows Instead of manually tracking keyboard focus with GrabKeyboardEvents (which was reverted due to regressions), use the Weston-side zwp_surface_activation_control_v1 protocol to tell the compositor that non-activatable surfaces should not steal keyboard focus. When a WaylandWindow is created with activatable=false, we now: 1. Bind to zwp_surface_activation_control_manager_v1 in WaylandConnection 2. Get an activation control object for the surface 3. Call set_no_activate on it This prevents Weston from sending wl_keyboard.leave to the previously focused window when an unfocusable window is clicked, so physical keyboard input continues working alongside virtual keyboard windows. Also cleans up the orphaned GrabKeyboardEvents/UngrabKeyboardEvents code from the previous workaround.
I don't know how this got committed originally but it won't accept my patches on top of it without fixing this.
Windows with zero opacity (e.g. the virtual keyboard overlay when hidden) were still reporting as visible because Show() unconditionally sets visible_=true regardless of the current opacity. This caused the transparent keyboard window to intercept touch events meant for the underlying HTML widget on the Nexus platform. Fix IsVisible() to return visible_ && opacity_ > 0.0f so that zero-opacity windows are correctly treated as invisible for event dispatch in both CanDispatchEvent() and GetWindowAt().
…S-21049 When the topmost focused window is destroyed, keyboard_grabber_ was reset to kNullAcceleratedWidget with no fallback, causing all subsequent keyboard events to be silently dropped until a mouse click or touch reassigned focus. Fix RemoveWindow() to iterate the remaining windows in reverse order (topmost first) and transfer keyboard focus to the first activatable window. Also call Activate() on the new focus recipient so its delegate is notified via OnActivationChanged(true). Add NexusWindow::IsActivatable() accessor to support the check from the window manager.
Tooltip, popup, and menu windows were created without a Nexus surface role (eNone). After the SetZOrderLevel changes gave the main window a Graphics role, libbscompositor's role-based z-ordering placed the role-less overlay windows behind the graphics surface. Fix by assigning NEXUS_BrightSignSurfaceRole_eVirtualKeyboard to tooltip, popup, and menu windows at creation time. In the compositor's arrangement order (video > graphics > subtitles > ticker > virtual keyboard > cursor), this ensures overlays appear above the main graphics content.
Increase disk image size from 150GB to 200GB to resolve out of space failures during CI builds. Update ec2-image-id to the new AMI (ami-0aa7bb41d6a04e736) built with the larger disk.
#130) * Add further optimisations for Show using opacity change OS-20603 * fix(nexus): fix window opacity handling for show/hide transparency * fix: Semi transparent window case * fix semi-transparent window opacity not rendering on Nexus platform
…21158 Continuously creating and destroying Electron BrowserWindows in a loop causes the Electron main process's memory consumption to grow monotonically. Part of this is caused by NativeWindowViews::SetContentView, which when replacing a content view detaches the child from the view hierarchy but does not delete it. Under the views ownership model, child views are implicitly owned by their parent, so the detached subtree, including the WebContentsView and associated widget plumbing, becomes unreachable and leaks. Switch to RemoveChildViewT, which transfers ownership back to the caller as a unique_ptr that is immediately dropped to destroy the subtree. The existing RemoveChildView path is retained for views with owned_by_client() set, since the framework must not delete those (see crbug/40115694). Future upstream merges may address this leak differently. This change should be re-evaluated when Electron/Chromium is upgraded.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Continuously creating and destroying Electron BrowserWindows in a loop
causes the Electron main process's memory consumption to grow
monotonically. Part of this is caused by
NativeWindowViews::SetContentView, which when replacing a content
view detaches the child from the view hierarchy but does not delete
it. Under the views ownership model, child views are implicitly owned
by their parent, so the detached subtree, including the
WebContentsView and associated widget plumbing, becomes unreachable
and leaks. Switch to RemoveChildViewT, which transfers ownership back
to the caller as a unique_ptr that is immediately dropped to destroy
the subtree. The existing RemoveChildView path is retained for views
with owned_by_client() set, since the framework must not delete those
(see crbug/40115694).
Future upstream merges may address this leak differently. This change
should be re-evaluated when Electron/Chromium is upgraded.