[iOS] Use Chromium web embedder (`CWVWebView`) #24657

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

Resolves brave/brave-browser#38667

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: Use HTTPS Upgrade tab helpers
- Expose clearing HttpsUpgradeAllowlist when clearing browser history
- Clear allowlist on pref change (will be handled in fix(privacy): Use HttpsUpgradeService / HttpsOnlyModeAllowlist on iOS #25454)

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)
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)
CWVWebViewConfiguration teardown needs to be added to WebMainParts

Bugs

multiple downloads at once (UI-only: progress doesn't show combined, downloads are parallel now. Chrome only allows one download a time, so we're doing the same for now)
URL debounce breaks certain load behaviours (Load this TomsGuide page and click on the Amazon refs, its supposed to open a new tab because its marked as target=_blank but debounce breaks this)
Crashes when attempting to start a download that is triggered on a new tab (e.g. https://1password.com/downloads/mac)
Popup/child tabs receive JS messages from the parent tab incorrectly
Back/forward navigation with on http websites hits a DCHECK (Chromium bug)

Cosmetic Issues

Product Changes

user agent contains CriOS
error pages now match desktop/android but lose Brave branding
iPad uses mobile user agent by default, controlled by pref

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). There is web::BrowserState::GetCookieManager() that may be a replacement
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
safe browsing private API for sec fix (WKWebView - private API)

Cleanup

New Feature Support

These will get follow-up issues and can be implemented after this merges

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

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

ios/web_view/BUILD.gn

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

… and buttons not working.

kylehickinson · 2024-12-09T18:15:56Z

@bridiver @stoletheminerals could you guys prioritize reviewing this in the near future? It's been in limbo for quite a while 😄

kylehickinson · 2024-12-09T18:17:36Z

chromium_src/ios/web/web_state/ui/wk_web_view_configuration_provider.mm

+
+namespace brave {
+
+// FIXME: Replace patch + override with WKWebViewConfigurationProviderObserver


WKWebViewConfigurationProviderObserver has actually been removed from Chromium at some point, so will need an alternative "proper" fix

kylehickinson · 2024-12-09T18:20:26Z

ios/brave-ios/Sources/Brave/Frontend/Browser/BrowserViewController/BVC+WKDownloadDelegate.swift

-      try? await AsyncFileManager.default.removeItem(at: url)
+      let proposedFilenameWithoutExtension = "\(filenameWithoutExtension) (\(count))"
+      proposedPath = downloadsPath.appending(path: proposedFilenameWithoutExtension)
+        .appending(path: fileExtension)


Needs to be fixed same way as https://github.com/brave/brave-core/pull/26865/files

cdesouza-chromium · 2024-12-09T18:59:19Z

I don't want to barge in here with unreasonable demands, specially because there's a lot of work here, but this is PR is too large. Can this work be broken apart into smaller changes? I don't want to be insensitive about authors time, but I'm incredulous we can have accurate reviews of work of this scale.

kylehickinson · 2024-12-09T19:25:57Z

@cdesouza-chromium unfortunately there isn't a great way to break this up that would help with reviewing. This PR consists of:

Support for using ios/web_view (the bulk of this change, chromium_src overrides + patches)
Additional Obj-C pieces around ios/web_view (in brave/ios/web_view) that expose additional properties required to implement certain Brave features
The Swift changes in the iOS app to use the above 2

Even if I pulled out the top part into its own PR it would still be a lot of code to review, and additionally would be impossible to test without the other 2 bits anyways

cdesouza-chromium · 2024-12-09T19:30:45Z

I think we have a problem in Brave that needs some better approach for these cases. Changes this large do not occur in upstream Chromium. They'll simply refuse to review. The way these types of changes are handled is that you have a deprecation/migration/deletion pathway where things slowly get delivered, but this requires us to change the way we approach code changes.

kylehickinson · 2024-12-09T19:46:21Z

@cdesouza-chromium yeah we mostly build things until they're done before making a PR instead of introducing several sub-issues that progressively build towards an end goal. iOS is also a bit trickier to test because we currently can't run unit tests from core in the iOS build due to how we export a framework thats imported by the iOS app (can't even export test code because of testonly GN flags)

As for this PR, if we really want to deem this un-reviewable in its current state, I can create a sub-issues for each "part" to split up but it'll take a bit of work to split it up nicely and inevitably there will still be one or two larger PRs due to the nature of the project and i'll likely need to set up some sort of shell UI to test things per PR

cdesouza-chromium · 2024-12-09T19:57:39Z

As for this PR, if we really want to deem this un-reviewable in its current state, I can create a sub-issues for each "part" to split up but it'll take a bit of work to split it up nicely and inevitably there will still be one or two larger PRs due to the nature of the project and i'll likely need to set up some sort of shell UI to test things per PR

Well, now I feel bad, because it feels unfair to impose such a demand all of a sudden, for something you've worked hard, and you had no such requirement.

We can potentially review this, but we have a few draw backs. now:

Review most likely will be less effective.
As a reviewer I could very well now be wasting your time if I decided to ask you to experiment with something that may not work, and may involve changing a lot of files. So we can end up in a situation where the reviewer is reluctant to ask, or the author is reluctant to try, which is understandable.

Again, I'm not sure what is the best course here, and maybe we should take the punch with this large PR and proceed with it now that it is here while no formal guidelines on this exist (which I'm working on draft for), because it does seems unfair to demand this all of a sudden (better to ask forgiveness than permission and all that), but I think it would be good to raise awareness going forward.

kylehickinson · 2024-12-09T20:06:49Z

Well this PR has been open for a long time and has gone back and forth on a numerous implementation details since then so mostly my goal is to get it over the finish line 🙂 If splitting it up will do that then I can take a whack at it, there's just no promises that the end result will be any easier to review 😅

bridiver · 2024-12-09T20:34:08Z

It is quite large which makes it very hard to review, but also not sure if breaking it up will make it easier to review. I'll take a look and see if there are parts that make sense to split up. Possibly splitting the chromium changes from the brave-ios changes?

bridiver · 2024-12-09T21:17:25Z

chromium_src/ios/chrome/browser/shared/model/prefs/pref_names.h

+
+namespace prefs {
+// A boolean specifying whether HTTPS Upgrades are enabled.
+inline constexpr char kHttpsUpgradesEnabled[] = "ios.https_upgrades_enabled";


we normally wouldn't put this kind of stuff in a chromium_src override, but it appears that we're doing that quite a bit already in ios. We shouldn't use chromium_src overrides as a replacement for brave code that should go under ios. So for instance above in BraveRegisterBrowserStatePrefs should really go in ios/brave/browser/shared/model/prefs/browser_prefs.mm and same here.

kylehickinson · 2024-12-10T18:16:31Z

@cdesouza-chromium @bridiver @stoletheminerals
Closing this in favor of splitting up the PR into multiple smaller PRs, first one is up: #26957

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