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

[Web] Release picked file data for Web #1482

Merged
merged 2 commits into from
Jun 12, 2024

Conversation

tddang-linagora
Copy link
Contributor

Issue

There's a memory leak happening with web version

Reproducible code

See here
import 'package:file_picker/file_picker.dart';
import 'package:flutter/material.dart';

void main() => runApp(const MyApp());

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

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Material App',
      home: FirstScreen(),
    );
  }
}

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

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Center(
        child: TextButton(
          onPressed: () {
            Navigator.push(
              context,
              MaterialPageRoute(builder: (context) => const SecondScreen()),
            );
          },
          child: Text('Go to second screen'),
        ),
      ),
    );
  }
}

class SecondScreen extends StatefulWidget {
  const SecondScreen({super.key});

  @override
  State<SecondScreen> createState() => _SecondScreenState();
}

class _SecondScreenState extends State<SecondScreen> {
  String _name = '';

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: [
            Text("Picked file's name: $_name"),
            TextButton(
              onPressed: () async {
                final picked = await FilePicker.platform.pickFiles(
                  type: FileType.image,
                  withData: true,
                );
                if (mounted && picked != null) {
                  setState(() {
                    _name = picked.files.firstOrNull?.name ?? '';
                  });
                }
              },
              child: Text('Pick image'),
            ),
            TextButton(
              onPressed: () {
                Navigator.pop(context);
              },
              child: Text('Go back'),
            ),
          ],
        ),
      ),
    );
  }
}

Steps to reproduce

  1. Run flutter run -d chrome --profile
  2. Open Chrome dev tools, Memory tab, trigger garbage collector then snapshot main heap
  3. Navigate to second screen by clicking on Go to second screen button
  4. Pick a heavy file. In the above code, I chose to pick image because I already have a heavy image (16.3mb)
  5. Go back to the previous screen either by clicking on Go back or any conventional ways for web
  6. Trigger garbage collector then snapshot main heap again. Compare two snapshots.

Expected behavior

The file's data will be deallocated from memory

Actual behavior

The file's data is still on memory

Resolved demo

Before
Screenshot 2024-04-09 at 11 51 30
After
Screenshot 2024-04-09 at 11 53 41

@amrgetment
Copy link
Contributor

as my PR for WASM support got merged you need to update clear children, check the file changes here
https://github.com/miguelpruivo/flutter_file_picker/pull/1481/files
image

@tddang-linagora
Copy link
Contributor Author

as my PR for WASM support got merged you need to update clear children, check the file changes here https://github.com/miguelpruivo/flutter_file_picker/pull/1481/files image

Right. Thank you.

@navaronbracke
Copy link
Collaborator

@tddang-linagora Could you fix the merge conflict for the version & changelog? We released a new version, which removes references to the v1 Android embedding, but that means your version needs to be incremented to 8.0.5.

@tddang-linagora
Copy link
Contributor Author

@tddang-linagora Could you fix the merge conflict for the version & changelog? We released a new version, which removes references to the v1 Android embedding, but that means your version needs to be incremented to 8.0.5.

Done, thank you.

@navaronbracke
Copy link
Collaborator

navaronbracke commented Jun 11, 2024

@tddang-linagora Hmm, it seems the clear() method is not defined for the Element class (of which _target is an instance). I think you can just remove that line? You iterate manually using firstChild anyway.

As for the Windows deprecation warnings, I think you can just replace them with their replacements, as noted in the deprecation message. I think we picked up a new version of win32 along the way.

   info - lib/src/windows/file_picker_windows.dart:97:18 - 'COINIT_APARTMENTTHREADED' is deprecated and shouldn't be used. Use COINIT.COINIT_APARTMENTTHREADED instead. Try replacing the use of the deprecated member with the replacement. - deprecated_member_use
   info - lib/src/windows/file_picker_windows.dart:97:45 - 'COINIT_DISABLE_OLE1DDE' is deprecated and shouldn't be used. Use COINIT.COINIT_DISABLE_OLE1DDE instead. Try replacing the use of the deprecated member with the replacement. - deprecated_member_use
   info - lib/src/windows/file_picker_windows.dart:137:36 - 'ERROR_CANCELLED' is deprecated and shouldn't be used. Use WIN32_ERROR.ERROR_CANCELLED instead. Try replacing the use of the deprecated member with the replacement. - deprecated_member_use

@tddang-linagora
Copy link
Contributor Author

Hmm, it seems the clear() method is not defined for the Element class (of which _target is an instance). I think you can just remove that line? You iterate manually using firstChild anyway.

Yep, my bad sorry. Fixed.

As for the Windows deprecation warnings, I think you can just replace them with their replacements, as noted in the deprecation message.

Also done.

@navaronbracke
Copy link
Collaborator

navaronbracke commented Jun 11, 2024

@tddang-linagora Looks like you'll need to run dart format on lib/src/windows/file_picker_windows.dart.
It think that first change in the Windows implementation is now over the 80 character threshold for formatting.

Adding a comma like this should fix it.

    int hr = CoInitializeEx(
        nullptr, COINIT.COINIT_APARTMENTTHREADED | COINIT.COINIT_DISABLE_OLE1DDE,);

then the formatter will probably format it like:

    int hr = CoInitializeEx(
        nullptr,
        COINIT.COINIT_APARTMENTTHREADED | COINIT.COINIT_DISABLE_OLE1DDE,
     );

@tddang-linagora
Copy link
Contributor Author

Yep sorry I turned off my formatters

@navaronbracke navaronbracke merged commit 5e989e4 into miguelpruivo:master Jun 12, 2024
3 checks passed
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.

3 participants