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

Made flutter_view_ atomic #22259

Closed
gaaclarke wants to merge 1 commit into from
Closed

Made flutter_view_ atomic #22259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 25 additions & 11 deletions shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@
}

void FlutterPlatformViewsController::SetFlutterView(UIView* flutter_view) {
flutter_view_.reset([flutter_view retain]);
[flutter_view_.exchange([flutter_view retain]) release];
Copy link

@blasten blasten Nov 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: what about a comment that this is retaining the new flutter_view and releasing the previous reference?

Copy link
Member Author

@gaaclarke gaaclarke Nov 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is what the one line of code says =)

}

void FlutterPlatformViewsController::SetFlutterViewController(
Expand Down Expand Up @@ -358,7 +358,7 @@
// `ChildClippingView`. Since the mask view's frame will be based on the `clipView`'s coordinate
// system, we need to convert the flutter_view's frame to the clipView's coordinate system. The
// mask view is not displayed on the screen.
CGRect maskViewFrame = [flutter_view_ convertRect:flutter_view_.get().frame toView:clipView];
CGRect maskViewFrame = [flutter_view_ convertRect:flutter_view_.load().frame toView:clipView];
FlutterClippingMaskView* maskView =
[[[FlutterClippingMaskView alloc] initWithFrame:maskViewFrame] autorelease];
auto iter = mutators_stack.Begin();
Expand Down Expand Up @@ -433,7 +433,7 @@
}

void FlutterPlatformViewsController::Reset() {
UIView* flutter_view = flutter_view_.get();
UIView* flutter_view = flutter_view_.load();
for (UIView* sub_view in [flutter_view subviews]) {
[sub_view removeFromSuperview];
}
Expand Down Expand Up @@ -469,7 +469,19 @@
bool FlutterPlatformViewsController::SubmitFrame(GrDirectContext* gr_context,
std::shared_ptr<IOSContext> ios_context,
std::unique_ptr<SurfaceFrame> frame) {
FML_DCHECK(flutter_view_);
// Warning: flutter_view_ could be deleted on the platform thread between load
// and retain. Both are atomic but their composition isn't.
fml::scoped_nsobject<UIView> flutter_view([flutter_view_.load() retain]);
Copy link

@blasten blasten Nov 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is retain required again?

Copy link
Member Author

@gaaclarke gaaclarke Nov 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we are dealing with multiple threads. The platform thread could delete the view while we are running SubmitFrame on the raster thread. This protects us from that (except in the case it is deleted between load and retain).

Copy link

@blasten blasten Nov 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting. Do you know what event can cause the platform thread to delete the view? I guess in add-to-app that can happen at any time.

if (!flutter_view) {
// `flutter_view_` is set by the platform thread but read on the raster
// thread. It is possible this is executing on the raster thread before the
// flutter_view_ has been set. In such case we just submit the frame
// instead of crashing. The may cause incorrect frames to render
Copy link

@blasten blasten Nov 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine, but I wonder why this is even allowed. The shell setup should ensure that things are wired up on the platform thread before the raster thread starts consuming tasks. Do you know where this is breaking?

Copy link
Member Author

@gaaclarke gaaclarke Nov 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please see the attached issue. TLDR viewDidAppear starts the raster thread, viewDidLayoutSubviews sets up the platforms view controller. There is no guarantee that viewDidLayoutSubviews is executed before SubmitFrame. We do need a larger fix. I outlined our 3 options in the issue.

Copy link

@blasten blasten Nov 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. That makes sense. This has nothing to do with the setup in shell after looking at the code.

// momentarily. @cyanglaz has reported this code will get refactored.
// See also: https://github.com/flutter/flutter/issues/69449
frame->Submit();
return true;
}

// Any UIKit related code has to run on main thread.
// When on a non-main thread, we only allow the rest of the method to run if there is no
Expand Down Expand Up @@ -542,7 +554,8 @@
picture, //
joined_rect, //
current_platform_view_id, //
overlay_id //
overlay_id, //
flutter_view //
);
did_submit &= layer->did_submit_last_frame;
platform_view_layers[current_platform_view_id].push_back(layer);
Expand All @@ -555,7 +568,7 @@
// then it can be removed from the scene.
RemoveUnusedLayers();
// Organize the layers by their z indexes.
BringLayersIntoView(platform_view_layers);
BringLayersIntoView(platform_view_layers, flutter_view);
// Mark all layers as available, so they can be used in the next frame.
layer_pool_->RecycleLayers();
// Reset the composition order, so next frame starts empty.
Expand All @@ -570,9 +583,9 @@
return did_submit;
}

