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

Bug: Try to use RustArc<dynamic> after it has been disposed #1937

Closed
aran opened this issue May 14, 2024 · 10 comments
Closed

Bug: Try to use RustArc<dynamic> after it has been disposed #1937

aran opened this issue May 14, 2024 · 10 comments
Labels
awaiting Waiting for responses, PR, further discussions, upstream release, etc bug Something isn't working

Comments

@aran
Copy link
Contributor

aran commented May 14, 2024

Describe the bug

I'm stuck on an error that seems to be an error in the serialization strategy for inner opaque objects.

In calling a function that serializes a Vec, it causes a Try to use RustArc after it has been disposed. In the code below, it occurs on the second call to getItemContents.

In the example below, the VecDeque can be replaced by any other type.
It doesn't make a difference if I have full_dep: true or not, or whether I label the offending function serialize to try to get the SSE codec. Haven't managed to find any workaround yet.

Steps to reproduce

Up-to-date flutter.
Confirmed to occur on frb 32 or 33.
Error occurs in flutter web in up-to-date Chrome.

simple.rs: (Here, we take VecDeque as a placeholder for an arbitrary opaque object.)

pub use std::collections::VecDeque;

pub struct ItemContainer {
    pub items: Vec<Item>,
}

pub struct Item {
    pub contents: VecDeque<u8>,
}

#[flutter_rust_bridge::frb(sync)]
pub fn new_container() -> ItemContainer {
    ItemContainer {
        items: vec![
            Item {
                contents: VecDeque::from(vec![1, 2, 3]),
            },
        ],
    }
}

#[flutter_rust_bridge::frb(sync)]
pub fn get_item_contents(container: &ItemContainer) -> Vec<Vec<u8>> {
    container.items.iter().map(|item| item.contents.iter().copied().collect()).collect()
}

#[flutter_rust_bridge::frb(init)]
pub fn init_app() {
    // Default utilities - feel free to customize
    flutter_rust_bridge::setup_default_user_utils();
}
import 'package:flutter/material.dart';
import 'package:dispose_repro/src/rust/api/simple.dart';
import 'package:dispose_repro/src/rust/frb_generated.dart';

Future<void> main() async {
  await RustLib.init();
  runApp(const MyApp());
}

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

  @override
  Widget build(BuildContext context) {
    final container = newContainer();

    final contents = getItemContents(container: container);
    final contents2 = getItemContents(container: container);

    return MaterialApp(
      home: Scaffold(
        appBar: AppBar(title: const Text('flutter_rust_bridge quickstart')),
        body: Center(
          child: Text(''),
        ),
      ),
    );
  }
}
Launching lib/main.dart on Chrome in debug mode...
This app is linked to the debug service: ws://127.0.0.1:62285/4aYkKf0lhmQ=/ws
Debug service listening on ws://127.0.0.1:62285/4aYkKf0lhmQ=/ws
Connecting to VM Service at ws://127.0.0.1:62285/4aYkKf0lhmQ=/ws
══╡ EXCEPTION CAUGHT BY WIDGETS LIBRARY ╞═══════════════════════════════════════════════════════════
The following DroppableDisposedException was thrown building MyApp(dirty):
Try to use `RustArc<dynamic>` after it has been disposed

The relevant error-causing widget was:
  MyApp MyApp:file:///Users/aran/Projects/dispose_repro/lib/main.dart:136:16

