Enhance performance by attaching only active tabs to a webview (single-webview)

[petemill]

Description

At the moment, a <webview> element is created for each open tab in a window. This creates the following issues:

• Additional DOM elements
• <webview> overhead in muon - it's a plugin that has rendering overhead - the guestview assumes that a guest (attached to a webview) is visible, even if we make the invisible through css
• Attach events firing for all tabs at the same time seems to create an event storm that creates even further performance impact by each event creating one or more redux actions, which then may also attempt to make render changes.

In order to fix this, we aim to only have a single webview per active / previewed tab, switching the guest contents that are attached to the webview when the active or previewed tab changes.

This will mainly fix startup with a profile containing many tabs, but should also improve general responsiveness and possible memory usage.

Steps to reproduce:

Manual

Startup with a profile containing 4 windows with 150 tabs in each window. All tabs marked as unloaded: true (which is enforced in Brave on shutdown for all saved tabs)

Expected result

• App starts within a few seconds
• Only the active tab loads its content

Actual result

• App takes > 5 minutes to load
• Only the active tab loads its content

Automated perf test from #11424

Automated test (feature/perf-tests branch): 648bafd#diff-e5c305031ff191e0f8d1fb021abeb435

1. Start Brave
2. Open 100 tabs each with unique URLs
3. Close Brave
4. Open Brave. It should remember the 100 tabs.
5. Focus the URL bar, type a URL and hit enter to navigate.

Actual result:
Much slower with many tabs

Expected result:
Similar speed to a fresh profile

Prerequisites

• Ability to receive events for a tab in the render process without a webview.
  This allows us to not move every bit of content functionality to the browser process from the renderer process which, although will be beneficial for performance too, will be too big of a refactor to do at once.
  Status: Working implementation in muon/single-webview2 with petemill/browser-laptop/single-webview2

TODO

Bugs

Content-related

• Ability to load contents before tabs are attached to a GuestViewContainer.
  Currently, if a tab does not have a <webview>, it will not proceed with loading in the background. This is the same reason that in the current version of browser-laptop, there is a delay in loading background tabs whilst the frame/ is created and attached to. In the meantime 'Untitled' is displayed as the tab title. The WebContents SetOwnerWindow method is not called. Perhaps the contents are not activated until they are attached (guest_view_base.cc::ActivateContents checks if is attached)? It will also not fire its updated events when made active (although it will fire the set-active event, which we previously were not relying on). We need to allow tab's without <webviews> to load, whilst preserving lazy-load functionality via the discarded: true flag in a tab's createProperties, (e.g. background ctrl-click tabs)

• Background tabs remain active but do not continue processing requests or finishing navigation.
  - e.g. a tab which console.logs and reloads on a setTimeout, javascript will continue to be processed whilst tab is not attached to a <webview> and load-start & 'did-get-response-details' events will be received for tab, but not until the tab is re-attached will the tab send navigation-entry-commited & did-navigate & dom-ready, etc events
  - e.g. a tab which you switch to and whilst still loading switch away. The loading spinner will never stop because we don't receive those events.

• tab.discard() does not destroy/replace unattached tab contents
  Tab content memory will not be released if the tab is discarded whilst it is detached. When it is eventually re-attached (e.g. when it is made active) the tab will immediately discard at that point and then load again, which kind of defeats the purpose.

• many tabs startup crash: [36162:775:0322/105702.875384:FATAL:navigator_impl.cc(156)] Check failed: 0.

• move never-displayed tabs to a new window

• Background tab theme color is incorrect

  • workaround: wait for first window resize event

• Lazy-load tabs are loaded initially then reloaded on first visit

• background loaded tabs have initial DOM window inner{Width,Height} size of 0x0 (webcompat) (minor)

  • RenderWidgetHostViewGuest::GetViewBounds returns 0 x 0 size
    WebContentsImpl::GetSizeForNewRenderView returns the default 300 x 300 size

• detach then move tab to new window crash (check if fixed)

• clone tab (check if works)

Webview attaching / painting issues

• Webview cannot attach anything else if will-destroy of contents has been emitted whilst attached to it via guest
  For all subsequent <webview>.attachGuest() calls, those contents do not get attach events, even though web view / guest view container thinks it is attached successfully.
  Perhaps something else is being destroyed and not re-created / reset, inside browser_plugin_guest or browser_plugin_embedder.
  A new <webview> can attach to the tabs fine.
  Workaround: Another reason to use a new <webview> each time a tab is made active

• Very often, when a guest attaches to a it has already been attached to previously, the webview is white and will never paint, even when re-paint forced (hide and show in js)
  Workaround: (see webviewDisplay.js) Create and use a new each time we want to attach and display a guest.

• harden race conditions for rapid attach / detach / destroy (rapid cmd-w, tab switching, detaching)

• GuestView receives no events when not attached to webview, and duplicate events for guest when re-attached
  Solution: fixed in muon with brave/muon@2fbabc6 (new set-window event for assigning to first / subsequent window IDs, similar to add-new-contents), see <webview> / GuestView receives duplicate events for guest on re-attachment muon#515 requires review from @bridiver

• Removing from DOM destroys most-recently-attached WebContents, even if detached
  Solution: fixed in muon with brave/muon@32ebe0b (requires review with @bridiver) <-- reverted
  Modified guest_view_container not to call guest_view.destroy() on the last attached guest (detach() does not remove this.guest)
  We still want the guest to be destroyed when the web contents is, and that's ok because it's done in guest_view_base.cc WebContentsDestroyed()
  Status: Not a big blocker if we're not re-creating webview every active tab change.

Solved by webview pool (2 webviews)

• Very often, detaching a guest, attaching another guest and then re-attaching the original guest will result in a white , even if its to a different / new that has never been attached to before. Forcing a paint by resizing the or changing a css style attribute and then back again fixes this (although causing another flicker which we hide).
  Workaround: (see webviewDisplay.js) AttachGuest -> Hide -> Show . Unfortunate since this takes an additional > 50ms before we are sure the frame is visible.

• has brief white flash between attaches
  Workaround: (see webviewDisplay.js) Use 2 single-webviews, only hide the currently attached webview when the newly attached webview is attached.

State-related

• webview focus timing (especially re-open closed tab)
• tab title / loading status if events happen before event handler can be attached
  • solution, handle all tab events immediately on web contents creation, don't wait for react render cycle

Implementation

• active frame display

• implement tab preview

• Ability to move tabs between windows, even if they have not had a guest attached to their assigned window yet. @bridiver agress we implement something similar to WebExtension's tab.move(tabId, { windowId, index }): API to change a tab's windowId: tab.move() muon#489.
  Workaround: Working workaround implemented via using new 'set-window-id' event to remove frame from departing window, and bypassing the tab.detach() call if !tab.attached. The tab 'gets' to the new window since we always make a moved frame active, so it gets attached to a <webview>. Not ideal since a move to a new window could happen internally to muon, but works for basic browser-laptop tab moving.

Cleanup

• Tidy up frame.js - consider non-React class, and do not use fake DOM element as a stand-in for eventemitter.
• remove logging / put behind flag
• consider tab-detached-at should be emitted by embedder contents not tab

Testing

Areas to focus

Focus areas for testing are contained on #13783 but essentially:

• Tab creation, switching, closing and restore on restart
• webcompat of various sites, especially for tabs loading in background through cmd/ctrl-click links as well as foreground loading)
• entry and exit of fullscreen via on-site html buttons / keyboard shortcut (esc), and switching away from tab
• origin settings (exceptions for flash / noscript when navigating to different origins in the same tab, as well as allow-once vs allow-always for all origin settings inc fullscreen dont’t call to get site settings outside of mergeProps #9475)
• widevine popup (background-loaded tabs especially)
• Autofill
• Brave Shield settings
• Ledger tab activity time logging
• SSL (+EV) certificate and lock icon status

[petemill]

@bridiver I've updated the Description on this issue with my current progress. It's working fairly nicely now, with the caveat that I've had to fix a couple of things in the muon side, and implement some workarounds. Please see Bugs section above.

Most notably, I've had to go down a route of creating a new <webview> element each time we attach a new guest, as keeping them around was not reliable enough. That is perhaps the most severe workaround which may impact resource utilization - the other workarounds (keeping two <webviews> in a pool, hiding and showing to force re-paint) are more acceptable.

The <webview> bugs sound similar but are independent. Let me know if you want me to make a muon Issue for each with a more detailed STR that I can grab from my notes.

Still using single-webview2 branch on origin for both muon and browser-laptop.

[petemill]

@srirambv there is a test plan on this but essentially it's a full pass - this is the tracking issue for the whole single-webview release.

[kjozwiak]

Thanks @petemill, we'll mark this as QA/Checked once we've gone through the last remaining verification and manual passes before the release as this basically covers all the single webview work.

@srirambv @LaurenWags @GeetaSarvadnya @btlechowski we'll tag this issue as QA/Checked last once we've done everything else. Removing QA/test-plan-required and adding QA/test-plan-specified so it doesn't appear when searching for issues that are still marked as QA/test-plan-required.