Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash android dispose nullpointerdereference #203

Conversation

GaelleJoubert
Copy link
Contributor

Fix issue #182

GaelleJoubert and others added 3 commits February 23, 2023 15:29
Fix issue maplibre#182
I took the file from the fix from mapBox (this one), and add it at the same place in maplibre.

It seems to fix the issue ! At least I have not mannage to make it happen again.
@m0nac0
Copy link
Collaborator

m0nac0 commented Feb 25, 2023

Thank you for contributing this!
I assume this was copied from https://github.com/flutter-mapbox-gl/maps?

If so, I think we should fully port both flutter-mapbox-gl/maps#1172 and flutter-mapbox-gl/maps#1217 which introduced/improved this fix over there (and contain a few other changes as well).

@GaelleJoubert
Copy link
Contributor Author

Yes, as I mentionned in the issue #182 , I copied this file, from the MapBox library.
This file was added in flutter-mapbox-gl/maps#1217 .

I have not check the rest :)

@mariusvn
Copy link
Contributor

can you flutter format the code and try to fix the warnings ?

@GaelleJoubert
Copy link
Contributor Author

I try to format automatically the code, but it did not change the warning :/
Same with the static code analysis errors .. no idea how to fix them, on my side I have no errors ..
Any idea what to do ?

@m0nac0
Copy link
Collaborator

m0nac0 commented Mar 1, 2023

I haven't looked at in detail, but I assume this is because we need to port all changes from flutter-mapbox-gl/maps#1172 and flutter-mapbox-gl/maps#1217.

