[url_launcher] Fixed call to onPlatformViewCreated after dispose #5431

kristoffer-zliide · 2022-04-28T13:03:42Z

Don't call onPlatformViewCreated after dispose, since it calls setState().

Pre-launch Checklist

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

which caused an error, since it calls setState().

flutter-dashboard · 2022-04-28T13:03:44Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

stuartmorgan · 2022-05-12T20:29:07Z

@ditman Ping on this review?

ditman · 2022-05-12T22:11:25Z

I'm about to get to it; thanks for the ping! (@mdebbar is also back, he might want to take a look as well!)

mdebbar

The fix looks good, thanks for the contribution!

I only have a small suggestion and then the PR should be good to go (if @ditman also thinks so).

mdebbar · 2022-05-14T13:54:39Z

packages/url_launcher/url_launcher_web/lib/src/link.dart

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


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

Suggested change

if (context == null) {

if (_isDisposed) {

mdebbar · 2022-05-14T13:55:10Z

packages/url_launcher/url_launcher_web/lib/src/link.dart

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

  @override
-  Future<void> dispose() async {
+  Future<void> dispose() {
+    context = null;


Suggested change

context = null;

_isDisposed = true;

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 😄

stuartmorgan · 2022-05-14T16:08:47Z

I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.

I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.

...

I added new tests to check the change I am making, or this PR is test-exempt.

These three steps are all required before the PR can land

kristoffer-zliide · 2022-05-15T10:37:39Z

@stuartmorgan: didn't want to bump the version prematurely. I've bumped it from 2.0.11 to 2.0.12, hope that's fine with you.

Worse, I have no idea as to how to test it, except for the updated example app. And I don't see test-exempt covering this case 😄

kristoffer-zliide · 2022-05-15T10:56:31Z

BTW, I was struggling a bit with dependencies. It seems to me that the tests in the example app isn't actually testing the code in the repo, rather it's testing the code on pub.dev. Or am I missing something? Not used to this mono-repo thing... The best solution I could think of would be dependency overrides in the example project, but that gets rejected by one of the github checks.

stuartmorgan · 2022-05-16T14:18:35Z

Worse, I have no idea as to how to test it, except for the updated example app.

The example app is not itself an automated test. @mdebbar or @ditman may be able to advise on how to write a widget test for this scenario.

It seems to me that the tests in the example app isn't actually testing the code in the repo, rather it's testing the code on pub.dev.

I'm not sure how you came to that conclusion; can you elaborate on why you think that?

Not used to this mono-repo thing

Your changes are all to a single package, so it's not clear to me what the relevance of the monorepo is here. What is the issue?

kristoffer-zliide · 2022-05-16T14:31:43Z

Not concluding; it just seems to me that way 😄 The tests live in the example package which references url_launcer from ../../url_launcer which references url_launcer_web from url_launcher_web: ^2.0.0, i.e. from pub.dev. If I try to change that (either with path dependencies or dependency overrides), all kinds of things break outside the url_launcher_web package. But maybe there's some bigger mono-repo related things at play, so things are not what they seem to me.

stuartmorgan · 2022-05-16T14:43:16Z

The tests live in the example package which references url_launcer from ../../url_launcer

url_launcher_web/example doesn't reference url_launcher at all.

kristoffer-zliide · 2022-05-16T15:30:27Z

That was me. I wanted to use the Link widget in the example app. I've reverted the changes to example app, so now it is again an example of nothing and everything is fine.

kristoffer-zliide · 2022-05-16T16:11:55Z

I managed to make a test out of the scraps of the example app, that was failing without the changes to the Link.

…k widgets.

stuartmorgan · 2022-05-16T16:16:46Z

[...] so now it is again an example of nothing and everything is fine.
[...] out of the scraps of the example app

I'm not sure exactly what these comments mean, but you are welcome to add anything from within the package itself (i.e., that doesn't use things from a higher layer in the stack) to make the example useful for testing purposes, since that's what implementation package examples are for.

ditman

This looks good. I saw the issue happening with the test (run manually) but couldn't get it to fail either. After the fix, the exception seems to not be thrown.

I have a small question about the unawaited _asyncInit, and some other details, but in general this seems good!

packages/url_launcher/url_launcher_web/lib/src/link.dart

packages/url_launcher/url_launcher_web/CHANGELOG.md

ditman

(Another quick comment)

ditman · 2022-06-02T23:13:28Z

packages/url_launcher/url_launcher_web/lib/src/link.dart

+        'viewType': linkViewType,
+      });
+      if (_isDisposed) {
+        return;


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

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.

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)

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?

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".

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

ditman · 2022-06-14T01:31:08Z

@kristoffer-zliide, I've simplified your PR a little bit here:

[url_launcher] Fixes call to setState after dispose. #5963

I used your same test, and I think everything is working as expected. I don't know if you're testing this locally on your project, but you can use my branch from github to see if it solves your actual issue!

Thanks for helping me understand this bug!

[url-launcher] Fixed call to onPlatformViewCreated after dispose

8fb082e

which caused an error, since it calls setState().

kristoffer-zliide requested a review from ditman as a code owner April 28, 2022 13:03

flutter-dashboard bot added the needs tests label Apr 28, 2022

github-actions bot added p: url_launcher platform-web labels Apr 28, 2022

kristoffer-zliide mentioned this pull request Apr 28, 2022

[url_launcher]Error: setState() called after dispose(): _PlatformViewLinkState#946fb(lifecycle state: defunct, not mounted) flutter/flutter#102741

Closed

ditman changed the title ~~[url-launcher] Fixed call to onPlatformViewCreated after dispose~~ [url_launcher] Fixed call to onPlatformViewCreated after dispose May 12, 2022

mdebbar reviewed May 14, 2022

View reviewed changes

Merge branch 'main' into fix-setstate-after-dispose

a640546

kristoffer-zliide force-pushed the fix-setstate-after-dispose branch from 92b7f7b to a640546 Compare May 15, 2022 08:54

kristoffer-zliide added 2 commits May 15, 2022 12:00

[url_launcher_web] Simplified example app.

3b09ff5

[url_launcher_web] Simplified LinkViewController state.

0354b1d

kristoffer-zliide force-pushed the fix-setstate-after-dispose branch 2 times, most recently from 1ce7cf3 to b4359de Compare May 15, 2022 10:22

[url_launcher_web] Version bump.

c49ce9b

kristoffer-zliide force-pushed the fix-setstate-after-dispose branch from b4359de to c49ce9b Compare May 15, 2022 10:33

kristoffer-zliide added 2 commits May 16, 2022 18:14

[url_launcher_web] Revert changes to example app.

00f0f2a

[url_launcher_web] Added stress test of creation and disposing of Lin…

7b5e35a

…k widgets.

kristoffer-zliide force-pushed the fix-setstate-after-dispose branch from 85c83d1 to 7b5e35a Compare May 16, 2022 16:14

ditman reviewed Jun 3, 2022

View reviewed changes

packages/url_launcher/url_launcher_web/lib/src/link.dart Show resolved Hide resolved

packages/url_launcher/url_launcher_web/lib/src/link.dart Show resolved Hide resolved

packages/url_launcher/url_launcher_web/CHANGELOG.md Show resolved Hide resolved

ditman reviewed Jun 3, 2022

View reviewed changes

ditman mentioned this pull request Jun 14, 2022

[url_launcher] Fixes call to setState after dispose. #5963

Merged

11 tasks

fluttergithubbot closed this in #5963 Jun 14, 2022

ditman mentioned this pull request Jun 17, 2022

Github not closing linked issues of PRs merged by new autosubmit bot (?) flutter/flutter#106211

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[url_launcher] Fixed call to onPlatformViewCreated after dispose #5431

[url_launcher] Fixed call to onPlatformViewCreated after dispose #5431

kristoffer-zliide commented Apr 28, 2022 •

edited

Loading

flutter-dashboard bot commented Apr 28, 2022

stuartmorgan commented May 12, 2022

ditman commented May 12, 2022

mdebbar left a comment

mdebbar May 14, 2022

mdebbar May 14, 2022

kristoffer-zliide May 15, 2022 •

edited

Loading

stuartmorgan commented May 14, 2022

kristoffer-zliide commented May 15, 2022

kristoffer-zliide commented May 15, 2022

stuartmorgan commented May 16, 2022

kristoffer-zliide commented May 16, 2022

stuartmorgan commented May 16, 2022

kristoffer-zliide commented May 16, 2022

kristoffer-zliide commented May 16, 2022

stuartmorgan commented May 16, 2022

ditman left a comment

ditman left a comment

ditman Jun 2, 2022

kristoffer-zliide Jun 11, 2022

ditman Jun 13, 2022

kristoffer-zliide Jun 13, 2022

ditman Jun 13, 2022

ditman Jun 13, 2022

ditman commented Jun 14, 2022

[url_launcher] Fixed call to onPlatformViewCreated after dispose #5431

[url_launcher] Fixed call to onPlatformViewCreated after dispose #5431

Conversation

kristoffer-zliide commented Apr 28, 2022 • edited Loading

Pre-launch Checklist

flutter-dashboard bot commented Apr 28, 2022

stuartmorgan commented May 12, 2022

ditman commented May 12, 2022

mdebbar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristoffer-zliide May 15, 2022 • edited Loading

Choose a reason for hiding this comment

stuartmorgan commented May 14, 2022

kristoffer-zliide commented May 15, 2022

kristoffer-zliide commented May 15, 2022

stuartmorgan commented May 16, 2022

kristoffer-zliide commented May 16, 2022

stuartmorgan commented May 16, 2022

kristoffer-zliide commented May 16, 2022

kristoffer-zliide commented May 16, 2022

stuartmorgan commented May 16, 2022

ditman left a comment

Choose a reason for hiding this comment

ditman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ditman commented Jun 14, 2022

kristoffer-zliide commented Apr 28, 2022 •

edited

Loading

kristoffer-zliide May 15, 2022 •

edited

Loading