Skip to content
This repository has been 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 1 commit
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
57 changes: 48 additions & 9 deletions packages/url_launcher/url_launcher_web/example/lib/main.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,63 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'package:flutter/gestures.dart';
import 'package:flutter/material.dart';
import 'package:url_launcher/link.dart';

void main() {
runApp(MyApp());
}

class _HorizontalScrollBehavior extends MaterialScrollBehavior {
@override
Set<PointerDeviceKind> get dragDevices => <PointerDeviceKind>{
PointerDeviceKind.touch,
PointerDeviceKind.stylus,
PointerDeviceKind.invertedStylus,
PointerDeviceKind.mouse,
};
}

/// App for testing
class MyApp extends StatefulWidget {
class MyApp extends StatelessWidget {
@override
_MyAppState createState() => _MyAppState();
Widget build(BuildContext context) => MaterialApp(
home: Scaffold(
body: ConstrainedBox(
constraints: const BoxConstraints(maxHeight: 300),
child: const _List(),
),
),
);
}

class _MyAppState extends State<MyApp> {
class _List extends StatelessWidget {
const _List({
Key? key,
}) : super(key: key);

@override
Widget build(BuildContext context) {
return const Directionality(
textDirection: TextDirection.ltr,
child: Text('Testing... Look at the console output for results!'),
);
}
Widget build(BuildContext context) => ScrollConfiguration(
behavior: _HorizontalScrollBehavior(),
child: ListView.separated(
scrollDirection: Axis.horizontal,
itemCount: 500,
separatorBuilder: (_, __) => const SizedBox(height: 24, width: 24),
itemBuilder: (_, int index) => Link(
uri: Uri.parse('https://example.com/$index'),
builder: (_, __) => GestureDetector(
onTap: () {
ScaffoldMessenger.of(context).showSnackBar(
SnackBar(content: Text('https://example.com/$index')));
},
child: Card(
child: Padding(
padding: const EdgeInsets.all(64),
child: Text('#$index', textAlign: TextAlign.center),
)),
),
),
),
);
}
5 changes: 5 additions & 0 deletions packages/url_launcher/url_launcher_web/example/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ environment:
dependencies:
flutter:
sdk: flutter
url_launcher:
path: ../../url_launcher/
dependency_overrides:
url_launcher_web:
path: ../

dev_dependencies:
build_runner: ^2.1.1
Expand Down
42 changes: 34 additions & 8 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 @@ -114,13 +114,24 @@ class LinkViewController extends PlatformViewController {
factory LinkViewController.fromParams(
PlatformViewCreationParams params,
BuildContext context,
) {
final int viewId = params.id;
final LinkViewController controller = LinkViewController(viewId, context);
controller._initialize().then((_) {
) =>
LinkViewController(params.id, context).._asyncInit(params);

Future<void> _asyncInit(PlatformViewCreationParams params) async {
try {
await _initialize();
if (context == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest keeping context final and adding a new field _isDisposed:

Suggested change
if (context == null) {
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 @@ -160,7 +171,7 @@ class LinkViewController extends PlatformViewController {
final int viewId;

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

late html.Element _element;

Expand Down Expand Up @@ -263,14 +274,29 @@ class LinkViewController extends PlatformViewController {
}

@override
Future<void> dispose() async {
Future<void> dispose() {
context = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
context = null;
_isDisposed = true;

Copy link
Author

@kristoffer-zliide kristoffer-zliide May 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I kind of like the context being nullable (despite the $billion mistake), since it makes people aware that it may not be safe to access it.

I went over it again and realized the context wasn't actually used anywhere, so I removed it. I also realized that the element need not be late initialized, so there'd be no need for _isInitialized and the asserts, and so with nothing nullable, I added an _isDisposed as you suggest 😄

if (_isInitialized) {
assert(_instances[viewId] == this);
_instances.remove(viewId);
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'),
));
}
}
}
Expand Down