When the exception was thrown, this was the stack:
dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 297:3        throw_
packages/flutter_rust_bridge/src/droppable/_common.dart 24:16                      dangerousReadInternalPtr
packages/flutter_rust_bridge/src/rust_arc/_common.dart 13:42                       get [_ptr]
packages/flutter_rust_bridge/src/rust_arc/_common.dart 44:17                       intoRaw
packages/flutter_rust_bridge/src/misc/rust_opaque.dart 52:18                       cstEncode
packages/dispose_repro/src/rust/frb_generated.dart 346:15                          cst_encode_Auto_Owned_RustOpaque_flutter_rust_bridgefor_generatedRustAutoOpaqueInnerItem
dart-sdk/lib/internal/iterable.dart 425:31                                         elementAt
dart-sdk/lib/internal/iterable.dart 354:26                                         moveNext
dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/operations.dart 1140:20  next
dart-sdk/lib/_internal/js_dev_runtime/patch/core_patch.dart 555:14                 of
dart-sdk/lib/internal/iterable.dart 224:7                                          toList
packages/dispose_repro/src/rust/frb_generated.web.dart 125:13                      cst_encode_list_Auto_Owned_RustOpaque_flutter_rust_bridgefor_generatedRustAutoOpaqueInnerItem
packages/dispose_repro/src/rust/frb_generated.web.dart 113:7                       cst_encode_item_container
packages/dispose_repro/src/rust/frb_generated.web.dart 106:12                      cst_encode_box_autoadd_item_container
packages/dispose_repro/src/rust/frb_generated.dart 97:20                           <fn>
packages/flutter_rust_bridge/src/main_components/handler.dart 25:24                executeSync
packages/dispose_repro/src/rust/frb_generated.dart 95:20                           getItemContents
packages/dispose_repro/src/rust/api/simple.dart 14:26                              getItemContents
packages/dispose_repro/main.dart 147:23                                            build
packages/flutter/src/widgets/framework.dart 5550:22                                build
packages/flutter/src/widgets/framework.dart 5480:15                                performRebuild
packages/flutter/src/widgets/framework.dart 5196:7                                 rebuild
packages/flutter/src/widgets/framework.dart 5462:5                                 [_firstBuild]
packages/flutter/src/widgets/framework.dart 5456:5                                 mount
packages/flutter/src/widgets/framework.dart 4335:15                                inflateWidget
packages/flutter/src/widgets/framework.dart 3846:18                                updateChild
packages/flutter/src/widgets/framework.dart 5505:16                                performRebuild
packages/flutter/src/widgets/framework.dart 5196:7                                 rebuild
packages/flutter/src/widgets/framework.dart 5462:5                                 [_firstBuild]
packages/flutter/src/widgets/framework.dart 5456:5                                 mount
packages/flutter/src/widgets/framework.dart 4335:15                                inflateWidget
packages/flutter/src/widgets/framework.dart 3846:18                                updateChild
packages/flutter/src/widgets/framework.dart 5505:16                                performRebuild
packages/flutter/src/widgets/framework.dart 5643:11                                performRebuild
packages/flutter/src/widgets/framework.dart 5196:7                                 rebuild
packages/flutter/src/widgets/framework.dart 5462:5                                 [_firstBuild]
packages/flutter/src/widgets/framework.dart 5634:11                                [_firstBuild]
packages/flutter/src/widgets/framework.dart 5456:5                                 mount
packages/flutter/src/widgets/framework.dart 4335:15                                inflateWidget
packages/flutter/src/widgets/framework.dart 3846:18                                updateChild
packages/flutter/src/widgets/framework.dart 5505:16                                performRebuild
packages/flutter/src/widgets/framework.dart 5196:7                                 rebuild
packages/flutter/src/widgets/framework.dart 5462:5                                 [_firstBuild]
packages/flutter/src/widgets/framework.dart 5456:5                                 mount
packages/flutter/src/widgets/framework.dart 4335:15                                inflateWidget
packages/flutter/src/widgets/framework.dart 3846:18                                updateChild
packages/flutter/src/widgets/framework.dart 5505:16                                performRebuild
packages/flutter/src/widgets/framework.dart 5196:7                                 rebuild
packages/flutter/src/widgets/framework.dart 5462:5                                 [_firstBuild]
packages/flutter/src/widgets/framework.dart 5456:5                                 mount
packages/flutter/src/widgets/framework.dart 4335:15                                inflateWidget
packages/flutter/src/widgets/framework.dart 3846:18                                updateChild
packages/flutter/src/widgets/view.dart 291:16                                      [_updateChild]
packages/flutter/src/widgets/view.dart 314:5                                       mount
packages/flutter/src/widgets/framework.dart 4335:15                                inflateWidget
packages/flutter/src/widgets/framework.dart 3846:18                                updateChild
packages/flutter/src/widgets/framework.dart 5505:16                                performRebuild
packages/flutter/src/widgets/framework.dart 5196:7                                 rebuild
packages/flutter/src/widgets/framework.dart 5462:5                                 [_firstBuild]
packages/flutter/src/widgets/framework.dart 5456:5                                 mount
packages/flutter/src/widgets/framework.dart 4335:15                                inflateWidget
packages/flutter/src/widgets/framework.dart 3846:18                                updateChild
packages/flutter/src/widgets/binding.dart 1354:16                                  [_rebuild]
packages/flutter/src/widgets/binding.dart 1323:5                                   mount
packages/flutter/src/widgets/binding.dart 1276:16                                  <fn>
packages/flutter/src/widgets/framework.dart 2844:11                                buildScope
packages/flutter/src/widgets/binding.dart 1275:12                                  attach
packages/flutter/src/widgets/binding.dart 1088:26                                  attachToBuildOwner
packages/flutter/src/widgets/binding.dart 1070:5                                   attachRootWidget
packages/flutter/src/widgets/binding.dart 1056:7                                   <fn>
dart-sdk/lib/_internal/js_dev_runtime/private/isolate_helper.dart 48:11            internalCallback

════════════════════════════════════════════════════════════════════════════════════════════════════

Logs

N/A

Expected behavior

No response

Generated binding code

No response

OS

No response

Version of flutter_rust_bridge_codegen

No response

Flutter info

No response

Version of clang++

No response

Additional context

No response

@aran aran added the bug Something isn't working label May 14, 2024
@fzyzcjy
Copy link
Owner

fzyzcjy commented May 14, 2024

Hi, could you please firstly check whether ItemContainer is opaque or non-opaque? If it is opaque then it should be a bug. Otherwise, if it is non-opaque, then the code is that a Vec<Item> is obtained - which is a vec of owned Item, thus it cannot be obtained twice. For non-opaque, the solution is to mark as #[frb(opaque)] (or try to use it in other ways)

