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

Flush all embedded Android views on hot restart. #5929

Merged
amirh merged 3 commits into from
Aug 3, 2018
Merged

Flush all embedded Android views on hot restart. #5929

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
42804e2
Flush all embedded Android view on hot restart.
amirh Aug 2, 2018
63891d1
review comments followup
amirh Aug 2, 2018
1585169
rename to OnPreEngineRestart, call before Clone
amirh Aug 2, 2018
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
1 change: 1 addition & 0 deletions shell/common/engine.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ bool Engine::Restart(RunConfiguration configuration) {
}
runtime_controller_ = runtime_controller_->Clone();
UpdateAssetManager(nullptr);
delegate_.OnEngineRestart();

@chinmaygarde chinmaygarde Aug 2, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You need to check if the run call actually succeeded before firing the delegate method. As written, failed attempts to relaunch the engine will still fire the delegate callback.

@amirh amirh Aug 2, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That might be a problem - I need to make sure I flushed the embedded views before the dart code starts...
What happens when a hot restart fails? do we retry? what if all retries failed?

@chinmaygarde chinmaygarde Aug 2, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When a hot restart fails, we just sit there waiting for another restart (all restarts are initiated by the service protocol).

If you really want a callback just before the restart, I would name this delegate method OnEngineWillRestart to better indicate that the restart has not yet happened. Also, move this delegate call up before the runtime_controller_->Clone() call. On line 106, the old runtime controller has already been destroyed.

@amirh amirh Aug 2, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Renamed to OnPreEngineRestart

return Run(std::move(configuration));
}

Expand Down
2 changes: 2 additions & 0 deletions shell/common/engine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ class Engine final : public blink::RuntimeDelegate {
virtual void OnEngineHandlePlatformMessage(
const Engine& engine,
fml::RefPtr<blink::PlatformMessage> message) = 0;

virtual void OnEngineRestart() = 0;
};

Engine(Delegate& delegate,
Expand Down
2 changes: 2 additions & 0 deletions shell/common/platform_view.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ void PlatformView::HandlePlatformMessage(
response->CompleteEmpty();
}

void PlatformView::OnEngineRestart() const {}

void PlatformView::RegisterTexture(std::shared_ptr<flow::Texture> texture) {
delegate_.OnPlatformViewRegisterTexture(*this, std::move(texture));
}
Expand Down
2 changes: 2 additions & 0 deletions shell/common/platform_view.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,8 @@ class PlatformView {
virtual void HandlePlatformMessage(
fml::RefPtr<blink::PlatformMessage> message);

virtual void OnEngineRestart() const;

void SetNextFrameCallback(fml::closure closure);

void DispatchPointerDataPacket(
Expand Down
15 changes: 15 additions & 0 deletions shell/common/shell.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -740,6 +740,21 @@ void Shell::OnEngineHandlePlatformMessage(
});
}

// |shell::Engine::Delegate|
void Shell::OnEngineRestart() {

@chinmaygarde chinmaygarde Aug 2, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This will deadlock on configurations where the task runners are the same (test/embedder configurations). Please use fml::TaskRunner::RunNowOrPostTask instead.

@chinmaygarde chinmaygarde Aug 2, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add the assertions for setup and the thread checker as done in all the other methods in this file.

@amirh amirh Aug 2, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Wow good catch on the threading issue! thanks!

@amirh amirh Aug 2, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added the same assertions, though I believe we don't really care to assert that we're on the ui thread

@chinmaygarde chinmaygarde Aug 2, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, we are just being super paranoid about threading here. Also, I like the assertion as a nice way of quickly figuring out who initiates the restart call. For example, it is possible that the service protocol handler fires off this delegate method. But now that you added the assertion, I can easily tell that the engine has had a chance to service the call.

fml::AutoResetWaitableEvent latch;
task_runners_.GetPlatformTaskRunner()->PostTask(
[view = platform_view_->GetWeakPtr(), &latch] {
if (view) {
view->OnEngineRestart();
}
latch.Signal();
});
// This is blocking as any embedded platform views has to be flushed before
// we re-run the Dart code.
latch.Wait();
}

// |blink::ServiceProtocol::Handler|
fml::RefPtr<fml::TaskRunner> Shell::GetServiceProtocolHandlerTaskRunner(
fml::StringView method) const {
Expand Down
3 changes: 3 additions & 0 deletions shell/common/shell.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,9 @@ class Shell final : public PlatformView::Delegate,
const Engine& engine,
fml::RefPtr<blink::PlatformMessage> message) override;

// |shell::Engine::Delegate|
void OnEngineRestart() override;

// |blink::ServiceProtocol::Handler|
fml::RefPtr<fml::TaskRunner> GetServiceProtocolHandlerTaskRunner(
fml::StringView method) const override;
Expand Down
4 changes: 4 additions & 0 deletions shell/platform/android/io/flutter/app/FlutterPluginRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,10 @@ public void detach() {
mActivity = null;
}

public void onEngineRestart() {
mPlatformViewsController.onEngineRestart();
}

private class FlutterRegistrar implements Registrar {
private final String pluginKey;

Expand Down
15 changes: 11 additions & 4 deletions shell/platform/android/io/flutter/plugin/platform/PlatformViewsController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,11 @@ public PlatformViewRegistry getRegistry() {
}

public void onFlutterViewDestroyed() {
for (VirtualDisplayController controller : vdControllers.values()) {
controller.dispose();
}
vdControllers.clear();
flushAllViews();
}

public void onEngineRestart() {
flushAllViews();
}

@Override
Expand Down Expand Up @@ -297,4 +298,10 @@ private int toPhysicalPixels(double logicalPixels) {
return (int) Math.round(logicalPixels * density);
}

private void flushAllViews() {
for (VirtualDisplayController controller : vdControllers.values()) {
controller.dispose();
}
vdControllers.clear();
}
}
8 changes: 8 additions & 0 deletions shell/platform/android/io/flutter/view/FlutterNativeView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,14 @@ private void onFirstFrame() {
mFlutterView.onFirstFrame();
}

// Called by native to notify when the engine is restarted (cold reload).
@SuppressWarnings("unused")
private void onEngineRestart() {
if (mPluginRegistry == null)
return;
mPluginRegistry.onEngineRestart();
}

private static native long nativeAttach(FlutterNativeView view);
private static native void nativeDestroy(long nativePlatformViewAndroid);
private static native void nativeDetach(long nativePlatformViewAndroid);
Expand Down
11 changes: 11 additions & 0 deletions shell/platform/android/platform_view_android.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,17 @@ void PlatformViewAndroid::HandlePlatformMessage(
}
}

// |shell::PlatformView|
void PlatformViewAndroid::OnEngineRestart() const {
JNIEnv* env = fml::jni::AttachCurrentThread();
fml::jni::ScopedJavaLocalRef<jobject> view = java_object_.get(env);
if (view.is_null()) {
// The Java object died.
return;
}
FlutterViewOnEngineRestart(fml::jni::AttachCurrentThread(), view.obj());
}

void PlatformViewAndroid::DispatchSemanticsAction(JNIEnv* env,
jint id,
jint action,
Expand Down
3 changes: 3 additions & 0 deletions shell/platform/android/platform_view_android.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,9 @@ class PlatformViewAndroid final : public PlatformView {
void HandlePlatformMessage(
fml::RefPtr<blink::PlatformMessage> message) override;

// |shell::PlatformView|
void OnEngineRestart() const override;

// |shell::PlatformView|
std::unique_ptr<VsyncWaiter> CreateVSyncWaiter() override;

Expand Down
13 changes: 13 additions & 0 deletions shell/platform/android/platform_view_android_jni.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,12 @@ void FlutterViewOnFirstFrame(JNIEnv* env, jobject obj) {
FML_CHECK(CheckException(env));
}

static jmethodID g_on_engine_restart_method = nullptr;
void FlutterViewOnEngineRestart(JNIEnv* env, jobject obj) {
env->CallVoidMethod(obj, g_on_engine_restart_method);
FML_CHECK(CheckException(env));
}

static jmethodID g_attach_to_gl_context_method = nullptr;
void SurfaceTextureAttachToGLContext(JNIEnv* env, jobject obj, jint textureId) {
env->CallVoidMethod(obj, g_attach_to_gl_context_method, textureId);
Expand Down Expand Up @@ -698,6 +704,13 @@ bool PlatformViewAndroid::Register(JNIEnv* env) {
return false;
}

g_on_engine_restart_method = env->GetMethodID(
g_flutter_native_view_class->obj(), "onEngineRestart", "()V");

if (g_on_engine_restart_method == nullptr) {
return false;
}

g_attach_to_gl_context_method = env->GetMethodID(
g_surface_texture_class->obj(), "attachToGLContext", "(I)V");

Expand Down
2 changes: 2 additions & 0 deletions shell/platform/android/platform_view_android_jni.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ void FlutterViewUpdateCustomAccessibilityActions(JNIEnv* env,

void FlutterViewOnFirstFrame(JNIEnv* env, jobject obj);

void FlutterViewOnEngineRestart(JNIEnv* env, jobject obj);

void SurfaceTextureAttachToGLContext(JNIEnv* env, jobject obj, jint textureId);

void SurfaceTextureUpdateTexImage(JNIEnv* env, jobject obj);
Expand Down