Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Always use fml::TaskRunnerAffineWeakPtr for SnapshotDelegate #36772

Merged
dnfield merged 1 commit into from
Oct 15, 2022
Merged

Always use fml::TaskRunnerAffineWeakPtr for SnapshotDelegate #36772

dnfield merged 1 commit into from
Oct 15, 2022

Conversation

@dnfield
Copy link
Contributor

@dnfield dnfield commented Oct 14, 2022 •
edited
Loading

Fixes flutter/flutter#113437

I hope someone who knows C++ better can explain this to me, I would not have expected this to cause problems (maybe it's related to unopt builds? it can only show up in an unopt build).

What's happening before this patch is that when threads get merged, the checks from fml::WeakPtr are getting used instead of fml::TaskRunnerAffineWeakPtr, which is what SnapshotDelegate is. I don't get why the upcast causes that.

@stuartmorgan fyi

@dnfield dnfield merged commit 9e6721e into flutter:main Oct 15, 2022
@dnfield dnfield deleted the snapshot_delegate branch October 15, 2022 01:10
@stuartmorgan-g
Copy link
Contributor

stuartmorgan-g commented Oct 15, 2022

I hope someone who knows C++ better can explain this to me, I would not have expected this to cause problems

Capturing from Discord for future code archeologists: the issue here was object slicing. Since the WeakPtrs are passed by value, the received object is a new base WeakPtr created by passing the the subclass object to the WeakPtr copy constructor.

@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 17, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 17, 2022
@engine-flutter-autoroll
9e6721e Always use fml::TaskRunnerAffineWeakPtr for SnapshotDelegate (f…
2e19909 
…lutter/engine#36772)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 17, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 17, 2022
@engine-flutter-autoroll
9e6721e Always use fml::TaskRunnerAffineWeakPtr for SnapshotDelegate (f…
b7b5441 
…lutter/engine#36772)
@flar
Copy link
Contributor

flar commented Oct 17, 2022

I hope someone who knows C++ better can explain this to me, I would not have expected this to cause problems

Capturing from Discord for future code archeologists: the issue here was object slicing. Since the WeakPtrs are passed by value, the received object is a new base WeakPtr created by passing the the subclass object to the WeakPtr copy constructor.

Shouldn't that copy constructor be deleted to prevent future issues?

@stuartmorgan-g
Copy link
Contributor

stuartmorgan-g commented Oct 17, 2022

Isn't WeakPtr intentionally copyable?

From Discord it sounded like the longer-term plan was to make TaskRunnerAffineWeakPtr not inherit from WeakPtr to prevent this class of problems.

@jason-simmons
Copy link
Member

jason-simmons commented Oct 17, 2022

Yes - given how these classes work I don't think it makes sense for TaskRunnerAffineWeakPtr to be a subclass of WeakPtr or be assignable to a WeakPtr.

I have a patch in progress that fixes this.

@flar
Copy link
Contributor

flar commented Oct 17, 2022

Isn't WeakPtr intentionally copyable?

Maybe copying makes sense for base WeakPtr objects, but it shouldn't be inherited through the subclass that imposes additional constraints. It sounds like this is being addressed though.

jason-simmons added a commit to jason-simmons/flutter_engine that referenced this pull request Oct 17, 2022
@jason-simmons
Change TaskRunnerAffineWeakPtr to no longer be a subclass of WeakPtr
236f886 
Constructing a WeakPtr from a TaskRunnerAffineWeakPtr is undesirable
because the resulting WeakPtr will get default WeakPtr thread checking
behavior.

See flutter#36772
@jason-simmons jason-simmons mentioned this pull request Oct 17, 2022
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@cyanglaz cyanglaz Awaiting requested review from cyanglaz

@gaaclarke gaaclarke Awaiting requested review from gaaclarke

1 more reviewer

@jason-simmons jason-simmons jason-simmons approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Thread assertion failure in SnapshotDisplayList with AndroidPlatformView

4 participants

@dnfield @stuartmorgan-g @flar @jason-simmons