@fzyzcjy fzyzcjy added the awaiting Waiting for responses, PR, further discussions, upstream release, etc label May 14, 2024
@aran
Copy link
Contributor Author

aran commented May 14, 2024

Yes, ItemContainer is non-opaque, and the bug does not occur in this repro when it is opaque. I don't understand why that makes it not a bug though, can you explain a little more?

getItemContents receives the ItemContainer by reference, so it shouldn't be possible for owned data to escape. The error seems caused just by calling the function twice, thereby serializing the data twice, and doesn't seem to depend on what are the contents of the function. But if we look at the function, maybe I misunderstand, but looks likeget_item_contents doesn't leak the items themselves, it processes them and returns copied data.

In my real app, there's an ergonomics issue too. If this isn't a bug, adding one opaque leaf field then requires converting the entire object tree to opaque to keep correctness?

@aran
Copy link
Contributor Author

aran commented May 14, 2024

As a bit more context, I was hoping to keep as much data non opaque as possible. It's not performance intensive, but my structs have a bunch of pub fields. To make them opaque, in the original data crate I need to add a boilerplate impl full of getters that clone, and then mirror that impl as an external in the FRB crate to expose the getters to the Dart code.

@fzyzcjy
Copy link
Owner

fzyzcjy commented May 15, 2024

I don't understand why that makes it not a bug though, can you explain a little more?

When ItemContainer is non-opaque (i.e. like a "plain old java object (POJO)"), the whole object (with each of its field) is constructed each time it is called, no matter whether there is & or not, because there is no way to keep a single Rust object. Then you can see that, surely we cannot construct two ItemContainer objects.

I was hoping to keep as much data non opaque as possible.
I need to add a boilerplate impl full of getters that clone, and then mirror that impl as an external in the FRB crate to expose the getters to the Dart code.

Btw I am thinking about another feature: Even if something is opaque, if it has pub fields, we can automatically generate getters and setters for it. For example,

#[frb(opaque)]
pub struct S {
  pub a: i32, // <- pub, thus we can generate getter&setter
  b: i32, // <- private, no way
}

Then we get this in Dart

class S {
  int get a => ...auto call rust;
  set a (int value) => ...auto call rust;
}

What do you think?

@aran
Copy link
Contributor Author

aran commented May 15, 2024

I like the automatic getters and setters. It would be nice to have an annotation to turn them off if I want to write my own impl for them.

I think maybe there should be an error if combining & with non-opaque containing opaque because it is not safe and doesn't respect the semantics of &.

@aran
Copy link
Contributor Author

aran commented May 15, 2024

Oh one more point, not sure if it's relevant, but my Item.contents implements Clone. So you could safely clone the rust-side object when constructing new ItemContainer from the same Rust obj.

@fzyzcjy
Copy link
Owner

fzyzcjy commented May 16, 2024

It would be nice to have an annotation to turn them off if I want to write my own impl for them.

I guess one way is to only generate for pub and ignore for non-pub, since the semantics for pub means "it should be publicly accessible (from external code like Dart)".

I think maybe there should be an error if combining & with non-opaque containing opaque because it is not safe and doesn't respect the semantics of &.
Oh one more point, not sure if it's relevant, but my Item.contents implements Clone. So you could safely clone the rust-side object when constructing new ItemContainer from the same Rust obj.

Yes, the semantics is somehow a problem. Some more discussions are in #1906 (comment)

@kjzl
Copy link

kjzl commented May 23, 2024

I ran into this issue once again, because many of my non_opaque structs had one opaque struct deeply nested inside. I do not really understand why this causes issues, but it seems like having structs which are made up of opaque structs essentially makes them kinda "poisoned", if they are not also opaque. Seems to me that having the ability to create explicitly non_opaque structs does not really make sense then if its so easy to misuse, and the idea with the automatically gen getters would solve the problem, right? Or would that not change a thing either?

@fzyzcjy
Copy link
Owner

fzyzcjy commented May 23, 2024

I am considering some changes like: For non-opaque structs, support code such as

pub struct MyNonOpaqueStruct {
  a: String,
  b: RustAutoOpaque<MyAnotherOpaqueType>,
}

Then, you can use MyNonOpaqueStruct as non-opaque type and freely access its field, and no worries about the issue above, because RustAutoOpaque is essentially an Arc.

@fzyzcjy
Copy link
Owner

fzyzcjy commented May 26, 2024

Now auto getters/setters of opaque types is supported: https://cjycode.com/flutter_rust_bridge/guides/types/arbitrary/rust-auto-opaque/properties

And, opaque types can also be inside non-opaque types: https://cjycode.com/flutter_rust_bridge/guides/types/arbitrary/rust-auto-opaque/opaque-in-translatable

Thus closing this. Feel free to reopen if needed!

@fzyzcjy fzyzcjy closed this as completed May 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting Waiting for responses, PR, further discussions, upstream release, etc bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants