searchfield icon indicating copy to clipboard operation
searchfield copied to clipboard

removeOverlay condition too strict

Open franzlst opened this issue 1 year ago • 13 comments

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.

  • [x] By clicking this checkbox, I confirm I am using the latest version of the package found on pub.dev/searchfield

franzlst avatar Oct 11 '24 07:10 franzlst

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

maheshj01 avatar Oct 11 '24 20:10 maheshj01

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(),
                ),
              ),
            ],
          ),
        ));
  }
}

https://github.com/user-attachments/assets/f9e08e54-d0be-4347-8ab0-6cd4a6e3f41c

maheshj01 avatar Oct 15 '24 01:10 maheshj01

@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 avatar Oct 15 '24 07:10 franzlst

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.

franzlst avatar Oct 15 '24 09:10 franzlst

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 https://github.com/maheshj01/searchfield/issues/43 and https://github.com/flutter/flutter/issues/145466

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

maheshj01 avatar Oct 20 '24 13:10 maheshj01

@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();

franzlst avatar Oct 21 '24 06:10 franzlst

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 https://github.com/flutter/flutter/issues/145466

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

maheshj01 avatar Oct 21 '24 16:10 maheshj01

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

franzlst avatar Oct 22 '24 09:10 franzlst

~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 avatar Oct 22 '24 20:10 maheshj01

Published a new release v1.1.7

Thank you for your contribution!

maheshj01 avatar Oct 22 '24 20:10 maheshj01

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

franzlst avatar Oct 23 '24 07:10 franzlst

@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

sabari7320 avatar Nov 06 '24 12:11 sabari7320

@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 avatar Nov 25 '24 20:11 maheshj01

@franzlst Curious is this still and issue with the latest version of the package v1.29?

maheshj01 avatar May 05 '25 20:05 maheshj01

@maheshj01 I could imagine that this solves the issues. However, we have now switched to the native Flutter widget Autocomplete. Therefore I cannot say for sure.

franzlst avatar May 06 '25 05:05 franzlst

I am going to close this issue as solved for now. Should anyone encounter this issue feel free to comment here or file a new issue and I would love to take a look.

maheshj01 avatar May 16 '25 22:05 maheshj01