[wip] use CWVWebView embedder in iOS #24657

kylehickinson · 2024-07-15T17:46:23Z

Do not merge, meant for gathering feedback on this solution's direction

Running list:

Needs Migration

session restoration data check/migration: SessionTab holds onto WKWebView.interactionState, need to drop it or ignore it while restoring CWVWebView and vice-versa.
Optional: Swap to Chromium safe browsing implementation
Optional: Use HTTPS Upgrade tab helpers

Broken Features

SSL certificate pinning (not supported by Chromium)
block javascript shield (not supported by Chromium)
link previews (not supported by Chromium)
sampled top page color KVO (private API)
multiple downloads at once (UI-only: progress doesn't show combined, downloads are parallel now)
redirected loads (getInternalRedirect, loading a new request before cancelling an active load) on pages that fail to load in the end hit DCHECK (e.g. http://api.segment.io will get upgraded to https but the link itself is blocked by PiHole/content blockers)

Cosmetic Issues

Product Changes

user agent contains CriOS
error pages now match desktop/android but lose Brave branding
block scripts shield may be removed
iPad uses mobile user agent by default, controlled by pref
Chromium doesnt support link previews

Accesses WebKit

These access -[CWVWebView(Extras) underlyingWebView] or -[CWVWebView(Extras) WKConfiguration] directly and should be migrated over if possible

content blockers access (WKWebViewConfiguration)
cookie store access (WKWebViewConfiguration)
deprecated javascript control preference (WKWebViewConfiguration)
Sampled page top color (WKWebView - private API)
PDF data in Leo (WKWebView - private API)
zoom level (WKWebView - private API). There is FontSizeTabHelper as a replacement but it's worse
playlist web loader (WKWebView - loadHTMLString), only exposed for testing in Chromium

Cleanup

New Feature Support

Autofill Passwords
Autofill Credit Cards
Autofill Addresses

Security Checklist

TBD

chromium_src/ios/web_view/internal/webui/web_view_web_ui_provider.mm

kylehickinson · 2024-07-15T17:49:38Z

ios/web_view/BUILD.gn

+  common_flags = [ "-fapplication-extension" ]
+  cflags_objc = common_flags
+  cflags_objcc = common_flags
+  defines = [ "CWV_IMPLEMENTATION" ]


This needs to be redefined for cwv_exports.h to properly export the symbols since we're copying public headers as-is

kylehickinson · 2024-07-17T19:38:44Z

...rave-ios/Sources/Brave/Frontend/Browser/BrowserViewController/BVC+WKNavigationDelegate.swift

-    if webView.fullscreenState == .inFullscreen || webView.fullscreenState == .enteringFullscreen {
-      webView.closeAllMediaPresentations {
-        self.present(alertController, animated: true)
-      }
-      return
-    }


This is done by chromium already https://source.chromium.org/chromium/chromium/src/+/main:ios/web/web_state/ui/crw_wk_ui_handler.mm;l=124

ios/brave-ios/Sources/Brave/Frontend/Browser/Tab.swift

patches/ios-web_view-internal-cwv_web_view_configuration.mm.patch

bridiver · 2024-07-24T20:01:36Z

patches/ios-web_view-internal-cwv_web_view_configuration.mm.patch

+
+ - (CWVReuseCheckService*)reuseCheckService {
+-  if (!_reuseCheckService && self.persistent) {
+-    affiliations::AffiliationService* affiliation_service =


this AffiliationService seems concerning in general, I asked @ShivanKaul and @pes10k about it because it looks like something we might want to just replace with a no-op implemenation

Right now I don't compile any of the actual affiliation services (I just dont include them in the //brave/ios/web_view target) which is why this implementation is patched out and would return nil always. We'd likely go the route of compiling //ios/web_view as is though before landing this and no-op them with chromium_src overrides

it looks like this is something to do with credential sharing with apps, but we already disable it by disabling the fetch

What was the reason this was commented out?

Its commented out because we dont compile the necessary files for it atm (just //ios/web_view/internal/affiliations/web_view_affiliation_service_factory.h/mm at this point)

bridiver · 2024-07-24T20:32:17Z

chromium_src/ios/web_view/internal/app/application_context.mm

+ApplicationContext::~ApplicationContext() = default;
+
+PrefService* ApplicationContext::GetLocalState() {
+  return GetApplicationContext()->GetLocalState();


This looks a layering violation because I don't think ios/web_view is supposed to access ios/chrome/browser, but I'm a bit confused about why it has a separate ApplicationContext

Discussed in DM but will reiterate here anyways: ios/web_view basically has a whole "app" setup (custom ApplicationContext, custom WebClient, custom factories) because its completely independent from //ios/chrome. This isn't really helpful for us though because we already have the Chrome app setup in place, so we basically have to replace all of ios/web_views implementations with the Chrome ones. This may need better overrides to avoid layering violations though

bridiver · 2024-07-24T20:45:16Z

ios/web_view/BUILD.gn

+  ]
+}
+
+source_set("web_view") {


why are we adding these here instead of using web_view_sources, etc...?

It was originally to control what was compiled, the hope is to be able to eventually just use //ios/web_view:web_view_sources directly as more chromium_src overrides + patches are added

bridiver · 2024-07-24T20:46:14Z

ios/web_view/web_view_sources.gni

+
+# This file exposes the //ios/web_view embedder as a regular source_set
+
+ios_web_view_public_headers = [


is this different from ios_web_view_public_headers in ios/web_view/BUILD.gn? If so it would be better to use +=/-= instead of duplicating

The parts under our own 1 extra public header aren't different but since the original list is defined in a GN file and not GNI I couldn't access them to add into our framework so for now it was duped. If its possible to get to let me know!

ios/app/brave_core_main.mm

the underlying WebState needs to be associated with the Browser/WebStateList and be assigned a SessionID from the Browser so for now we'll go back to using the fake WebState to handle this. For FaviconDriver we can use WebFaviconDriver in the future but need to add an observer to Swift side to update Tab.favicon properly

This change removes a majority of underlying web view access which would create the web view too early for `target=_blank` style links and hit an assertion in WebKit

…anup

this should instead be patched/overridden at the service/factory level, we wont use this variable on CWVWebViewConfiguration either way

this is already handled by Chromium

rewrites `chrome` schemes to `brave` when formatting and loads them as standard requests

kylehickinson self-assigned this Jul 15, 2024

kylehickinson commented Jul 15, 2024

View reviewed changes

chromium_src/ios/web_view/internal/webui/web_view_web_ui_provider.mm Outdated Show resolved Hide resolved

kylehickinson commented Jul 15, 2024

View reviewed changes

kylehickinson added CI/skip Do not run CI builds (except noplatform) CI/skip-all-linters Do not run linters and presubmit checks labels Jul 15, 2024

stoletheminerals added the needs-security-review label Jul 16, 2024

stoletheminerals self-requested a review July 16, 2024 11:26

kylehickinson force-pushed the ios-cwvwebview branch from 9cf323a to 0817eb4 Compare July 17, 2024 19:36

kylehickinson commented Jul 17, 2024

View reviewed changes

ios/brave-ios/Sources/Brave/Frontend/Browser/Tab.swift Outdated Show resolved Hide resolved

kylehickinson force-pushed the ios-cwvwebview branch 5 times, most recently from 6ccffe6 to 66895b3 Compare July 23, 2024 17:08

bridiver reviewed Jul 24, 2024

View reviewed changes

patches/ios-web_view-internal-cwv_web_view_configuration.mm.patch Show resolved Hide resolved

bridiver reviewed Jul 24, 2024

View reviewed changes

kylehickinson force-pushed the ios-cwvwebview branch 2 times, most recently from 4be9903 to 0231609 Compare July 25, 2024 16:12

kylehickinson commented Jul 25, 2024

View reviewed changes

ios/app/brave_core_main.mm Show resolved Hide resolved

kylehickinson force-pushed the ios-cwvwebview branch 6 times, most recently from 042c989 to 39840e8 Compare August 1, 2024 14:56

kylehickinson force-pushed the ios-cwvwebview branch from 88bbb98 to 95bc4df Compare August 2, 2024 21:38

kylehickinson added 29 commits August 23, 2024 16:32

minimize changes in copied files

cc0c052

override factories with chrome factories, re-enable autofill/translate

600bb1f

remove unneeded chromium_src override on web controller

b7882e7

replace copied files with temporary patches

2a1c8a4

chromium user agent handling

2a8b344

fix web downloads (passkit, mobileconfig, ical, regular)

1350641

add autofill sync type

840c67d

fix tab titles on NTP

2caa98d

fix links that open new tabs & shift js execution to WebState

060b787

This change removes a majority of underlying web view access which would create the web view too early for `target=_blank` style links and hit an assertion in WebKit

use //ios/web_view dep directly now, initial gn check/presubmit cle…

b4d83cc

…anup

expose SecCertificate from CWVX509Certificate

72c3829

restore user scripts check in navigation action handler

4d2da92

restore universal link block feature

ebde2e5

use chromium error pages

54a2b3a

fix website redirects feature

83e7eb5

replace web view factories setup with chrome counterparts

f36269b

remove patch on CWVWebViewConfiguration.reuseCheckService

df20221

this should instead be patched/overridden at the service/factory level, we wont use this variable on CWVWebViewConfiguration either way

restore always request desktop site feature

5f965b8

restore basic auth handling

f489960

use correct ssl status error messages in certificate viewer

7b2f11b

[wip]: restore certificate pinning

b0a2810

remove mixed content upgrade content blocker

74d1401

this is already handled by Chromium

restore playlist web loader delegates

79161f4

better handling and display for webui schemes

64560c7

rewrites `chrome` schemes to `brave` when formatting and loads them as standard requests

restore link preview feature

ff6af5d

restore block scripts shield functionality

ddd2d58

fix alternative app store installs

93718d8

fix loading redirected request that may error after said redirected load

f851327

kylehickinson force-pushed the ios-cwvwebview branch from f284118 to f851327 Compare August 23, 2024 20:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wip] use CWVWebView embedder in iOS #24657

[wip] use CWVWebView embedder in iOS #24657

kylehickinson commented Jul 15, 2024 •

edited

Loading

kylehickinson Jul 15, 2024

kylehickinson Jul 17, 2024

bridiver Jul 24, 2024

kylehickinson Jul 24, 2024 •

edited

Loading

bridiver Jul 24, 2024

bridiver Jul 24, 2024 •

edited

Loading

kylehickinson Jul 24, 2024

bridiver Jul 24, 2024

kylehickinson Jul 24, 2024

bridiver Jul 24, 2024

kylehickinson Jul 24, 2024

bridiver Jul 24, 2024

kylehickinson Jul 24, 2024


		# This file exposes the //ios/web_view embedder as a regular source_set

		ios_web_view_public_headers = [

[wip] use CWVWebView embedder in iOS #24657

Are you sure you want to change the base?

[wip] use CWVWebView embedder in iOS #24657

Conversation

kylehickinson commented Jul 15, 2024 • edited Loading

Needs Migration

Broken Features

Cosmetic Issues

Product Changes

Accesses WebKit

Cleanup

New Feature Support

Security Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylehickinson Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Jul 24, 2024 • 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

kylehickinson commented Jul 15, 2024 •

edited

Loading

kylehickinson Jul 24, 2024 •

edited

Loading

bridiver Jul 24, 2024 •

edited

Loading