void FlutterPlatformViewsController::BringLayersIntoView(LayersMap layer_map) {
FML_DCHECK(flutter_view_);
UIView* flutter_view = flutter_view_.get();
void FlutterPlatformViewsController::BringLayersIntoView(LayersMap layer_map,
UIView* flutter_view) {
FML_DCHECK(flutter_view);
auto zIndex = 0;
// Clear the `active_composition_order_`, which will be populated down below.
active_composition_order_.clear();
Expand Down Expand Up @@ -607,7 +620,8 @@
sk_sp<SkPicture> picture,
SkRect rect,
int64_t view_id,
int64_t overlay_id) {
int64_t overlay_id,
UIView* flutter_view) {
FML_DCHECK(flutter_view_);
std::shared_ptr<FlutterPlatformViewLayer> layer = layer_pool_->GetLayer(gr_context, ios_context);

Expand All @@ -624,7 +638,7 @@
UIView* overlay_view = layer->overlay_view.get();
// Set the size of the overlay view.
// This size is equal to the the device screen size.
overlay_view.frame = flutter_view_.get().bounds;
overlay_view.frame = flutter_view.bounds;

std::unique_ptr<SurfaceFrame> frame = layer->surface->AcquireFrame(frame_size_);
// If frame is null, AcquireFrame already printed out an error message.
Expand Down
19 changes: 11 additions & 8 deletions shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,18 @@
#ifndef FLUTTER_SHELL_PLATFORM_DARWIN_IOS_FRAMEWORK_SOURCE_FLUTTERPLATFORMVIEWS_INTERNAL_H_
#define FLUTTER_SHELL_PLATFORM_DARWIN_IOS_FRAMEWORK_SOURCE_FLUTTERPLATFORMVIEWS_INTERNAL_H_

#include "flutter/flow/embedded_views.h"
#include "flutter/flow/rtree.h"
#include "flutter/fml/platform/darwin/scoped_nsobject.h"
#include "flutter/shell/common/shell.h"
#import <atomic>

#import "flutter/flow/embedded_views.h"
#import "flutter/flow/rtree.h"
#import "flutter/fml/platform/darwin/scoped_nsobject.h"
#import "flutter/shell/common/shell.h"
#import "flutter/shell/platform/darwin/common/framework/Headers/FlutterBinaryMessenger.h"
#import "flutter/shell/platform/darwin/common/framework/Headers/FlutterChannels.h"
#import "flutter/shell/platform/darwin/ios/framework/Headers/FlutterPlatformViews.h"
#import "flutter/shell/platform/darwin/ios/framework/Headers/FlutterPlugin.h"
#import "flutter/shell/platform/darwin/ios/ios_context.h"
#include "third_party/skia/include/core/SkPictureRecorder.h"
#import "third_party/skia/include/core/SkPictureRecorder.h"

@class FlutterTouchInterceptingView;

Expand Down Expand Up @@ -205,7 +207,7 @@ class FlutterPlatformViewsController {
std::map<int64_t, std::unique_ptr<SkPictureRecorder>> picture_recorders_;

fml::scoped_nsobject<FlutterMethodChannel> channel_;
fml::scoped_nsobject<UIView> flutter_view_;
std::atomic<UIView*> flutter_view_ = nil;
fml::scoped_nsobject<UIViewController> flutter_view_controller_;
std::map<std::string, fml::scoped_nsobject<NSObject<FlutterPlatformViewFactory>>> factories_;
std::map<int64_t, fml::scoped_nsobject<NSObject<FlutterPlatformView>>> views_;
Expand Down Expand Up @@ -293,13 +295,14 @@ class FlutterPlatformViewsController {
sk_sp<SkPicture> picture,
SkRect rect,
int64_t view_id,
int64_t overlay_id);
int64_t overlay_id,
UIView* flutter_view);
// Removes overlay views and platform views that aren't needed in the current frame.
// Must run on the platform thread.
void RemoveUnusedLayers();
// Appends the overlay views and platform view and sets their z index based on the composition
// order.
void BringLayersIntoView(LayersMap layer_map);
void BringLayersIntoView(LayersMap layer_map, UIView* flutter_view);

// Begin a CATransaction.
// This transaction needs to be balanced with |CommitCATransactionIfNeeded|.
Expand Down
4 changes: 3 additions & 1 deletion shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@
: layer_pool_(std::make_unique<FlutterPlatformViewLayerPool>(surface_factory)),
weak_factory_(std::make_unique<fml::WeakPtrFactory<FlutterPlatformViewsController>>(this)){};

FlutterPlatformViewsController::~FlutterPlatformViewsController() = default;
FlutterPlatformViewsController::~FlutterPlatformViewsController() {
[flutter_view_.load() release];
Copy link

@blasten blasten Nov 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah. good catch.

Copy link
Member Author

@gaaclarke gaaclarke Nov 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously the object was contained by a smart pointer, this wasn't needed. Now that we need to make it atomic I had to add the explicit release.

}

fml::WeakPtr<flutter::FlutterPlatformViewsController> FlutterPlatformViewsController::GetWeakPtr() {
return weak_factory_->GetWeakPtr();
Expand Down