README.md Outdated
@@ -147,6 +147,11 @@ buildTypes {
}
```

## Flutter 3.x.x issues
Since Flutter 3.x.x was introduced, it exposed some race conditions resulting in occasional crashes upon map disposal. The parameter `useDelayedDisposal` was introduced as a workaround for this issue until Flutter and/or Mapbox fix this issue properly. Use with caution.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Since Flutter 3.x.x was introduced, it exposed some race conditions resulting in occasional crashes upon map disposal. The parameter `useDelayedDisposal` was introduced as a workaround for this issue until Flutter and/or Mapbox fix this issue properly. Use with caution.
Since Flutter 3.x.x was introduced, it exposed some race conditions resulting in occasional crashes upon map disposal. The parameter `useDelayedDisposal` was introduced as a workaround for this issue until Flutter and/or MapLibre fix this issue properly. Use with caution.

@GaelleJoubert
Copy link
Contributor Author

I am porting all the changes at the moment :)

@GaelleJoubert
Copy link
Contributor Author

Ok, I finnaly fixed all the warning, this is ready to merge.

@GaelleJoubert
Copy link
Contributor Author

I have tried it on my maps.
I still have the issue sometimes, but less often than before.
So It is not a complete fix, but it is still an improvement.
So I think it still worth it to merge it.
And once it will be merge with all the other recent fix, I'll test it again.

@GaelleJoubert
Copy link
Contributor Author

By the way, I found a nice workaround to avois the crash.
Since it happens at the Dispose() of the map, if you wrap the page where you have the MapLibre() widget in a KeepAlive, then dispose is never called when changing page, and the app does not crash.

Exemple of code :

class MapWidgetWithMarker extends StatefulWidget {
  MapWidgetWithMarker();
  @override
  _MapWidgetWithMarkerState createState() => _MapWidgetWithMarkerState();
}

class _MapWidgetWithMarkerState extends State<MapWidgetWithMarker> with AutomaticKeepAliveClientMixin  {

 @override
  Widget build(BuildContext context)   {

    return MaplibreMap( 
    //all your options
    );
  }

@override
  bool get wantKeepAlive => true;

}

Should I add that to the Readme, in Fix frequent errors ?

@mariusvn
Copy link
Contributor

mariusvn commented Mar 1, 2023

with AutomaticKeepAliveClientMixin, is the maplibre instance being recreated when reopening the widget ?

@GaelleJoubert
Copy link
Contributor Author

with AutomaticKeepAliveClientMixin, is the maplibre instance being recreated when reopening the widget ?

If I understand correctly, no It is not, it is the same that was created the first time.
I tested it with my app and it works perfectly.
The way my app works is the following :
I have a Scaffold wiith a PageView that handle 3 pages, and Bottom AppBar to navigate these pages.
One of this page is a MapLibre.
Before, when I switched between the pages, I had a crash.
The fix of this PR reduced the occurence of the crash (it use to be all the time, now it's maybe 1/5 time in average.
With the AutomaticKeepAlaive workaround, I don't have anymore crashes, as the page with the map is always the same instance.

I also use other MapLibre instance in some pop up or preview in other pages, but these one did not cause crash, and I did not wrap them in KeppAlives.

I think I have the crash at dispose because each time I switch page the page view rebuild and sipose all pages, and do that too fast, and that cause the crash.

@allou-deyforyou
Copy link

Hey, You have found a solution.

@GaelleJoubert
Copy link
Contributor Author

@mariusvn Hi, did you have any time to check (and update?) the CI that build the apk ?
I've try but I did not figure it out ... this is the last thing preventing this PR to be merge (in my side it builds and works fine. )

1 similar comment
@GaelleJoubert
Copy link
Contributor Author

@mariusvn Hi, did you have any time to check (and update?) the CI that build the apk ?
I've try but I did not figure it out ... this is the last thing preventing this PR to be merge (in my side it builds and works fine. )

@mariusvn
Copy link
Contributor

mariusvn commented Mar 27, 2023

we need to update the dependencies (old versions are in the pubspec.lock) by executing flutter pub upgrade.
I cannot write to your PR so you must do it.

@GaelleJoubert
Copy link
Contributor Author

we need to update the dependencies (old versions are in the pubspec.lock) by executing flutter pub upgrade. I cannot write to your PR so you must do it.

Thanks for the tips ! I'll do it by tomorrow :)

@mariusvn
Copy link
Contributor

i was talking of a flutter upgrade not, updating the pubspec.yaml. You should rollback your last changes because the dependencies upgrade in pubspec.yaml should be done in a separate PR.
The only file who should change is the pubspec.lock (the only thing to do is flutter pub upgrade)

@GaelleJoubert
Copy link
Contributor Author

ok ok, I'll revert the changes.
So the only thing to do is to open the flutter console where my local repo maplibre is and run the command flutter pub upgrade, is that it ?

@mariusvn
Copy link
Contributor

yep should be

@mariusvn
Copy link
Contributor

hmm it looks like it didn't pass but it works on my side after the upgrade, i wait till the end of the android and i'll check

@GaelleJoubert
Copy link
Contributor Author

Ok, I just ran "flutter pub upgrade", but it looks like it is not enough ...

Here is the result of it in my console, if it helps :

C:\Users\GaelleJoubert\StudioProjects\flutter-maplibre-gl>flutter pub upgrade
Resolving dependencies...
> archive 3.3.6 (was 3.1.2)
  characters 1.2.1 (1.3.0 available)
  collection 1.17.0 (1.17.1 available)
+ convert 3.1.1
> crypto 3.0.2 (was 3.0.1)
  flutter 0.0.0 from sdk flutter
  flutter_web_plugins 0.0.0 from sdk flutter
> image 3.3.0 (was 3.0.2) (4.0.15 available)
  js 0.6.5 (0.6.7 available)
  maplibre_gl_dart 0.2.2 from git https://github.com/m0nac0/maplibre-gl-dart.git at 24a48e
! maplibre_gl_platform_interface 0.16.0 from path maplibre_gl_platform_interface (overridden)
! maplibre_gl_web 0.16.0 from path maplibre_gl_web (overridden)
  material_color_utilities 0.2.0 (0.3.0 available)
  meta 1.8.0 (1.9.1 available)
> path 1.8.3 (was 1.8.0)
> petitparser 5.1.0 (was 4.1.0) (5.3.0 available)
+ pointycastle 3.7.2
  sky_engine 0.0.99 from sdk flutter
> typed_data 1.3.1 (was 1.3.0)
  vector_math 2.1.4
> xml 6.2.2 (was 5.1.0)
Warning: You are using these overridden dependencies:
! maplibre_gl_platform_interface 0.16.0 from path maplibre_gl_platform_interface
! maplibre_gl_web 0.16.0 from path maplibre_gl_web
Changed 9 dependencies!
7 packages have newer versions incompatible with dependency constraints.

@mariusvn
Copy link
Contributor

Can you flutter upgrade before doing flutter pub upgrade ? maybe you're not on the last version of flutter (not sure if it will resolve the problem)

@GaelleJoubert
Copy link
Contributor Author

GaelleJoubert commented Mar 27, 2023

I just did that but It did not changed anything :
image

(I did flutter upgrade before this command, but I was already up to date)

@GaelleJoubert
Copy link
Contributor Author

Can you flutter upgrade before doing flutter pub upgrade ? maybe you're not on the last version of flutter (not sure if it will resolve the problem)

But From what I understand, the CI does not use my local version of flutter to build the APK.
(It all work well locally on my side).

But it use subosito, as we can see in the workflow file. It looks like the "not up to date" comes from the workflow library ...
image

GaelleJoubert and others added 2 commits March 27, 2023 17:13
Try to change flutter ci according to exemple given by subosito
@GaelleJoubert
Copy link
Contributor Author

@mariusvn I try to edit flutter ci according to the subosito exemple, but no success here. @m0nac0 any idea what's wrong and how to fix the issue ?
It builds well on both mine and Marius local branch, but it does not build in the CI ...

source: hosted
version: "1.2.1"
file:
dependency: transitive
description:
name: file
url: "https://pub.dartlang.org"
sha256: b69516f2c26a5bcac4eee2e32512e1a5205ab312b3536c1c1227b2b942b5f9ad
url: "https://pub.dev"
source: hosted
version: "6.1.2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we may need to upgrade the file plugin version of the example app. But we shouldn‘t do this manually here in the pubspec.lock file. Have you tried running flutter pub upgrade in the example directory (not the project root) and committed/pushed the changes to example/pubspec.lock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Youhou ! Looks like it was that !!
I ran pub upgrade in the exemple directory (and not the project root), and it worked !! :D
Finnally !!

@m0nac0
Copy link
Collaborator

m0nac0 commented Mar 27, 2023

The CI uses flutter 3.7.7, is that what you are using?

I think the issue is that version 6.1.2 of the file plugin is used in the example app, but there was a breaking change of sorts in version 6.1.3 https://pub.dev/packages/file/changelog

@GaelleJoubert
Copy link
Contributor Author

@m0nac0 ready to be merged, finnaly !

@m0nac0 m0nac0 enabled auto-merge (squash) April 4, 2023 17:13
@m0nac0 m0nac0 merged commit ee82380 into maplibre:main Apr 4, 2023
JanikoNaber pushed a commit to JanikoNaber/flutter-maplibre-gl that referenced this pull request Aug 23, 2023
Fix issue maplibre#182

---------

Co-authored-by: m0nac0 <58807793+m0nac0@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants