Do not create a new <webview> element or <Frame /> component for each Tab (single-webview) #13783

petemill · 2018-04-09T19:44:47Z

Fix #13216 of which the overall goal is to move towarrds making sure that performance of a renderer window is not affected by the number of tabs that are associated with that window.

Removes `<Frame />` Component

This PR not only removes the <webview> element for each frame, the problems of which are addressed in the attached issue, but also removes its React wrapper: the <Frame /> component.

The main issue with this component for performance is that for each tab in a window, a new instance of that component was being created. Then, each time the store dispatched an action, the Frame's mergeProps function would run. Even though this is a very quick function, this could add up to blocking for ~10% of the window's JS thread time, when there are > 150 tabs. In addition, the component had a componentDidUpdate function which would then dispatch further store actions based on the `mergeProps result, which would cause every Frame (and every other React component in the window) to perform this cycle again.

Previously, this component listened to events on its associated Tab's WebContents via routing through the <webview>. Instead of keeping this model, creating a new React element for each tab, and providing direct access to the WebContents event handler, we create a single event handler function instance that is called for all events on any tab in a window. Ideally these event handlers and dispatched actions should be performed by the browser process, but a change of that scope is too large to be done at once, especially with the already-large changes required by removing the multiple webviews.

Most event handler code is transported as-is from frame.js to rendererTabEvents.js and rendererShortcutHandler.js with the exception of items that relied on internal component instance state, including prevProps. These should be especially tested:

When navigating to a different origin, expire temporary site-settings for widevine, flash and noscript - 17c1814
When changing full-screen via in-content control, keyboard shortcut (esc), or switching tab, ensure that content area and window go in and out of full screen
Whenever frame state for a tab was changed after componentDidUpdate, the frame would dispatch an action to the browser each time, for each frame. This has been moved to a single-purpose function - rendererFrameTracker.js

Converted `app-new-web-contents-added` action to IPC

This action communicates to a window that it has a new frame, and adds it that window's frams state. Moved from store action to IPC because:
- Events are received much more immediately by the renderer now due to:
  - Tabs load immediately, not just when webview is attached
  - Event handlers are not only setup when component is created, there is a single event handler for all window tab events
- These events must have the frame created in state
- We can't simply listen for 'tab-inserted-at' as we need the frame data
- We can't simply pass it in on window creation with shared memory as window creation is not at all the only way a frame can be added to a window (e.g. moving tabs to existing window, using buffer window)

Creates `(Pooled)WebviewDisplay`

This is a vanilla JS class which controls the lifecycle of the <webview>, including attaching and detaching to different tabs. This is controlled by a React Component: <GuestInstanceRenderer /> which is a direct child of <Main />.
This class contains all the workarounds for quirks of the <webview /> including issues with not displaying content of an attached tab until the painting has been forced by hiding and showing, and handling fragility in timing when it comes to attaching to destroyed WebContents.
In order to prevent a 'white flash' between the detach of a previous tab and the attach of the next tab to display, we actually maintain a pool of 2 <webview> elements. We first attach the incoming Tab to an unused <webview>, make sure it is displaying, before we hide and detach the previously displayed tab from its <webview>. To this component, switching to a Tab, or viewing it as a preview is the same thing, it only cares about which tab should be currently displayed.
Should an incoing request to view a different Tab occur whilst the WebviewDisplay is still attaching to a new tab, it will either queue up the next Tab to be displayed after the current attachment is complete, or cancel the current attachment and begin attaching the newly-requested Tab instead, depending on the status of the attachment process.

Handles tab window attachment differently

Relying on did-attach and did-detach had to be changed, since these events are about attachment to a webview. Previously, the only time a tab was detached from a webview was when moving window, and the tab would always get a new webview in another window, even if it was a background tab. Now, every time we switch active tab one tab is detached from the webview and another is attached. We made new events in muon that specifically inform about being removed from or added to windows: tab-detached-at and tab-inserted-at, as well as a new muon api to tell a tab to move to a different window.

Other major muon changes were on the topic of ensuring that the tab content was fetched and loaded even when never having been attached - previously the tab would just be created but not actually do anything until it was made active.

Other optimizations

Plus a few more changes with the same goal of reducing performance impact of many tabs, windows, or items in state, such as:

Not changing the state.tabs Immutable.List when deleting a tab (setting to null instead)
Not forwarding frame objects for each tab back to every window in appState updates, since the window is where the frame state came from in the first place
Setting the New Tab theme color immediately, rather than waiting for it to be computed from DOM content

There's definitely more to be done in this area, especially with the amount of events locking up the browser and window threads on app startup with even a medium-sized profile (#14082), but I had to draw the line somewhere.

Logging

New flag --debug-store-actions now outputs each action at the time of dispatch in both the browser console, and each window console (with timing info and fancy colors).

Using the flag --debug-tab-events will now show specialized console output in the renderer windows (in addition to the previous log output in the main browser terminal / console) which will show

Each event from each Tab in the window
A console group for each attempt to attach a Tab to a <webview> in WebviewDisplay, including timings for each step.

A new Debug menu option "Display Tab Identifiers" will now display: Tab Id, Index, Guest InstanceId, FrameKey and FrameIndex on each Tab, as well as the the Tab which is currently being displayed (or is in the process of attaching to a <webview>, or the reason why it is not):

Misc

Addded support for BRAVE_SHOW_FIRST_RUN_WELCOME=1 env variable in order to debug issues with the 'about:welcome' tab which is created on a blank profile (but disabled by default in dev environment).

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.
Request a security/privacy review as needed. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

Full pass
Tests have been ongoing with the beta channel for a few weeks

Reviewer Checklist:

Request a security/privacy review as needed if one was not already requested.

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

bsclifton · 2018-04-10T07:00:06Z

app/renderer/components/main/main.js

@@ -736,31 +744,23 @@ class Main extends React.Component {
          : null
        }
      </div>
-      <div className='mainContainer'>
-        <TransitionGroup className={cx({


Since this is gone now, can we remove the package.json reference to react-transition-group?

I was just about to do this, but then realised that this may make the merge a bit more of a pain, especially package-lock.json, across branches. Perhaps a separate PR unless you think that it won't be a pain?

Separate PR is great 😄 👍 Let's leave it alone

bsclifton · 2018-04-10T07:10:41Z

app/renderer/rendererTabEvents.js

+      if (shouldDebug) {
+        console.debug('%cNo frame for event yet, queueing until later', 'color: red', tabId, eventType)
+      }
+      windowStore.on(`new-frame-${tabId}`, (newFrame) => {


should be .once(

jumde · 2018-04-10T22:24:14Z

app/browser/reducers/windowsReducer.js

@@ -24,6 +24,7 @@ const BrowserWindow = electron.BrowserWindow
 const firstDefinedValue = require('../../../js/lib/functional').firstDefinedValue
 const settings = require('../../../js/constants/settings')
 const getSetting = require('../../../js/settings').getSetting
+const { shouldDebugStoreActions, shouldDebugTabEvents } = require('../../cmdLine')


Not needed?

Thanks for the note @jumde! I haven't satisfied linter and some tests yet. Some of this code was written in an experimental mode with things added, removed, or changed very quickly. But right now we're pretty locked down on architecture and are in clean-up mode, so that's what I'm looking in to over the course of the next day or 2.

bsclifton · 2018-04-11T06:01:42Z

app/renderer/components/frame/guestInstanceRenderer.js

+
+// Components
+const ReduxComponent = require('../reduxComponent')
+// comment out to not use pooled webview (2x webviews):


If time allows, it would be much better to check for an environment variable instead of having the notes about commenting in here. Something like:

let WebviewDisplay if (process.env.USE_POOLED_WEBVIEW === false) { WebviewDisplay = require('../../webviewDisplay') } else { WebviewDisplay = require('../../pooledWebviewDisplay') }

Info could then be added to https://github.com/brave/browser-laptop/wiki/Command-line-flags

I did not end up doing this as we don't have access to process.env in the front-end, and whilst I could transfer such a property from browser to environment variable, I felt that this is not something (right now) someone other than a developer looking at the source code needs to experiment with, yet.... It seems like with improvements on C66, the need for the pooledWebview is less, and possibly we can replace with the simpler webviewDisplay in the next release, after I have more time to test properly. The purpose of pooledWebview is to avoid a flash of blankness (usually white) between attaches of different tabs, which does not seem as noticeable with C66 in initial testing. However, I did not feel comfortable making the permanent switch as that would require another couple rounds of testing to reach the same level of confidence we have with pooledWebview

bsclifton · 2018-04-11T06:03:34Z

app/browser/tabMessageBox.js

+    ) => {
+      const tabId = webContents.getId()
+      if (shouldDebugTabEvents) {
+        console.log(`Tab [${tabId}] window-confirm`)


this should be window-prompt

bsclifton · 2018-04-11T06:19:38Z

app/browser/reducers/tabsReducer.js

+function expireContentSettings (state, tabId, origin) {
+  // Expired Flash settings should be deleted when the webview is
+  // navigated or closed. Same for NoScript's allow-once option.
+  const tabValue = tabState.getByTabId(state, tabId)


Is this new functionality (seems like it)? Do you have a suggested way to test this specifically? (should an issue be created to capture this?)

it's old functionality. you can enable flash in prefs, then go to https://www.onlinemictest.com/webcam-test-in-adobe-flash/, right click on the flash element, click 'allow once', then close the tab and re-visit it. flash should be re-blocked.

bsclifton · 2018-04-11T06:24:29Z

app/browser/reducers/tabsReducer.js

-    case appConstants.APP_FRAME_CHANGED:
-      state = tabState.updateFrame(state, action, shouldDebugTabEvents)
+    case appConstants.APP_FRAMES_CHANGED:
+      for (const frameAction of action.get('frames').valueSeq()) {


might be good to check if frames is truthy (ex: action.has('frames')) before using

This action does not get called without that property. Should we have a policy of trusting our actions in the way that they are currently being called, or optimize for possible future mistakes?

bsclifton · 2018-04-11T06:59:44Z

app/renderer/rendererFrameTracker.js

+
+  // compare frames when state changes debounced, and only when thread is idle
+  const onWindowStoreChanged = debounce(() => {
+    idleCallbackId = idleCallbackId || window.requestIdleCallback(relayFrameChanges, { timeout: 1000 })


Could you put parens around this to make the logic obvious?

Without them (maybe because it's such a long line), it looks like the OR may only run if the assignment is falsey (OR has higher precedence of course).

ex:
idleCallbackId = (idleCallbackId || window.requestIdleCallback(relayFrameChanges, { timeout: 1000 }))

bsclifton · 2018-04-11T07:10:32Z

app/renderer/rendererFrameTracker.js

+      const frame = frames[frameKey]
+      const lastFrame = lastFrames[frameKey]
+      if (!lastFrame || !lastFrame.equals(frame)) {
+        changedFrames.push(Immutable.Map().set('frame', frame))


You should be able to initialize this in one go; such as:
changedFrames.push(Immutable.Map({frame}))

bsclifton · 2018-04-11T07:15:40Z

app/renderer/rendererShortcutHandler.js

+}
+
+function handleShortcut (frameKey, shortcut, e, args) {
+  console.log('got shortcut', shortcut, frameKey)


does this console.log need to be removed / commented out / wrapped with debug?

bsclifton · 2018-04-11T07:16:44Z

app/renderer/rendererShortcutHandler.js

+    }
+    case 'stop': {
+      const tabId = frameStateUtil.getTabIdByFrameKey(windowStore.state, frameKey)
+      console.log('getting webcontets', tabId)


this (and below) - more console.logs possibly needing removal

jumde · 2018-04-11T17:44:40Z

app/browser/reducers/tabsReducer.js

+    case appConstants.APP_TAB_SET_FULL_SCREEN: {
+      const isFullscreen = action.get('isFullScreen')
+      const tabId = action.get('tabId')
+      if (isFullscreen === true || isFullscreen === false) {


If the tab is already FullScreen, do we need to set it to full screen again?

This code was moved in 23f5b71. At this point, I believe that the tab content area is not fullscreen, but other UX may be (e.g. the window). Agreed that if that's the case, perhaps isFullscreen is not the best variable name here, although it's more of a state description - "This tab is in fullscreen mode or not, so tell the tab's content to go fullscreen too, or not)".

diracdeltas · 2018-04-24T23:37:59Z

app/renderer/webviewDisplay.js

+  }
+
+  createWebview () {
+    console.log('creating a webview')


extra console.log

I've left these console.logs in as this is an experimental class at the moment, which we may switch to in an upcoming release.

diracdeltas · 2018-04-24T23:38:32Z

app/renderer/webviewDisplay.js

+  }
+
+  attachActiveTab (tabId) {
+    console.log('webviewDisplay: attaching tab id', tabId)


extra console.log?

Thanks for the spot. I actually haven't done a pass of cleanup or lint fixing as we're still debugging a couple display issues.

diracdeltas · 2018-04-24T23:39:00Z

app/browser/windows.js

+          // TODO(petemill): race condition if multiple different tabs are moved at the same time
+          // ...tab-strip-empty may fire before all of those tabs are inserted to new window
+          win.webContents.once('detached-tab-new-window', () => {
+            console.log('departing tab made it to new window')


extra console.log?

@petemill

Auditors: @petemill

…emove other rendering workarounds Fix #13798 since "display: none" and then "display: ''" of a webview will cause the guest to (asynchronously) detach and attach again, and by that time we had attached another guest, which causes the original tab to be destroyed since we are attaching a guest to a webview which already has a guest. Luckily, it doesn't look like this workaround is needed in order to paint the first attachGuest of a webview anymore. Remove all composition workarounds from c65 as they are no longer needed for c66

Not pinned tab Fix #13797

@PeteMiller

auditors @PeteMiller

…sh fix

guest-ready could be sent to the original window whilst the frame object has already been sent to the browser / new window without the `guestIsReady` property. Instead of moving this to the browser process, I've removed the event handling entirely as it does not seem like we need to wait for guest-ready anymore (we weren't for attaching the Tab for preview mode anyway...).

Ensure 'tab-inserted-at' message is not missed when frame is mid-transit between windows. Ideally we would just use the property set on tab state, and not rely on frame state, but that would require renderer components reading from frame and tab state objects, instead of only frame objects, which would make those components' state selection functions more expensive.

Fix #14046 Avoid a tab being moved to a new window with a frame index higher than it should have. This occurred because when dropping on the blank area of a tab strip, there is no tab being dragged over. The last tab being dragged over could have been in another window, or could be the dragged tab itself, causing a frame to be created in the new window with an index that is too high, corrupting the "frame index <-> tabId" cache. Instead, we insert the tab at the beginning, and rely on muon to provide an updated tab index (if provided with an index of -1, muon will add the tab to the end of the strip).

Fix #14045 Previously this was set on window state, but was not used, possibly because it was not reliable as it did not get the initial value which is set internally in muon based on origin. Instead, when this was working, it was read directly from the webview, which we try to avoid due to the webview's multi-purpose nature. For example, the webview may not be displaying the active tab - it may be displaying a preview, or waiting to display the active tab asynchronously. Since there is not yet a muon Tab event for 'zoom-changed', we must manually update the zoomPercent tabState property every time we knowingly change the zoom level.

Fix #14044 Avoids race-condition where tab does not have window host webcontents in order to IPC messages to store properties on the frame. Note that not all error page data handling was moved. It would be best to move all load complete / fail handling from renderer to browser in one go. However, certificate errors are not provided on the WebContents, but provided as an app event, so this can be moved by itself more easily. Test Plan: - Test certificate error page has data, and the actions (ignore error and show certifiacte) work correctly when occuring in existing or background tab. - Test other load errors (e.g. bad domain or no network) display correctly when opened in existing or background tab.

Fix #14064 with a band-aid. A proper fix would be to insure that the correct frame / tab index is used when removing a tab from a tab strip and adding it to a new one.

…e is now reliable Fix #14080 Reintroduce `tabs.updateTabsStateForWindow` which should be called whenever we believe muon has new tab properties that we wouldn't have otherwise received events for. It seems that the only time this can happen anymore is when the index of tabs has changed because another tab, with a lower index, was moved or closed, so renamed the function `updateTabIndexesForWindow`. Also allows us to remove the call to dispatch the action `UPDATE_TAB` for each tab with modified properties, which was a performance issue. Instead, since we're only concerned with index change, we make sure that this is handled in the renderer on the appropriate events (tab inserted, removed, deleted, moved, pinned (which triggers moved)). Adds performance measuring for that function when using --debug-tab-events flag.

Fix #14081 Avoids having to send large diffs covering every property of every tab with an index higher than the one being removed. The browser diff and window patch operations are expensive.

…rge often-changing tabs[].frame.x properties These frame state objects are sent from the window to the browser, and whilst this could be looked at being removed as all the data should already be accessible on the tab state, they do not need to be sent back to the renderer, where they originated. The performance impact of this is measurable when there are a lot of frames being modified very quickly, such as app startup, tab removal, tab deletion or anything that should change many frame properties at once (such as index). Also do not add on 'bookmarked' property since I cannot find an instance of it being used. Remove double-setting of appStoreRenderer.state. Whilst this does have a check to make sure the argument is different each time it is called, it is not necessary to call it twice.

bsclifton

Had already reviewed before- been involved in recent testing and have looked at the code while @petemill has been fixing bugs. Great work on this! 😄

Do not create a new <webview> element or <Frame /> component for each Tab (single-webview)

petemill · 2018-05-11T17:15:49Z

0.23.x b088714
0.22.x-release3 / 0.22.x 2371c55

petemill added PR/work-in-progress ⚒ PR/needs-rebase ۞ labels Apr 9, 2018

petemill added this to the 0.22.x Release 3 milestone Apr 9, 2018

petemill self-assigned this Apr 9, 2018

bsclifton reviewed Apr 10, 2018

View reviewed changes

petemill force-pushed the single-webview branch 3 times, most recently from 71f2f78 to 7c03b6f Compare April 10, 2018 19:38

jumde reviewed Apr 10, 2018

View reviewed changes

petemill removed the PR/needs-rebase ۞ label Apr 10, 2018

petemill force-pushed the single-webview branch from 7c03b6f to 03801fa Compare April 10, 2018 23:11

bsclifton reviewed Apr 11, 2018

View reviewed changes

petemill force-pushed the single-webview branch from 777bf6b to 48f6d5c Compare April 11, 2018 17:25

jumde reviewed Apr 11, 2018

View reviewed changes

diracdeltas reviewed Apr 24, 2018

View reviewed changes

bsclifton mentioned this pull request Apr 26, 2018

Using Brave over several days with a lot of tabs can cause a freeze #13885

Closed

petemill force-pushed the single-webview branch 3 times, most recently from effaee5 to 5db1435 Compare May 3, 2018 06:02

petemill force-pushed the single-webview branch from 16e724e to d6f4ee9 Compare May 7, 2018 20:02

bsclifton and others added 20 commits May 10, 2018 21:34

Default action value to empty map to prevent call to get on undefined

dc4f184

Auditors: @petemill

Ensure new windows have non-pinned tab active

c4f98d9

Not pinned tab Fix #13797

update muon to 6.0.3

2a2fe7c

Update to muon 6.0.4 with Chromium 66.0.3359.117

a4ade29

muon v6.0.5

f2ca674

don't add non-tabs to webContentsCache

393a2c1

auditors @PeteMiller

Bump muon to 6.0.7 for webview src attribute fix and find-in-page cra…

b0a39ac

…sh fix

force updateTab on set-active to fix delay switching to unloaded tabs

ff508dc

update muon to 6.0.8

5e5cb6d

Ensure that frames are not inserted at an index higher than the maximum

3bf1520

Fix #14064 with a band-aid. A proper fix would be to insure that the correct frame / tab index is used when removing a tab from a tab strip and adding it to a new one.

Do not change every tab's index in state when deleting a tab

c40d998

Fix #14081 Avoids having to send large diffs covering every property of every tab with an index higher than the one being removed. The browser diff and window patch operations are expensive.

bump muon to 6.0.9 for extension api fix

6b125c4

petemill force-pushed the single-webview branch from fdbb4b8 to 7288f52 Compare May 11, 2018 04:36

petemill added initiative/perf and removed PR/work-in-progress ⚒ labels May 11, 2018

bsclifton approved these changes May 11, 2018

View reviewed changes

petemill merged commit 73886ff into master May 11, 2018

petemill added a commit that referenced this pull request May 11, 2018

Merge pull request #13783 from brave/single-webview

b088714

Do not create a new <webview> element or <Frame /> component for each Tab (single-webview)

petemill added a commit that referenced this pull request May 11, 2018

Merge pull request #13783 from brave/single-webview

2371c55

Do not create a new <webview> element or <Frame /> component for each Tab (single-webview)

petemill mentioned this pull request May 11, 2018

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

Closed

26 tasks

bsclifton deleted the single-webview branch May 31, 2018 16:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not create a new <webview> element or <Frame /> component for each Tab (single-webview) #13783

Do not create a new <webview> element or <Frame /> component for each Tab (single-webview) #13783

petemill commented Apr 9, 2018 •

edited

Loading

bsclifton Apr 10, 2018

petemill May 11, 2018

bsclifton May 11, 2018

bsclifton Apr 10, 2018

jumde Apr 10, 2018

petemill Apr 10, 2018

bsclifton Apr 11, 2018 •

edited

Loading

petemill May 11, 2018

bsclifton Apr 11, 2018

bsclifton Apr 11, 2018 •

edited

Loading

diracdeltas Apr 24, 2018

bsclifton Apr 11, 2018

petemill May 11, 2018

bsclifton Apr 11, 2018 •

edited

Loading

bsclifton Apr 11, 2018

bsclifton Apr 11, 2018 •

edited

Loading

bsclifton Apr 11, 2018

jumde Apr 11, 2018

petemill Apr 11, 2018

diracdeltas Apr 24, 2018

petemill May 11, 2018

diracdeltas Apr 24, 2018

petemill Apr 25, 2018

diracdeltas Apr 24, 2018

bsclifton left a comment

petemill commented May 11, 2018

Do not create a new <webview> element or <Frame /> component for each Tab (single-webview) #13783

Do not create a new <webview> element or <Frame /> component for each Tab (single-webview) #13783

Conversation

petemill commented Apr 9, 2018 • edited Loading

Removes <Frame /> Component

Converted app-new-web-contents-added action to IPC

Creates (Pooled)WebviewDisplay

Handles tab window attachment differently

Other optimizations

Logging

Misc

Submitter Checklist:

Test Plan:

Reviewer Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

petemill commented May 11, 2018

petemill commented Apr 9, 2018 •

edited

Loading

Removes `<Frame />` Component

Converted `app-new-web-contents-added` action to IPC

Creates `(Pooled)WebviewDisplay`

bsclifton Apr 11, 2018 •

edited

Loading

bsclifton Apr 11, 2018 •

edited

Loading

bsclifton Apr 11, 2018 •

edited

Loading

bsclifton Apr 11, 2018 •

edited

Loading