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

[url_launcher] Fixed call to onPlatformViewCreated after dispose #5431

Closed
wants to merge 7 commits into from
Closed

[url_launcher] Fixed call to onPlatformViewCreated after dispose #5431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8fb082e
[url-launcher] Fixed call to onPlatformViewCreated after dispose
kristoffer-zliide Apr 28, 2022
a640546
Merge branch 'main' into fix-setstate-after-dispose
kristoffer-zliide May 15, 2022
3b09ff5
[url_launcher_web] Simplified example app.
kristoffer-zliide May 15, 2022
0354b1d
[url_launcher_web] Simplified LinkViewController state.
kristoffer-zliide May 15, 2022
c49ce9b
[url_launcher_web] Version bump.
kristoffer-zliide May 15, 2022
00f0f2a
[url_launcher_web] Revert changes to example app.
kristoffer-zliide May 16, 2022
7b5e35a
[url_launcher_web] Added stress test of creation and disposing of Lin…
kristoffer-zliide May 16, 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
4 changes: 4 additions & 0 deletions packages/url_launcher/url_launcher_web/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 2.0.12

* Fixed call to `setState` after dispose.

## 2.0.11

* Minor fixes for new analysis options.
Expand Down
30 changes: 30 additions & 0 deletions packages/url_launcher/url_launcher_web/example/integration_test/link_widget_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,36 @@ void main() {
final html.Element anchor = _findSingleAnchor();
expect(anchor.hasAttribute('href'), false);
});

testWidgets('can be created and disposed', (WidgetTester tester) async {
final Uri uri = Uri.parse('http://foobar');
const int itemCount = 500;
await tester.pumpWidget(
Directionality(
textDirection: TextDirection.ltr,
child: MediaQuery(
data: const MediaQueryData(),
child: ListView.builder(
itemCount: itemCount,
itemBuilder: (_, int index) => WebLinkDelegate(TestLinkInfo(
uri: uri,
target: LinkTarget.defaultTarget,
builder: (BuildContext context, FollowLink? followLink) =>
Text('#$index', textAlign: TextAlign.center),
)),
),
),
),
);

await tester.pumpAndSettle();

await tester.scrollUntilVisible(
find.text('#${itemCount - 1}'),
2500,
maxScrolls: 1000,
);
});
});
}

