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

removeOverlay condition too strict #182

Open
1 task done
franzlst opened this issue Oct 11, 2024 · 13 comments · Fixed by #185
Open
1 task done

removeOverlay condition too strict #182

franzlst opened this issue Oct 11, 2024 · 13 comments · Fixed by #185
Labels
bug Something isn't working P3 Low Priority: A bug or a feature request we plan to work on in near future (default)

Comments

@franzlst
Copy link

Describe the bug
I have a strange bug that does not always occur, but which seems to be some kind of race condition. However, it is quite severe and leads to a freeze of the whole Flutter app. So far, I was only able to reproduce it in the web build of my app, not in any debug builds. The error logs are not so helpful:

08:44:22.610 Bad state: RenderBox was not laid out: minified:am0#1cb7a [main.dart.js:48094:78](https://***.com/main.dart.js)
08:44:22.611 <empty string> [main.dart.js:48094:78](https://***.com/main.dart.js)
08:44:22.615 Another exception was thrown: Instance of 'minified:lb<void>' 334 [main.dart.js:48094:78](https://***.com/main.dart.js)

The error occurs when repeatedly clicking inside and outside of a SearchField.

However, in the debug builds I was able to sporadically create an error, which might be related to the crash:

======== Exception caught by foundation library ====================================================
The following assertion was thrown while dispatching notifications for FocusNode:
The specified entry is already present in the target Overlay.

The OverlayEntry was: OverlayEntry#fce75(opaque: false; maintainState: false)
The Overlay the OverlayEntry was trying to insert to was: OverlayState#1542a(entries: [OverlayEntry#5ea75(opaque: true; maintainState: false), OverlayEntry#8bd21(opaque: false; maintainState: true), OverlayEntry#fce75(opaque: false; maintainState: false)])
  entries: [OverlayEntry#5ea75(opaque: true; maintainState: false), OverlayEntry#8bd21(opaque: false; maintainState: true), OverlayEntry#fce75(opaque: false; maintainState: false)]

Consider calling remove on the OverlayEntry before inserting it to a different Overlay, or switching to the OverlayPortal API to avoid manual OverlayEntry management.

When the exception was thrown, this was the stack: 
dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 296:3  throw_
packages/flutter/src/widgets/overlay.dart 632:7                              [_debugCanInsertEntry]
packages/flutter/src/widgets/overlay.dart 670:12                             insert
packages/searchfield/src/searchfield.dart 465:20                             <fn>
packages/flutter/src/foundation/change_notifier.dart 437:24                  notifyListeners
packages/flutter/src/widgets/focus_manager.dart 1090:5                       [_notify]
packages/flutter/src/widgets/focus_manager.dart 1871:11                      applyFocusChangesIfNeeded
dart-sdk/lib/async/schedule_microtask.dart 40:11                             _microtaskLoop
dart-sdk/lib/async/schedule_microtask.dart 49:5                              _startMicrotaskLoop
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 181:7           <fn>
The FocusNode sending notification was: FocusNode#f6638([PRIMARY FOCUS])
  context: Focus
  PRIMARY FOCUS
====================================================================================================

By adding some debug print statements, I think I was able to pin down the cause of the exception. The current implementation of removeOverlay() is the following:

  void removeOverlay() {
    if (_overlayEntry != null && _overlayEntry!.mounted) {
      isSuggestionsShown = false;
      if (_overlayEntry != null) {
        _overlayEntry?.remove();
      }
    }
  }

So the overlay is only removed, when it is mounted. This causes the overlay not always to be removed, as the widget might not be removed immediately:

After the owner of the OverlayEntry calls remove and then dispose, the widget may not be immediately removed from the widget tree. As a result listeners of the OverlayEntry can get notified for one last time after the dispose call, when the widget is eventually unmounted.

From: https://api.flutter.dev/flutter/widgets/OverlayEntry-class.html

After removing the condition && _overlayEntry!.mounted, I was no longer able to reproduce the issue locally.

  • By clicking this checkbox, I confirm I am using the latest version of the package found on pub.dev/searchfield
@maheshj01 maheshj01 added the in triage Issue is currently being triaged label Oct 11, 2024
@maheshj01
Copy link
Owner

maheshj01 commented Oct 11, 2024

Hello @franzlst, Thanks for the detailed issue. I will take a look at it soon currently afk.

@maheshj01 maheshj01 added bug Something isn't working P1 High Priority: This is a show stopper and must be addressed immediately and removed in triage Issue is currently being triaged labels Oct 11, 2024
@maheshj01
Copy link
Owner

Hi @franzlst, Are you seeing this issue on the latest flutter stable channel? Unfortunately I am unable to reproduce it
on Flutter (Channel stable, 3.24.3, on macOS 15.0 24A335 darwin-arm64

code sample
// import 'package:example/pagination.dart';
import 'package:flutter/material.dart';
import 'package:searchfield/searchfield.dart';

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

class MyApp extends StatelessWidget {
  const MyApp({Key? key}) : super(key: key);

  // This widget is the root of your application.
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter App',
      themeMode: ThemeMode.light,
      theme: ThemeData(
        colorSchemeSeed: Colors.indigo,
        useMaterial3: true,
        brightness: Brightness.light,
      ),
      darkTheme: ThemeData(
        colorSchemeSeed: Colors.blue,
        useMaterial3: true,
        brightness: Brightness.dark,
      ),
      home: SearchFieldSample(),
      debugShowCheckedModeBanner: false,
    );
  }
}

class SearchFieldSample extends StatefulWidget {
  const SearchFieldSample({Key? key}) : super(key: key);

  @override
  State<SearchFieldSample> createState() => _SearchFieldSampleState();
}

class _SearchFieldSampleState extends State<SearchFieldSample> {
  @override
  void initState() {
    suggestions = [
      'United States',
      'Germany',
      'Canada',
      'United Kingdom',
      'France',
      'Italy',
      'Spain',
      'Australia',
      'India',
      'China',
      'Japan',
      'Brazil',
      'South Africa',
      'Mexico',
      'Argentina',
      'Russia',
      'Indonesia',
      'Turkey',
      'Saudi Arabia',
      'Nigeria',
      'Egypt',
    ];
    selected = suggestions[0];
    super.initState();
  }

  int suggestionsCount = 12;
  var suggestions = <String>[];
  var selected = '';
  @override
  Widget build(BuildContext context) {
    Widget searchChild(x, {bool isSelected = false}) => Padding(
          padding: const EdgeInsets.symmetric(horizontal: 12),
          child: Text(x,
              style: TextStyle(
                  fontSize: 18,
                  color: isSelected ? Colors.green : Colors.black)),
        );
    return Scaffold(
        appBar: AppBar(title: Text('Searchfield Demo')),
        body: Padding(
          padding: const EdgeInsets.all(8.0),
          child: ListView(
            children: [
              Padding(
                padding: const EdgeInsets.all(8.0),
                child: SearchField<String>(
                  maxSuggestionsInViewPort: 10,
                  suggestionAction: SuggestionAction.unfocus,
                  searchInputDecoration: SearchInputDecoration(
                    hintText: 'Search',
                    cursorColor: Colors.blue,
                    border: OutlineInputBorder(
                      borderRadius: BorderRadius.circular(10),
                    ),
                  ),
                  onSuggestionTap: (SearchFieldListItem<String> item) {
                    setState(() {
                      selected = item.searchKey;
                    });
                  },
                  suggestions: suggestions
                      .map(
                        (e) => SearchFieldListItem<String>(e,
                            item: e,
                            child: searchChild(e, isSelected: e == selected)),
                      )
                      .toList(),
                ),
              ),
            ],
          ),
        ));
  }
}
output_video.mov

@maheshj01 maheshj01 added waiting for author waiting for author to respond back with more info in triage Issue is currently being triaged and removed P1 High Priority: This is a show stopper and must be addressed immediately bug Something isn't working labels Oct 15, 2024
@franzlst
Copy link
Author

@maheshj01 Thank you for looking into this.

The following is a minimum example with which I can reproduce the issue:

import 'package:flutter/material.dart';
import 'package:searchfield/searchfield.dart';

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

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  // This widget is the root of your application.
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Searchfield Crash',
      theme: ThemeData(
        colorScheme: ColorScheme.fromSeed(seedColor: Colors.deepPurple),
        useMaterial3: true,
      ),
      home: const MyHomePage(title: 'Searchfield Crash'),
    );
  }
}

