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

Avoid capturing this unsafely in MultiFrameCodec #16824

Merged
merged 11 commits into from
Mar 10, 2020
Merged

Avoid capturing this unsafely in MultiFrameCodec #16824

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
dd4cbc3
test
dnfield Feb 26, 2020
46af095
avoid unsafe capture of this in MultiFrameCodec
dnfield Feb 27, 2020
c81cfd2
gn format
dnfield Feb 27, 2020
1b81acc
Merge remote-tracking branch 'upstream/master' into multi_frame_this
dnfield Mar 4, 2020
f3e51b9
missing config
dnfield Mar 4, 2020
4bff010
docs
dnfield Mar 4, 2020
58205cc
Merge remote-tracking branch 'upstream/master' into multi_frame_this
dnfield Mar 4, 2020
7403784
Merge remote-tracking branch 'upstream/master' into multi_frame_this
dnfield Mar 6, 2020
f5bbcdd
Review
dnfield Mar 6, 2020
7438df1
move to own TU
dnfield Mar 6, 2020
7cac8dc
licenses
dnfield Mar 10, 2020
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
6 changes: 6 additions & 0 deletions lib/ui/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,14 @@ if (current_toolchain == host_toolchain) {
":ui",
":ui_unittests_fixtures",
"//flutter/common",
"//flutter/fml",
"//flutter/lib/snapshot",
"//flutter/runtime",
"//flutter/shell/common",
"//flutter/testing:dart",
"//flutter/testing:opengl",
"//flutter/third_party/tonic",
"//third_party/dart/runtime/bin:elf_loader",
]
}
}
7 changes: 7 additions & 0 deletions lib/ui/fixtures/ui_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,11 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'dart:ui';

void main() {}

@pragma('vm:entry-point')
void frameCallback(FrameInfo info) {
print('called back');
}
139 changes: 138 additions & 1 deletion lib/ui/painting/image_decoder_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,12 @@
#include "flutter/fml/mapping.h"
#include "flutter/fml/synchronization/waitable_event.h"
#include "flutter/lib/ui/painting/image_decoder.h"
#include "flutter/lib/ui/painting/multi_frame_codec.h"
#include "flutter/runtime/dart_vm.h"
#include "flutter/runtime/dart_vm_lifecycle.h"
#include "flutter/testing/dart_isolate_runner.h"
#include "flutter/testing/elf_loader.h"
#include "flutter/testing/test_dart_native_resolver.h"
#include "flutter/testing/test_gl_surface.h"
#include "flutter/testing/testing.h"
#include "flutter/testing/thread_test.h"
Expand All @@ -14,7 +20,56 @@
namespace flutter {
namespace testing {

using ImageDecoderFixtureTest = ThreadTest;
class ImageDecoderFixtureTest : public ThreadTest {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Consider putting this in its own TU.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

public:
ImageDecoderFixtureTest()
: native_resolver_(std::make_shared<TestDartNativeResolver>()),
assets_dir_(fml::OpenDirectory(GetFixturesPath(),
false,
fml::FilePermission::kRead)),
aot_symbols_(LoadELFSymbolFromFixturesIfNeccessary()) {}

Settings CreateSettingsForFixture() {
Settings settings;
settings.leak_vm = false;
settings.task_observer_add = [](intptr_t, fml::closure) {};
settings.task_observer_remove = [](intptr_t) {};
settings.isolate_create_callback = [this]() {
native_resolver_->SetNativeResolverForIsolate();
};
SetSnapshotsAndAssets(settings);
return settings;
}

private:
std::shared_ptr<TestDartNativeResolver> native_resolver_;
fml::UniqueFD assets_dir_;
ELFAOTSymbols aot_symbols_;

void SetSnapshotsAndAssets(Settings& settings) {
if (!assets_dir_.is_valid()) {
return;
}

settings.assets_dir = assets_dir_.get();

// In JIT execution, all snapshots are present within the binary itself and
// don't need to be explicitly supplied by the embedder. In AOT, these
// snapshots will be present in the application AOT dylib.
if (DartVM::IsRunningPrecompiledCode()) {
PrepareSettingsForAOTWithSymbols(settings, aot_symbols_);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: FML_CHECK the result here. We don't want to fail later if for some reason we mess up symbol packaging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

} else {
settings.application_kernels = [this]() {
std::vector<std::unique_ptr<const fml::Mapping>> kernel_mappings;
kernel_mappings.emplace_back(
fml::FileMapping::CreateReadOnly(assets_dir_, "kernel_blob.bin"));
return kernel_mappings;
};
}
}

FML_DISALLOW_COPY_AND_ASSIGN(ImageDecoderFixtureTest);
};

class TestIOManager final : public IOManager {
public:
Expand Down Expand Up @@ -557,5 +612,87 @@ TEST(ImageDecoderTest, VerifySubpixelDecodingPreservesExifOrientation) {
assert_image(decode({}, 100));
}

TEST_F(ImageDecoderFixtureTest,
Copy link
Member

Choose a reason for hiding this comment

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

This test is pretty big, it can't be groked with a passing perusal. I recommend adding a docstring at the beginning of the test, or pulling out some helper functions so that the gist can be parsed quicker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment to the top. I'm not quite sure how to refactor this into smaller functions while still preserving the ordering that's needed, but maybe when I get in we can talk about it some more offline.

MultiFrameCodecCanBeCollectedBeforeIOTasksFinish) {
auto settings = CreateSettingsForFixture();
settings.leak_vm = false;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Maybe configure these in CreateSettingsForFixture?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

settings.enable_observatory = false;
auto vm_ref = DartVMRef::Create(settings);
auto vm_data = vm_ref.GetVMData();

auto gif_mapping = OpenFixtureAsSkData("hello_loop_2.gif");

ASSERT_TRUE(gif_mapping);

auto gif_codec = SkCodec::MakeFromData(gif_mapping);
ASSERT_TRUE(gif_codec);

auto loop = fml::ConcurrentMessageLoop::Create();
Copy link
Member

Choose a reason for hiding this comment

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

This isn't being used anywhere. Also, one will be created for you when you instantiate the VMRef. Just getting rid of it is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed - this was copypasted from another test and I missed dropping it during refactoring.

TaskRunners runners(GetCurrentTestName(), // label
CreateNewThread("platform"), // platform
CreateNewThread("gpu"), // gpu
CreateNewThread("ui"), // ui
CreateNewThread("io") // io
);

fml::AutoResetWaitableEvent latch;
fml::AutoResetWaitableEvent io_latch;
std::unique_ptr<TestIOManager> io_manager;

// Setup the IO manager.
runners.GetIOTaskRunner()->PostTask([&]() {
io_manager = std::make_unique<TestIOManager>(runners.GetIOTaskRunner());
latch.Signal();
});
latch.Wait();

auto isolate =
RunDartCodeInIsolate(vm_ref, settings, runners, "main", {},
GetFixturesPath(), io_manager->GetWeakIOManager());

// Latch the IO task runner.
runners.GetIOTaskRunner()->PostTask([&]() { io_latch.Wait(); });

runners.GetUITaskRunner()->PostTask([&]() {
fml::AutoResetWaitableEvent isolate_latch;
fml::RefPtr<MultiFrameCodec> codec;
EXPECT_TRUE(isolate->RunInIsolateScope([&]() -> bool {
Dart_Handle library = Dart_LookupLibrary(Dart_NewStringFromCString(
"package:engine_root/ui/fixtures/ui_test.dart"));
Copy link
Member

Choose a reason for hiding this comment

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

This makes the test depend on the path of the fixture which would be brittle. Doesn't Dart_RootLibrary achieve the same thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does, I didn't know about Dart_RootLibrary. Done.

if (Dart_IsError(library)) {
isolate_latch.Signal();
return false;
}
Dart_Handle closure =
Dart_GetField(library, Dart_NewStringFromCString("frameCallback"));
if (Dart_IsError(closure) || !Dart_IsClosure(closure)) {
isolate_latch.Signal();
return false;
}

codec = fml::MakeRefCounted<MultiFrameCodec>(std::move(gif_codec));
codec->getNextFrame(closure);
codec = nullptr;
isolate_latch.Signal();
return true;
}));
isolate_latch.Wait();

EXPECT_FALSE(codec);

io_latch.Signal();

latch.Signal();
});
latch.Wait();

// Destroy the IO manager
runners.GetIOTaskRunner()->PostTask([&]() {
Copy link
Member

Choose a reason for hiding this comment

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

There is alot of work here to serialize async operations with latches. It would be easier to follow if you made a PerformSyncTask function that takes in a task runner and a lambda and doesn't complete until the task runner finishes the lambda.

I've tried to add this in the past and have met resistance, maybe for tests at least people will be accepting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is idiomatic code for how the rest of the engine repo does it. I'd be hesitant to add a new test-only class for this that itself needs to be tested...

Copy link
Member

Choose a reason for hiding this comment

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

Boilerplate that detracts from the point shouldn't be encouraged. I wasn't think a new class I was thinking this:

void PerformSyncTask(TaskRunner runner, Function func) {
  Latch latch;
  runner->PerformTask({
    func();
    latch.signal();
  });
  latch.wait();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The synchronization work here is also highly constructed to simulate collecting the engine while IO work is ongoing in a repeatable, non-flaky way. I'm not sure it would help to have something like PerformSyncTask, since I need to specifically interleave certain actions. I hope the comment I added at the top clarifies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On this specific test, I already have the latch and would need to have it anyway. The added function would only save a line or two of code here :\

Copy link
Member

Choose a reason for hiding this comment

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

The same thing could be said of for-loops versus foreach-loops. It isn't about LOC, its about avoiding errors, extra variables, and having a meaningful abstraction with which to build off of.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - and I think such a decision on style is something we should follow consistently in the repo.

I'm not opposed to making this change so much as opposed to doing it differently just here, or even just in this test file, compared to the way we do it everywhere else. It'd be easier to just refactor all the callsites of this if they're all pretty similar, rather than doing it specially here.

io_manager.reset();
latch.Signal();
});
latch.Wait();
}

} // namespace testing
} // namespace flutter
40 changes: 27 additions & 13 deletions lib/ui/painting/multi_frame_codec.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,18 @@
namespace flutter {

MultiFrameCodec::MultiFrameCodec(std::unique_ptr<SkCodec> codec)
: state_(new State(std::move(codec))) {}

MultiFrameCodec::~MultiFrameCodec() {
state_->live_ = false;
}

MultiFrameCodec::State::State(std::unique_ptr<SkCodec> codec)
: codec_(std::move(codec)),
frameCount_(codec_->getFrameCount()),
repetitionCount_(codec_->getRepetitionCount()),
nextFrameIndex_(0) {}

MultiFrameCodec::~MultiFrameCodec() = default;
nextFrameIndex_(0),
live_(true) {}

static void InvokeNextFrameCallback(
fml::RefPtr<FrameInfo> frameInfo,
Expand Down Expand Up @@ -70,7 +76,7 @@ static bool CopyToBitmap(SkBitmap* dst,
return true;
}

sk_sp<SkImage> MultiFrameCodec::GetNextFrameImage(
sk_sp<SkImage> MultiFrameCodec::State::GetNextFrameImage(
fml::WeakPtr<GrContext> resourceContext) {
SkBitmap bitmap = SkBitmap();
SkImageInfo info = codec_->getInfo().makeColorType(kN32_SkColorType);
Expand Down Expand Up @@ -128,13 +134,20 @@ sk_sp<SkImage> MultiFrameCodec::GetNextFrameImage(
}
}

void MultiFrameCodec::GetNextFrameAndInvokeCallback(
void MultiFrameCodec::State::GetNextFrameAndInvokeCallback(
std::unique_ptr<DartPersistentValue> callback,
fml::RefPtr<fml::TaskRunner> ui_task_runner,
fml::WeakPtr<GrContext> resourceContext,
fml::RefPtr<flutter::SkiaUnrefQueue> unref_queue,
size_t trace_id) {
fml::RefPtr<FrameInfo> frameInfo = NULL;
if (!live_) {
ui_task_runner->PostTask(fml::MakeCopyable(
[callback = std::move(callback), frameInfo, trace_id]() mutable {
InvokeNextFrameCallback(frameInfo, std::move(callback), trace_id);
}));
return;
}
sk_sp<SkImage> skImage = GetNextFrameImage(resourceContext);
if (skImage) {
fml::RefPtr<CanvasImage> image = CanvasImage::Create();
Expand Down Expand Up @@ -164,12 +177,13 @@ Dart_Handle MultiFrameCodec::getNextFrame(Dart_Handle callback_handle) {

const auto& task_runners = dart_state->GetTaskRunners();

task_runners.GetIOTaskRunner()->PostTask(fml::MakeCopyable(
[callback = std::make_unique<DartPersistentValue>(
tonic::DartState::Current(), callback_handle),
this, trace_id, ui_task_runner = task_runners.GetUITaskRunner(),
io_manager = dart_state->GetIOManager()]() mutable {
GetNextFrameAndInvokeCallback(
Copy link
Contributor

Choose a reason for hiding this comment

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

I've seen this misuse of this in other places. The pattern to fix it in most places seems to be sending in an fml::WeakPtr, and check its validity before dereference so another thread can safely destruct the object. I wonder if there's any particular reason why we can't use fml::WeakPtr here, and have to create a new State object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't use a WeakPtr here because we have to access the object across threads, and WeakPtr is not thread safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And we can't create the weak ptr on the IO task runner because our test case would fail - e.g. if the IO runner is blocked, and the object gets collected before the task to create the weak ptr runs

Copy link
Member

Choose a reason for hiding this comment

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

@liyuqian: While weak pointers can be copied safely across threads, they may only be used on the thread on which their factories live. And weak pointer factories can only be created and collected on a single thread. In this case, this is the UI thread. Your intuition is absolutely correct but only in cases where the object will only be used on the one thread. This is not one of those cases.

task_runners.GetIOTaskRunner()->PostTask(
fml::MakeCopyable([callback = std::make_unique<DartPersistentValue>(
tonic::DartState::Current(), callback_handle),
state = state_, trace_id,
ui_task_runner = task_runners.GetUITaskRunner(),
io_manager = dart_state->GetIOManager()]() mutable {
state->GetNextFrameAndInvokeCallback(
Copy link
Member

Choose a reason for hiding this comment

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

The live_ variable is unnecessary. You can copy state via a weak_ptr instead of a shared_ptr. Here, you can attempt a lock() and if that fails, you can do what you are doing in line 144.

Copy link
Member

Choose a reason for hiding this comment

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

you can do what you are doing in line 144.

Wait. Why are we even invoking the next frame callback on a muli-frame codec that has died (which would be indicated by the weak pointer lock failing)?

Instead of invoking the next frame callback when the weak pointer lock fails, scheduled a task onto the UI thread that just releases the callback but not invoke it (that has Dart state and you cant just drop it on the IO thread).

Copy link
Member

Choose a reason for hiding this comment

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

As an aside, variable that check for "live"-ness are usually always an anti-pattern as the information can be invalid immediately after the check. There are valid use cases like double checked locks of course.

In this case, it seems it was an optimization I suppose. But again, the same can be achieved using weak_ptrs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not clear on where it's safe to create the weak_ptr for the State. I can't do it on the UI thread, and by the time I get to the IO thread the UI object might have been collected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohhh you mean a std::weak_ptr. I get it now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason I was thinking we had to still make the callback because some Dart code might be awaiting the completion of the callback, but now that I think about it that doesn't make sense, since the only way this can happen is if the engine is collected. If anyone is actually awaiting the callback, the codec must still be alive. I'm correct in that thinking, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done - I believe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only question is whether we need to guard a bit more so that if we get collected after this (e.g. we're still alive before we tell Skia to decode a frame, but we're dead after Skia finishes) - will this still be safe?

Copy link
Member

Choose a reason for hiding this comment

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

The lock will fail in that case.

std::move(callback), std::move(ui_task_runner),
io_manager->GetResourceContext(), io_manager->GetSkiaUnrefQueue(),
trace_id);
Expand All @@ -179,11 +193,11 @@ Dart_Handle MultiFrameCodec::getNextFrame(Dart_Handle callback_handle) {
}

int MultiFrameCodec::frameCount() const {
return frameCount_;
return state_->frameCount_;
}

int MultiFrameCodec::repetitionCount() const {
return repetitionCount_;
return state_->repetitionCount_;
}

} // namespace flutter
43 changes: 25 additions & 18 deletions lib/ui/painting/multi_frame_codec.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,24 +26,31 @@ class MultiFrameCodec : public Codec {
Dart_Handle getNextFrame(Dart_Handle args) override;

private:
const std::unique_ptr<SkCodec> codec_;
const int frameCount_;
const int repetitionCount_;
int nextFrameIndex_;

// The last decoded frame that's required to decode any subsequent frames.
std::unique_ptr<SkBitmap> lastRequiredFrame_;
// The index of the last decoded required frame.
int lastRequiredFrameIndex_ = -1;

sk_sp<SkImage> GetNextFrameImage(fml::WeakPtr<GrContext> resourceContext);

void GetNextFrameAndInvokeCallback(
std::unique_ptr<DartPersistentValue> callback,
fml::RefPtr<fml::TaskRunner> ui_task_runner,
fml::WeakPtr<GrContext> resourceContext,
fml::RefPtr<flutter::SkiaUnrefQueue> unref_queue,
size_t trace_id);
struct State {
Copy link
Member

Choose a reason for hiding this comment

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

class docstring

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. It's probably not in proper doxygen format, but this is a private class anyway.

State(std::unique_ptr<SkCodec> codec);

const std::unique_ptr<SkCodec> codec_;
const int frameCount_;
const int repetitionCount_;
int nextFrameIndex_;
Copy link
Member

Choose a reason for hiding this comment

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

While using shared pointers solves the lifecycle issues of State, it does not prevent data races due to concurrent access of state fields. Thats why non-const members of such objects are usually guarded by a mutex. In this case, auditing all uses seems to indicate that that all non-const members (except live_) are only read and updated on the IO thread. So it's safe but not obvious how.

Can we group these non const members together with a comment that makes it clear that these will only be read and updated on the IO thread? If we want to be paranoid, we could use a mutex but that is probably not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I'd like to avoid the mutex since this is a potentially performance critical path, e.g. for an animated image firing on every vsync.

std::atomic<bool> live_;

// The last decoded frame that's required to decode any subsequent frames.
std::unique_ptr<SkBitmap> lastRequiredFrame_;
// The index of the last decoded required frame.
int lastRequiredFrameIndex_ = -1;

sk_sp<SkImage> GetNextFrameImage(fml::WeakPtr<GrContext> resourceContext);
void GetNextFrameAndInvokeCallback(
std::unique_ptr<DartPersistentValue> callback,
fml::RefPtr<fml::TaskRunner> ui_task_runner,
fml::WeakPtr<GrContext> resourceContext,
fml::RefPtr<flutter::SkiaUnrefQueue> unref_queue,
size_t trace_id);
};

// Shared across the UI and IO task runners.
std::shared_ptr<State> state_;

FML_FRIEND_MAKE_REF_COUNTED(MultiFrameCodec);
FML_FRIEND_REF_COUNTED_THREAD_SAFE(MultiFrameCodec);
Expand Down
Loading