[webview_flutter_wkwebview] Fixes inspectable compile-time error and crash from equal `NSURL`s #4340

bparrishMines · 2023-06-28T19:54:22Z

Fixes flutter/flutter#128422 by using pointer equality in the NSMapTable. From my understanding and testing it seems that the default mode for NSMapTable is to copy objects that implement NSCopyable. The only class we wrap that does seems to implement NSCopyable is NSURL so this class eventually led to a race condition where adding an NSURL that was equivalent to an already added one would replace the original.

Note that we should probably now add an error when adding an already present instance. However, a clear method needs to be added to the InstanceManager first so that hot reload will continue working. This is the current solution on Android.

Fixes flutter/flutter#129587 by adding a compile-time check around the use of WKWebView.inspectable. Solution used from:

https://stackoverflow.com/questions/76216183/how-to-debug-wkwebview-in-ios-16-4-1-using-xcode-14-2

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

cyanglaz

LGTM

stuartmorgan-g · 2023-06-29T12:29:22Z

packages/webview_flutter/webview_flutter_wkwebview/ios/Classes/FWFWebViewHostApi.m

@@ -201,7 +201,10 @@ - (void)setInspectableForWebViewWithIdentifier:(NSNumber *)identifier
                                   inspectable:(NSNumber *)inspectable
                                         error:(FlutterError *_Nullable *_Nonnull)error {
  if (@available(macOS 13.3, iOS 16.4, tvOS 16.4, *)) {
+#if __MAC_OS_X_VERSION_MAX_ALLOWED >= 130300 || __IPHONE_OS_VERSION_MAX_ALLOWED >= 160400 || \
+    __TV_OS_VERSION_MAX_ALLOWED >= 160400


Let's not add tvOS checks, since we don't support tvOS, or have any plans to.

stuartmorgan-g · 2023-06-29T12:30:37Z

...s/webview_flutter/webview_flutter_wkwebview/example/ios/RunnerTests/FWFWebViewHostApiTests.m

@@ -465,6 +465,8 @@ - (void)testContentInsetsSumAlwaysZeroAfterSetFrame {
  XCTAssertTrue(CGRectEqualToRect(webView.frame, CGRectMake(0, 0, 300, 100)));
 }

+#if __MAC_OS_X_VERSION_MAX_ALLOWED >= 130300 || __IPHONE_OS_VERSION_MAX_ALLOWED >= 160400 || \
+    __TV_OS_VERSION_MAX_ALLOWED >= 160400


We shouldn't need this check on tests, since most clients aren't running our native tests, and it's fine for us to require a new Xcode for development.

bparrishMines · 2023-06-29T17:19:35Z

@cyanglaz @stuartmorgan Have you seen this error before?

/Volumes/Work/s/w/ir/x/w/packages/packages/webview_flutter/webview_flutter_wkwebview/example/ios/Pods/Pods.xcodeproj: warning: The iOS Simulator deployment target 'IPHONEOS_DEPLOYMENT_TARGET' is set to 9.0, but the range of supported deployment target versions is 11.0 to 16.4.99. (in target 'OCMock' from project 'Pods')

Test session results, code coverage, and logs:
	/Users/chrome-bot/Library/Developer/Xcode/DerivedData/Runner-gobtbkgsitlgxgbndnnohkiqixxx/Logs/Test/Test-Runner-2023.06.28_13-50-12--0700.xcresult

2023-06-28 13:50:12.712 xcodebuild[80036:429328] DVTCoreDeviceEnabledState: DVTCoreDeviceEnabledState_Disabled set via user default (DVTEnableCoreDevice=disabled)
--- xcodebuild: WARNING: Using the first of multiple matching destinations:
{ platform:iOS Simulator, id:FE4856FB-F70D-4B05-8DEA-BA4592F486A7, OS:16.4, name:iPhone 14 }
{ platform:iOS Simulator, id:FE4856FB-F70D-4B05-8DEA-BA4592F486A7, OS:16.4, name:iPhone 14 }
Testing failed:
	Null passed to a callee that requires a non-null argument
	Testing cancelled because the build failed.

** TEST FAILED **

Everything runs and builds locally. Is the warning the source of the build failure?

stuartmorgan-g · 2023-06-29T17:21:12Z

Testing failed:
Null passed to a callee that requires a non-null argument
Testing cancelled because the build failed.

This warning is the cause of the build failure; if you scroll up further you can find the original in context.

You're passing nil in a non-nullable callback; you need to pass a no-op block instead.

stuartmorgan-g · 2023-07-01T12:33:41Z

packages/webview_flutter/webview_flutter_wkwebview/ios/Classes/FWFWebViewHostApi.m

@@ -201,7 +201,9 @@ - (void)setInspectableForWebViewWithIdentifier:(NSNumber *)identifier
                                   inspectable:(NSNumber *)inspectable
                                         error:(FlutterError *_Nullable *_Nonnull)error {
  if (@available(macOS 13.3, iOS 16.4, tvOS 16.4, *)) {
+#if __MAC_OS_X_VERSION_MAX_ALLOWED >= 130300 || __IPHONE_OS_VERSION_MAX_ALLOWED >= 160400


Hm, looking at this again: do @available checks evaluate to false automatically when the compile SDK is lower than the available version? I'm not sure they do, and if they don't then this would, if compiled with an SDK < 16.4 but run on 16.4, silently no-op.

I guess that's not that big a deal for this particular case, but we should investigate before using this pattern again.

I'm not sure they do, and if they don't then this would, if compiled with an SDK < 16.4, but run on 16.4, silently no-op.

Yea, I think you're correct. This could probably use an additional #ifelse that also returns a FlutterError. I didn't quite understand this until after I autosubmitted.

stuartmorgan-g

LGTM

jpeace-pro · 2023-07-03T17:42:23Z

Hi there, could anyone help provide an update on when this will be released?

…ror and crash from equal `NSURL`s (flutter/packages#4340)

flutter/packages@cdae854...a03b900 2023-07-03 [email protected] [webview_flutter_wkwebview] Fixes inspectable compile-time error and crash from equal `NSURL`s (flutter/packages#4340) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

add some fixes

6f59765

bparrishMines requested a review from cyanglaz as a code owner June 28, 2023 19:54

github-actions bot added p: webview_flutter platform-ios labels Jun 28, 2023

bparrishMines added 2 commits June 28, 2023 16:19

fix versions

f8a8879

fix changelog

ad56919

bparrishMines changed the title ~~add some fixes~~ [webview_flutter_wkwebview] Fixes inspectable compile-time error and crash from equal NSURLs Jun 28, 2023

bparrishMines added 2 commits June 28, 2023 16:41

maybe version bump will fix?

2b003b3

set min ios version to 11

e985799

cyanglaz approved these changes Jun 28, 2023

View reviewed changes

stuartmorgan-g reviewed Jun 29, 2023

View reviewed changes

bparrishMines added 2 commits June 29, 2023 13:10

dont check tvos and dont guard test

21bbe26

Merge branch 'main' of github.com:flutter/packages into fix_wkwebview

173d8a9

fix error

2a91b99

bparrishMines requested a review from stuartmorgan-g June 30, 2023 14:51

stuartmorgan-g reviewed Jul 1, 2023

View reviewed changes

stuartmorgan-g approved these changes Jul 1, 2023

View reviewed changes

Merge branch 'main' of github.com:flutter/packages into fix_wkwebview

561029a

bparrishMines added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 3, 2023

auto-submit bot merged commit a03b900 into flutter:main Jul 3, 2023

bparrishMines deleted the fix_wkwebview branch July 3, 2023 23:52

engine-flutter-autoroll mentioned this pull request Jul 4, 2023

Roll Packages from cdae854a59b9 to a03b900efc0e (1 revision) flutter/flutter#129957

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 4, 2023

a03b900 [webview_flutter_wkwebview] Fixes inspectable compile-time er…

3f2d110

…ror and crash from equal `NSURL`s (flutter/packages#4340)

This was referenced Jul 7, 2023

[Tool] New tool to download android dependencies #4408

Merged

[camera] tree fix format 2023 05 12 #4450

Closed

reidbaker mentioned this pull request Jul 14, 2023

[tooling] mark infra steps as infra steps in yaml files #4473

Merged

11 tasks

reidbaker mentioned this pull request Jul 26, 2023

[Tooling] Add google owned cache for dependencies as an option in ci #4567

Merged

11 tasks

reidbaker mentioned this pull request Aug 4, 2023

[All] Expand artifact hub to all plugins #4645

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[webview_flutter_wkwebview] Fixes inspectable compile-time error and crash from equal `NSURL`s #4340

[webview_flutter_wkwebview] Fixes inspectable compile-time error and crash from equal `NSURL`s #4340

Uh oh!

bparrishMines commented Jun 28, 2023 •

edited

Loading

Uh oh!

cyanglaz left a comment

Uh oh!

stuartmorgan-g Jun 29, 2023

Uh oh!

stuartmorgan-g Jun 29, 2023

Uh oh!

bparrishMines commented Jun 29, 2023

Uh oh!

stuartmorgan-g commented Jun 29, 2023

Uh oh!

stuartmorgan-g Jul 1, 2023 •

edited

Loading

Uh oh!

bparrishMines Jul 3, 2023

Uh oh!

stuartmorgan-g left a comment

Uh oh!

jpeace-pro commented Jul 3, 2023

Uh oh!

Uh oh!

[webview_flutter_wkwebview] Fixes inspectable compile-time error and crash from equal NSURLs #4340

[webview_flutter_wkwebview] Fixes inspectable compile-time error and crash from equal NSURLs #4340

Uh oh!

Conversation

bparrishMines commented Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

cyanglaz left a comment

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Jun 29, 2023

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Jun 29, 2023

Choose a reason for hiding this comment

Uh oh!

bparrishMines commented Jun 29, 2023

Uh oh!

stuartmorgan-g commented Jun 29, 2023

Uh oh!

stuartmorgan-g Jul 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bparrishMines Jul 3, 2023

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

jpeace-pro commented Jul 3, 2023

Uh oh!

Uh oh!

[webview_flutter_wkwebview] Fixes inspectable compile-time error and crash from equal `NSURL`s #4340

[webview_flutter_wkwebview] Fixes inspectable compile-time error and crash from equal `NSURL`s #4340

bparrishMines commented Jun 28, 2023 •

edited

Loading

stuartmorgan-g Jul 1, 2023 •

edited

Loading