Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[webview_flutter_wkwebview] Prevent leaking when a callback method references an object that references itself #6056

Merged
merged 17 commits into from
Jul 7, 2022
Merged

[webview_flutter_wkwebview] Prevent leaking when a callback method references an object that references itself #6056

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
63478a1
prevent leaks
bparrishMines Jun 27, 2022
1c8bd6c
change doc
bparrishMines Jun 28, 2022
a41be67
some experiments
bparrishMines Jun 29, 2022
308dee8
use weak reference callback
bparrishMines Jun 29, 2022
6731367
create a weakref helper method
bparrishMines Jun 29, 2022
96077cb
tests running but failing
bparrishMines Jun 29, 2022
f468f2c
some test progress
bparrishMines Jun 29, 2022
6596bd7
use weak reference helper
bparrishMines Jun 29, 2022
2af6812
docs and remove runGarbageCollection
bparrishMines Jun 30, 2022
1114c07
unused imports
bparrishMines Jun 30, 2022
c66e8ec
license
bparrishMines Jun 30, 2022
75cab29
remove async from test
bparrishMines Jun 30, 2022
2f9e913
add a test
bparrishMines Jun 30, 2022
7b9261f
include weakReferenceTo in doc
bparrishMines Jun 30, 2022
1610344
move test to integration test
bparrishMines Jun 30, 2022
5f51fbd
dont print
bparrishMines Jun 30, 2022
bba8625
organize imports
bparrishMines Jun 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions ...view_flutter/webview_flutter_wkwebview/example/integration_test/webview_flutter_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import 'package:flutter/services.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:integration_test/integration_test.dart';
import 'package:webview_flutter_platform_interface/webview_flutter_platform_interface.dart';
import 'package:webview_flutter_wkwebview/src/common/instance_manager.dart';
import 'package:webview_flutter_wkwebview/src/common/weak_reference_utils.dart';
import 'package:webview_flutter_wkwebview_example/navigation_decision.dart';
import 'package:webview_flutter_wkwebview_example/navigation_request.dart';
import 'package:webview_flutter_wkwebview_example/web_view.dart';
Expand Down Expand Up @@ -66,6 +68,27 @@ Future<void> main() async {
expect(currentUrl, primaryUrl);
});

testWidgets(
'withWeakRefenceTo allows encapsulating class to be garbage collected',
(WidgetTester tester) async {
final Completer<int> gcCompleter = Completer<int>();
final InstanceManager instanceManager = InstanceManager(
onWeakReferenceRemoved: gcCompleter.complete,
);

ClassWithCallbackClass? instance = ClassWithCallbackClass();
instanceManager.addHostCreatedInstance(instance.callbackClass, 0);
instance = null;

// Force garbage collection.
await IntegrationTestWidgetsFlutterBinding.instance
.watchPerformance(() async {
await tester.pumpAndSettle();
});

expect(gcCompleter.future, completion(0));
}, timeout: const Timeout(Duration(seconds: 10)));

testWidgets('loadUrl', (WidgetTester tester) async {
final Completer<WebViewController> controllerCompleter =
Completer<WebViewController>();
Expand Down Expand Up @@ -1253,3 +1276,33 @@ class ResizableWebViewState extends State<ResizableWebView> {
);
}
}

class CopyableObjectWithCallback with Copyable {
CopyableObjectWithCallback(this.callback);

final VoidCallback callback;

@override
CopyableObjectWithCallback copy() {
return CopyableObjectWithCallback(callback);
}
}

class ClassWithCallbackClass {
ClassWithCallbackClass() {
callbackClass = CopyableObjectWithCallback(
withWeakRefenceTo(
this,
(WeakReference<ClassWithCallbackClass> weakReference) {
return () {
// Weak reference to `this` in callback.
// ignore: unnecessary_statements
weakReference;
};
},
),
);
}

late final CopyableObjectWithCallback callbackClass;
}
34 changes: 34 additions & 0 deletions packages/webview_flutter/webview_flutter_wkwebview/lib/src/common/weak_reference_utils.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

/// Helper method for creating callbacks methods with a weak reference.
///
/// Example:
/// ```
/// final JavascriptChannelRegistry javascriptChannelRegistry = ...
///
/// final WKScriptMessageHandler handler = WKScriptMessageHandler(
/// didReceiveScriptMessage: withWeakRefenceTo(
/// javascriptChannelRegistry,
/// (WeakReference<JavascriptChannelRegistry> weakReference) {
/// return (
/// WKUserContentController userContentController,
/// WKScriptMessage message,
/// ) {
/// weakReference.target?.onJavascriptChannelMessage(
/// message.name,
/// message.body!.toString(),
/// );
/// };
/// },
/// ),
/// );
/// ```
S withWeakRefenceTo<T extends Object, S extends Object>(
T reference,
S Function(WeakReference<T> weakReference) onCreate,
) {
final WeakReference<T> weakReference = WeakReference<T>(reference);
return onCreate(weakReference);
}
15 changes: 14 additions & 1 deletion packages/webview_flutter/webview_flutter_wkwebview/lib/src/foundation/foundation.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import 'dart:typed_data';

import 'package:flutter/foundation.dart';
import 'package:flutter/services.dart';
import 'package:webview_flutter_wkwebview/src/common/weak_reference_utils.dart';

import '../common/instance_manager.dart';
import 'foundation_api_impls.dart';
Expand Down Expand Up @@ -267,7 +268,19 @@ class NSObject with Copyable {

final NSObjectHostApiImpl _api;

/// Informs the observing object when the value at the specified key path has changed.
/// Informs the observing object when the value at the specified key path has
/// changed.
///
/// {@template webview_flutter_wkwebview.foundation.callbacks}
/// For the associated Objective-C object to be automatically garbage
/// collected, it is required that this Function doesn't contain a strong
/// reference to the encapsulating class instance. Consider using
/// `WeakReference` when referencing an object not received as a parameter.
/// Otherwise, use [NSObject.dispose] to release the associated Objective-C
/// object manually.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how the second recommendation makes sense in this case; KVO isn't a one-time deterministic callback, so disposing would sometimes never happen, and sometimes happen multiple times. In this context, only a weak reference seems correct. It seems like that would be true of many of the cases this template is being applied to.

Am I misunderstanding?

Copy link
Contributor Author

@bparrishMines bparrishMines Jun 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This explanation was for callbacks in general and not just KVO. I could write a separate one for KVO?

The use of NSObject.dispose was for the case that someone would want to guarantee a reference to an object until a specific callback was made. For example,

MyClass? myClass;

void aMethod() {
  myClass = MyClass(callbackWithImportantInfo: (String info) {
    if (myClass != null) {
      anotherMethod();
      NSObject.dispose(myClass);
    }
    myClass = null;
  });
}

void anotherMethod() {

}

void aMethodCalledSometimeLater() {
  myOtherClass.setMyClass(myClass);
}

I think this could still apply to KVO. I agree that this use case would probably be rare though.

///
/// See [withWeakRefenceTo].
/// {@endtemplate}
final void Function(
String keyPath,
NSObject object,
Expand Down
16 changes: 16 additions & 0 deletions packages/webview_flutter/webview_flutter_wkwebview/lib/src/web_kit/web_kit.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,8 @@ class WKScriptMessageHandler extends NSObject {
/// Use this method to respond to a message sent from the webpage’s
/// JavaScript code. Use the [message] parameter to get the message contents and
/// to determine the originating web view.
///
/// {@macro webview_flutter_wkwebview.foundation.callbacks}
final void Function(
WKUserContentController userContentController,
WKScriptMessage message,
Expand Down Expand Up @@ -733,6 +735,8 @@ class WKUIDelegate extends NSObject {
final WKUIDelegateHostApiImpl _uiDelegateApi;

/// Indicates a new [WKWebView] was requested to be created with [configuration].
///
/// {@macro webview_flutter_wkwebview.foundation.callbacks}
final void Function(
WKWebView webView,
WKWebViewConfiguration configuration,
Expand Down Expand Up @@ -803,26 +807,38 @@ class WKNavigationDelegate extends NSObject {
final WKNavigationDelegateHostApiImpl _navigationDelegateApi;

/// Called when navigation is complete.
///
/// {@macro webview_flutter_wkwebview.foundation.callbacks}
final void Function(WKWebView webView, String? url)? didFinishNavigation;

/// Called when navigation from the main frame has started.
///
/// {@macro webview_flutter_wkwebview.foundation.callbacks}
final void Function(WKWebView webView, String? url)?
didStartProvisionalNavigation;

/// Called when permission is needed to navigate to new content.
///
/// {@macro webview_flutter_wkwebview.foundation.callbacks}
final Future<WKNavigationActionPolicy> Function(
WKWebView webView,
WKNavigationAction navigationAction,
)? decidePolicyForNavigationAction;

/// Called when an error occurred during navigation.
///
/// {@macro webview_flutter_wkwebview.foundation.callbacks}
final void Function(WKWebView webView, NSError error)? didFailNavigation;

/// Called when an error occurred during the early navigation process.
///
/// {@macro webview_flutter_wkwebview.foundation.callbacks}
final void Function(WKWebView webView, NSError error)?
didFailProvisionalNavigation;

/// Called when the web view’s content process was terminated.
///
/// {@macro webview_flutter_wkwebview.foundation.callbacks}
final void Function(WKWebView webView)? webViewWebContentProcessDidTerminate;

@override
Expand Down
167 changes: 98 additions & 69 deletions packages/webview_flutter/webview_flutter_wkwebview/lib/src/web_kit_webview_widget.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import 'package:flutter/services.dart';
import 'package:path/path.dart' as path;
import 'package:webview_flutter_platform_interface/webview_flutter_platform_interface.dart';

import 'common/weak_reference_utils.dart';
import 'foundation/foundation.dart';
import 'web_kit/web_kit.dart';

Expand Down Expand Up @@ -114,58 +115,74 @@ class WebKitWebViewPlatformController extends WebViewPlatformController {

/// Used to integrate custom user interface elements into web view interactions.
@visibleForTesting
late final WKUIDelegate uiDelegate =
webViewProxy.createUIDelgate(onCreateWebView: (
WKWebView webView,
WKWebViewConfiguration configuration,
WKNavigationAction navigationAction,
) {
if (!navigationAction.targetFrame.isMainFrame) {
webView.loadRequest(navigationAction.request);
}
});
late final WKUIDelegate uiDelegate = webViewProxy.createUIDelgate(
onCreateWebView: (
WKWebView webView,
WKWebViewConfiguration configuration,
WKNavigationAction navigationAction,
) {
if (!navigationAction.targetFrame.isMainFrame) {
webView.loadRequest(navigationAction.request);
}
},
);

/// Methods for handling navigation changes and tracking navigation requests.
@visibleForTesting
late final WKNavigationDelegate navigationDelegate =
webViewProxy.createNavigationDelegate(
didFinishNavigation: (WKWebView webView, String? url) {
callbacksHandler.onPageFinished(url ?? '');
},
didStartProvisionalNavigation: (WKWebView webView, String? url) {
callbacksHandler.onPageStarted(url ?? '');
},
decidePolicyForNavigationAction: (
WKWebView webView,
WKNavigationAction action,
) async {
if (!_hasNavigationDelegate) {
return WKNavigationActionPolicy.allow;
}
late final WKNavigationDelegate navigationDelegate = withWeakRefenceTo(
this,
(WeakReference<WebKitWebViewPlatformController> weakReference) {
return webViewProxy.createNavigationDelegate(
didFinishNavigation: (WKWebView webView, String? url) {
weakReference.target?.callbacksHandler.onPageFinished(url ?? '');
},
didStartProvisionalNavigation: (WKWebView webView, String? url) {
weakReference.target?.callbacksHandler.onPageStarted(url ?? '');
},
decidePolicyForNavigationAction: (
WKWebView webView,
WKNavigationAction action,
) async {
if (weakReference.target == null) {
return WKNavigationActionPolicy.allow;
}

if (!weakReference.target!._hasNavigationDelegate) {
return WKNavigationActionPolicy.allow;
}

final bool allow =
await weakReference.target!.callbacksHandler.onNavigationRequest(
url: action.request.url,
isForMainFrame: action.targetFrame.isMainFrame,
);

final bool allow = await callbacksHandler.onNavigationRequest(
url: action.request.url,
isForMainFrame: action.targetFrame.isMainFrame,
return allow
? WKNavigationActionPolicy.allow
: WKNavigationActionPolicy.cancel;
},
didFailNavigation: (WKWebView webView, NSError error) {
weakReference.target?.callbacksHandler.onWebResourceError(
_toWebResourceError(error),
);
},
didFailProvisionalNavigation: (WKWebView webView, NSError error) {
weakReference.target?.callbacksHandler.onWebResourceError(
_toWebResourceError(error),
);
},
webViewWebContentProcessDidTerminate: (WKWebView webView) {
weakReference.target?.callbacksHandler.onWebResourceError(
WebResourceError(
errorCode: WKErrorCode.webContentProcessTerminated,
// Value from https://developer.apple.com/documentation/webkit/wkerrordomain?language=objc.
domain: 'WKErrorDomain',
description: '',
errorType: WebResourceErrorType.webContentProcessTerminated,
),
);
},
);

return allow
? WKNavigationActionPolicy.allow
: WKNavigationActionPolicy.cancel;
},
didFailNavigation: (WKWebView webView, NSError error) {
callbacksHandler.onWebResourceError(_toWebResourceError(error));
},
didFailProvisionalNavigation: (WKWebView webView, NSError error) {
callbacksHandler.onWebResourceError(_toWebResourceError(error));
},
webViewWebContentProcessDidTerminate: (WKWebView webView) {
callbacksHandler.onWebResourceError(WebResourceError(
errorCode: WKErrorCode.webContentProcessTerminated,
// Value from https://developer.apple.com/documentation/webkit/wkerrordomain?language=objc.
domain: 'WKErrorDomain',
description: '',
errorType: WebResourceErrorType.webContentProcessTerminated,
));
},
);

Expand All @@ -179,14 +196,23 @@ class WebKitWebViewPlatformController extends WebViewPlatformController {
autoMediaPlaybackPolicy: params.autoMediaPlaybackPolicy,
);

webView = webViewProxy.createWebView(configuration, observeValue: (
String keyPath,
NSObject object,
Map<NSKeyValueChangeKey, Object?> change,
) {
final double progress = change[NSKeyValueChangeKey.newValue]! as double;
callbacksHandler.onProgress((progress * 100).round());
});
webView = webViewProxy.createWebView(
configuration,
observeValue: withWeakRefenceTo(
callbacksHandler,
(WeakReference<WebViewPlatformCallbacksHandler> weakReference) {
return (
String keyPath,
NSObject object,
Map<NSKeyValueChangeKey, Object?> change,
) {
final double progress =
change[NSKeyValueChangeKey.newValue]! as double;
weakReference.target?.onProgress((progress * 100).round());
};
},
),
);

webView.setUIDelegate(uiDelegate);

Expand Down Expand Up @@ -413,15 +439,22 @@ class WebKitWebViewPlatformController extends WebViewPlatformController {
).map<Future<void>>(
(String channelName) {
final WKScriptMessageHandler handler =
webViewProxy.createScriptMessageHandler(didReceiveScriptMessage: (
WKUserContentController userContentController,
WKScriptMessage message,
) {
javascriptChannelRegistry.onJavascriptChannelMessage(
message.name,
message.body!.toString(),
);
});
webViewProxy.createScriptMessageHandler(
didReceiveScriptMessage: withWeakRefenceTo(
javascriptChannelRegistry,
(WeakReference<JavascriptChannelRegistry> weakReference) {
return (
WKUserContentController userContentController,
WKScriptMessage message,
) {
weakReference.target?.onJavascriptChannelMessage(
message.name,
message.body!.toString(),
);
};
},
),
);
_scriptMessageHandlers[channelName] = handler;

final String wrapperSource =
Expand Down Expand Up @@ -652,11 +685,7 @@ class WebViewWidgetProxy {

/// Constructs a [WKNavigationDelegate].
WKNavigationDelegate createNavigationDelegate({
void Function(
WKWebView webView,
String? url,
)?
didFinishNavigation,
void Function(WKWebView webView, String? url)? didFinishNavigation,
void Function(WKWebView webView, String? url)?
didStartProvisionalNavigation,
Future<WKNavigationActionPolicy> Function(
Expand Down