Expand Down
99 changes: 59 additions & 40 deletions packages/url_launcher/url_launcher_web/lib/src/link.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ class WebLinkDelegateState extends State<WebLinkDelegate> {
child: PlatformViewLink(
viewType: linkViewType,
onCreatePlatformView: (PlatformViewCreationParams params) {
_controller = LinkViewController.fromParams(params, context);
_controller = LinkViewController.fromParams(params);
return _controller
..setUri(widget.link.uri)
..setTarget(widget.link.target);
Expand All @@ -100,7 +100,7 @@ class WebLinkDelegateState extends State<WebLinkDelegate> {
/// Controls link views.
class LinkViewController extends PlatformViewController {
/// Creates a [LinkViewController] instance with the unique [viewId].
LinkViewController(this.viewId, this.context) {
LinkViewController(this.viewId) : _element = _makeElement(viewId) {
if (_instances.isEmpty) {
// This is the first controller being created, attach the global click
// listener.
Expand All @@ -113,14 +113,28 @@ class LinkViewController extends PlatformViewController {
/// platform view [params].
factory LinkViewController.fromParams(
PlatformViewCreationParams params,
BuildContext context,
) {
final int viewId = params.id;
final LinkViewController controller = LinkViewController(viewId, context);
controller._initialize().then((_) {
) =>
LinkViewController(params.id).._asyncInitialize(params);

Future<void> _asyncInitialize(PlatformViewCreationParams params) async {
try {
await SystemChannels.platform_views
.invokeMethod<void>('create', <String, dynamic>{
'id': viewId,
'viewType': linkViewType,
});
if (_isDisposed) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

Can this disposed check be performed before actually asking the framework to create a platform_view?

Copy link
Author

Choose a reason for hiding this comment

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

I think the whole point of this fix is to check after the await, sort of like you do when do if (!mounted) return; after awaits in stateful widget methods. Besides, I can't see how _isDisposed can be true before the await.

Copy link
Member

Choose a reason for hiding this comment

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

I can't see how _isDisposed can be true before the await.

@kristoffer-zliide the lifecycle of Flutter Widgets and platform views is somewhat separate. In this code, you're adding elements to the DOM (code) and then checking if the Widget has been disposed to then not call onPlatformViewCreated.

I don't think you need to modify the DOM at all, or call "create" on a "platform_view" if the Widget that is being initialized has been previously marked as disposed. That's why I asked you to move the isDisposed check above the invokeMethod, to exit as early as possible.

In fact, if onPlatformViewCreated could be called multiple times earlier, this should be complaining about viewId being already created. (Is the reportError swallowing the exception on the web? Looking.)

(Note: this LinkViewController was originally modeled to look like the class _HtmlElementViewController, here, so everything used initialized instead of disposed)

Copy link
Author

Choose a reason for hiding this comment

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

I appreciate the desire to skip anything that would need to be awaited, but I don't quite follow anything else you're writing. With the refactoring done in this PR, I don't think I need to read more than 15 lines of code in the Link.dart file to see that the check wouldn't do anything before the await, since _asyncInitialize is only called on newly constructed LinkViewControllers which have _isDisposed set to false. Moving the check up also implies not doing it after, which was the whole point of the PR. But maybe I missed something in the refactoring?

Copy link
Member

Choose a reason for hiding this comment

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

I'm just trying to understand what's flutter doing here, because I don't think we should be getting an "init" on something that has been already "disposed".

Copy link
Member

Choose a reason for hiding this comment

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

The framework seems to be disposing of the link while the await invokeMethod 'create' is executing, so when it's time to call onPlatformViewCreated, viewId is already disposed.

Something like:

Disposed? false - before await create 428 - 918280561
!!! - dispose 428 - 918280561
Disposed? true - after await create 428 - 918280561
Exception! Kaboom 428 - 918280561

}
params.onPlatformViewCreated(viewId);
});
return controller;
} catch (exception, stack) {
FlutterError.reportError(FlutterErrorDetails(
exception: exception,
stack: stack,
library: 'url_launcher',
context: ErrorDescription('while initializing view $viewId'),
));
}
}

static final Map<int, LinkViewController> _instances =
Expand Down Expand Up @@ -159,33 +173,9 @@ class LinkViewController extends PlatformViewController {
@override
final int viewId;

/// The context of the [Link] widget that created this controller.
final BuildContext context;

late html.Element _element;

bool get _isInitialized => _element != null;
html.Element _element;

Future<void> _initialize() async {
_element = html.Element.tag('a');
setProperty(_element, linkViewIdProperty, viewId);
_element.style
..opacity = '0'
..display = 'block'
..width = '100%'
..height = '100%'
..cursor = 'unset';

// This is recommended on MDN:
// - https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#attr-target
_element.setAttribute('rel', 'noreferrer noopener');

final Map<String, dynamic> args = <String, dynamic>{
'id': viewId,
'viewType': linkViewType,
};
await SystemChannels.platform_views.invokeMethod<void>('create', args);
}
bool _isDisposed = false;

void _onDomClick(html.MouseEvent event) {
final bool isHitTested = _hitTestedViewId == viewId;
Expand All @@ -208,7 +198,7 @@ class LinkViewController extends PlatformViewController {
// browser handle it.
event.preventDefault();
final String routeName = _uri.toString();
pushRouteNameToFramework(context, routeName);
pushRouteNameToFramework(null, routeName);
}

Uri? _uri;
Expand All @@ -217,7 +207,6 @@ class LinkViewController extends PlatformViewController {
///
/// When Uri is null, the `href` attribute of the link is removed.
void setUri(Uri? uri) {
assert(_isInitialized);
_uri = uri;
if (uri == null) {
_element.removeAttribute('href');
Expand All @@ -234,7 +223,6 @@ class LinkViewController extends PlatformViewController {

/// Set the [LinkTarget] value for this link.
void setTarget(LinkTarget target) {
assert(_isInitialized);
_element.setAttribute('target', _getHtmlTarget(target));
}

Expand Down Expand Up @@ -263,18 +251,49 @@ class LinkViewController extends PlatformViewController {
}

@override
Future<void> dispose() async {
if (_isInitialized) {
Future<void> dispose() {
if (!_isDisposed) {
assert(_instances[viewId] == this);
_instances.remove(viewId);
_isDisposed = true;
return _asyncDispose();
}
return Future<void>.value();
}

Future<void> _asyncDispose() async {
try {
if (_instances.isEmpty) {
await _clickSubscription.cancel();
}
await SystemChannels.platform_views.invokeMethod<void>('dispose', viewId);
} catch (exception, stack) {
FlutterError.reportError(FlutterErrorDetails(
exception: exception,
stack: stack,
library: 'url_launcher',
context: ErrorDescription('while disposing view $viewId'),
));
}
}
}

html.Element _makeElement(int viewId) {
final html.Element element = html.Element.tag('a');
setProperty(element, linkViewIdProperty, viewId);
element.style
..opacity = '0'
..display = 'block'
..width = '100%'
..height = '100%'
..cursor = 'unset';

// This is recommended on MDN:
// - https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#attr-target
element.setAttribute('rel', 'noreferrer noopener');
return element;
}

/// Finds the view id of the DOM element targeted by the [event].
int? getViewIdFromTarget(html.Event event) {
final html.Element? linkElement = getLinkElementFromTarget(event);
Expand Down
2 changes: 1 addition & 1 deletion packages/url_launcher/url_launcher_web/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: url_launcher_web
description: Web platform implementation of url_launcher
repository: https://github.com/flutter/plugins/tree/main/packages/url_launcher/url_launcher_web
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+url_launcher%22
version: 2.0.11
version: 2.0.12

environment:
sdk: ">=2.12.0 <3.0.0"
Expand Down