class MyHomePage extends StatefulWidget {
  const MyHomePage({super.key, required this.title});
  final String title;

  @override
  State<MyHomePage> createState() => _MyHomePageState();
}

class _MyHomePageState extends State<MyHomePage> {
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: Text(widget.title),
      ),
      body: Center(
        child: Column(
          crossAxisAlignment: CrossAxisAlignment.start,
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            SearchField<String>(
              suggestionAction: SuggestionAction.unfocus,
              suggestions: const [],
              initialValue: null, // user actively ne
              textInputAction: TextInputAction.search,
              suggestionState: Suggestion.hidden,
              emptyWidget: const SizedBox.shrink(),
              suggestionDirection: SuggestionDirection.flex,
            ),
          ],
        ),
      ),
    );
  }
}

In order to reproduce it, you need to build it (flutter build web --web-renderer canvaskit) and run it from the generated build. In this small app, it is not so easy to reproduce the issue (for me, probably system dependent). By purely using the mouse, I cannot reproduce it. Instead, I use the mouse to click into the SearchField and the Tab key to move the focus out of the field. By this, I can change the focus very frequently. You should also have the DevTools with the Console open to see the exception. I can reproduce the issue on Firefox 128.3 and Chrome 129, both on Windows.

I always get the following exceptions:

9:23:53.132 Null check operator used on a null value [js_primitives.dart:28:4](org-dartlang-sdk:///dart-sdk/lib/_internal/js_runtime/lib/js_primitives.dart)
09:23:53.134 <empty string> [js_primitives.dart:28:4](org-dartlang-sdk:///dart-sdk/lib/_internal/js_runtime/lib/js_primitives.dart)
09:23:53.145 Another exception was thrown: Instance of 'minified:hy<erased>' 41 [js_primitives.dart:28:4](org-dartlang-sdk:///dart-sdk/lib/_internal/js_runtime/lib/js_primitives.dart)

I tried to generate source maps, but somehow this doesn't work.

In any case, when changing the code as described above, the issue no longer occurs.

@franzlst
Copy link
Author

Some additional info: I was now able to use source maps to pin point the source of the exception:

At some point BuildScope_flushDirtyElements is called. Somewhere down the trace this ultimately calls the getter Widget get widget => _widget!; of the Element class. The _widget is no longe existing, which causes the exception "Null check operator used on a null value".

Within remove() of the OverlayEntry _markDirty is called. So this is definitely related to the removal of the overlay. Maybe it is also related to the fact, the the overlay widget is never recreated, but is always reused. So maybe it is marked as dirty, gets reinserted into the tree and then it is picked up and destroyed.

@maheshj01 maheshj01 removed the waiting for author waiting for author to respond back with more info label Oct 20, 2024
@maheshj01
Copy link
Owner

maheshj01 commented Oct 20, 2024

Thanks for the details @franzlst, Unfortunately I am not able to reproduce that issue on my end, I understand the mounted condition is not required, _overlayEntry != null should be enough, the other mounted condition has been added to accurately detect if overlay is still present in the widget tree, Because _overlayEntry != null is malfunctioned at this time see #43 and flutter/flutter#145466

Also, removing _overlayEntry!.mounted would break other tests.

@maheshj01 maheshj01 added the waiting for author waiting for author to respond back with more info label Oct 20, 2024
@franzlst
Copy link
Author

@maheshj01 That's a pity. Do you think it's an option to re-create the Overlay every time it's being required instead of re-using it? So basically calling
_overlayEntry = _createOverlay();
instead of
_overlayEntry ??= _createOverlay();

@maheshj01
Copy link
Owner

maheshj01 commented Oct 21, 2024

I think that would be unnecessary work to recreate the overlay everytime, considering the overlay widget could be a custom heavy widget with long list of items, But we can compromise performance (only in worst case scenarios) as a work around to prevent the exception that you are receiving until this is resolved flutter/flutter#145466

Does recreating overlay fixes issue on your end? If so, I can go ahead and submit a new release

@maheshj01 maheshj01 added waiting for author waiting for author to respond back with more info and removed waiting for author waiting for author to respond back with more info labels Oct 21, 2024
@franzlst
Copy link
Author

@maheshj01 I think a found a good compromise:
We do not need to recreate the full OverlayEntry, but we can e.g. keep the StreamBuilder:

Widget? _streamBuilder;
  OverlayEntry _createOverlay() {
    return OverlayEntry(builder: (context) {
      if(_streamBuilder != null) return _streamBuilder!;
      final textFieldRenderBox =
          key.currentContext!.findRenderObject() as RenderBox;
      final textFieldsize = textFieldRenderBox.size;
      final offset = textFieldRenderBox.localToGlobal(Offset.zero);
      var yOffset = Offset.zero;
      _totalHeight = widget.maxSuggestionsInViewPort * widget.itemHeight;
      _streamBuilder = StreamBuilder<List<SearchFieldListItem?>?>(
          stream: suggestionStream.stream,
          builder: (BuildContext context,
              AsyncSnapshot<List<SearchFieldListItem?>?> snapshot) {
            late var count = widget.maxSuggestionsInViewPort;
            if (snapshot.data != null) {
              count = snapshot.data!.length;
            }
            yOffset = getYOffset(offset, textFieldsize, count) ?? Offset.zero;
            return Positioned(
              left: offset.dx,
              width: widget.suggestionsDecoration?.width ?? textFieldsize.width,
              child: CompositedTransformFollower(
                offset: widget.offset ?? yOffset,
                link: _layerLink,
                child: Material(
                  borderRadius: widget.suggestionsDecoration?.borderRadius ??
                      BorderRadius.zero,
                  shadowColor: widget.suggestionsDecoration?.shadowColor,
                  elevation: widget.suggestionsDecoration?.elevation ??
                      kDefaultElevation,
                  child: _suggestionsBuilder(),
                ),
              ),
            );
          });
      return _streamBuilder!;
    });
  }

What I tried this changing removeOverlay():

void removeOverlay() {
    final overlay = _overlayEntry;
    _overlayEntry = null;
    if (overlay != null && overlay!.mounted) {
      isSuggestionsShown = false;
        overlay!.remove();
      }
    }
  }

And always creating the OverlayEntry:
_overlayEntry = _createOverlay();

With those changes (that probably hardly have an influence on performance), I am no longer able to reproduce the issue.

@maheshj01
Copy link
Owner

maheshj01 commented Oct 22, 2024

Sounds good to me feel free to send a PR, I will be travelling but I can review the PR. Otherwise I can take a look at it over the weekend.

Edit: Nevermind submitted a PR

@maheshj01
Copy link
Owner

Published a new release v1.1.7

Thank you for your contribution!

@maheshj01 maheshj01 added fixed The issue is fixed in new release and removed waiting for author waiting for author to respond back with more info in triage Issue is currently being triaged labels Oct 22, 2024
@franzlst
Copy link
Author

@maheshj01 Thanks a lot for integrating this. I can confirm that the error no longer occurs, thus the issue is fixed for me.

I still sporadically see message "Null check operator used on a null value" in the console,. but without any negative effect.

@sabari7320
Copy link

@franzlst sir after entering search text respective suggestions showing then i closed keyboard all the suggestions start from beginning check this https://drive.google.com/file/d/132iZYWgLzDNMllLGyOlkA15SayzkrcZe/view

@maheshj01
Copy link
Owner

@franzlst I will have to reopen this issue, The commit made to fix this issue broke the keyboard functionality #179. I am reverting back that change.

@maheshj01 maheshj01 reopened this Nov 25, 2024
@maheshj01 maheshj01 added bug Something isn't working P3 Low Priority: A bug or a feature request we plan to work on in near future (default) and removed fixed The issue is fixed in new release labels Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working P3 Low Priority: A bug or a feature request we plan to work on in near future (default)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants