From 2dc05aed94bd4273441f5706a610967b7a208260 Mon Sep 17 00:00:00 2001 From: Alexander Aprelev Date: Thu, 5 Apr 2018 09:10:00 -0700 Subject: [PATCH 1/4] Run setAssetBundlePath without UI thread. Running this on UI thread results in timeout if application is suspended on breakpoint and UI loop is not running. There is no reason to run setAssetBundlePath on UI thread as this request only updates internal asset location. --- shell/common/shell.cc | 28 +++------------------------- 1 file changed, 3 insertions(+), 25 deletions(-) diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 14f8fee84bb5f..83415ef5c28d8 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -303,30 +303,10 @@ void Shell::SetAssetBundlePathInPlatformView(uintptr_t view_id, bool* view_existed, int64_t* dart_isolate_id, std::string* isolate_name) { - fxl::AutoResetWaitableEvent latch; FXL_DCHECK(view_id != 0); FXL_DCHECK(asset_directory); FXL_DCHECK(view_existed); - blink::Threads::UI()->PostTask([this, view_id, asset_directory, view_existed, - dart_isolate_id, isolate_name, &latch]() { - SetAssetBundlePathInPlatformViewUIThread(view_id, asset_directory, - view_existed, dart_isolate_id, - isolate_name, &latch); - }); - latch.Wait(); -} - -void Shell::SetAssetBundlePathInPlatformViewUIThread( - uintptr_t view_id, - const std::string& assets_directory, - bool* view_existed, - int64_t* dart_isolate_id, - std::string* isolate_name, - fxl::AutoResetWaitableEvent* latch) { - FXL_DCHECK(ui_thread_checker_ && - ui_thread_checker_->IsCreationThreadCurrent()); - *view_existed = false; IteratePlatformViews( @@ -336,9 +316,9 @@ void Shell::SetAssetBundlePathInPlatformViewUIThread( // not supported on Windows for some reason. // TODO(https://github.com/flutter/flutter/issues/13908): // Investigate the root cause of the difference. - assets_directory = std::move(assets_directory), // argument + asset_directory = std::move(asset_directory), // argument #else - assets_directory, // argument + asset_directory, // argument #endif &view_existed, // out &dart_isolate_id, // out @@ -349,15 +329,13 @@ void Shell::SetAssetBundlePathInPlatformViewUIThread( return true; } *view_existed = true; - view->SetAssetBundlePath(assets_directory); + view->SetAssetBundlePath(asset_directory); *dart_isolate_id = view->engine().GetUIIsolateMainPort(); *isolate_name = view->engine().GetUIIsolateName(); // We found the requested view. Stop iterating over // platform views. return false; }); - - latch->Signal(); } } // namespace shell From bd9a4559d2d5b32a92592dcb7c0da8c187c3038f Mon Sep 17 00:00:00 2001 From: Alexander Aprelev Date: Thu, 5 Apr 2018 11:06:57 -0700 Subject: [PATCH 2/4] Remove rest of SetAssetBundlePathOnUI. Refactor multiple implementations of SetAssetBundlePath into single one. --- shell/common/platform_view.cc | 10 +++++ shell/common/platform_view.h | 2 +- .../common/platform_view_service_protocol.cc | 1 - shell/common/shell.h | 8 ---- .../io/flutter/view/FlutterNativeView.java | 6 +-- .../android/io/flutter/view/FlutterView.java | 2 +- .../platform/android/platform_view_android.cc | 42 ------------------- .../platform/android/platform_view_android.h | 4 -- .../android/platform_view_android_jni.cc | 15 +++---- .../darwin/desktop/platform_view_mac.h | 4 -- .../darwin/desktop/platform_view_mac.mm | 19 --------- shell/platform/darwin/ios/platform_view_ios.h | 4 -- .../platform/darwin/ios/platform_view_ios.mm | 19 --------- 13 files changed, 23 insertions(+), 113 deletions(-) diff --git a/shell/common/platform_view.cc b/shell/common/platform_view.cc index 2d5715a1733bd..72410c3792d28 100644 --- a/shell/common/platform_view.cc +++ b/shell/common/platform_view.cc @@ -202,4 +202,14 @@ void PlatformView::SetupResourceContextOnIOThreadPerform( latch->Signal(); } +void PlatformView::SetAssetBundlePath(const std::string& assets_directory) { + // Don't wait for this task to complete as UI thread can be blocked with + // application suspended at a breakpoint. + blink::Threads::UI()->PostTask([engine = engine_->GetWeakPtr(), + assets_directory] { + if (engine) + engine->SetAssetBundlePath(assets_directory); + }); +} + } // namespace shell diff --git a/shell/common/platform_view.h b/shell/common/platform_view.h index 66b1fcae10c79..2971883f50889 100644 --- a/shell/common/platform_view.h +++ b/shell/common/platform_view.h @@ -82,7 +82,7 @@ class PlatformView : public std::enable_shared_from_this { const std::string& main, const std::string& packages) = 0; - virtual void SetAssetBundlePath(const std::string& assets_directory) = 0; + virtual void SetAssetBundlePath(const std::string& assets_directory); protected: explicit PlatformView(std::unique_ptr rasterizer); diff --git a/shell/common/platform_view_service_protocol.cc b/shell/common/platform_view_service_protocol.cc index 8785c4812e202..1e3bbb29ee8bd 100644 --- a/shell/common/platform_view_service_protocol.cc +++ b/shell/common/platform_view_service_protocol.cc @@ -420,7 +420,6 @@ bool PlatformViewServiceProtocol::SetAssetBundlePath(const char* method, std::stoull((view_id + kViewIdPrefxLength), nullptr, 16); // Ask the Shell to update asset bundle path in the specified view. - // This will run a task on the UI thread before returning. Shell& shell = Shell::Shared(); bool view_existed = false; Dart_Port main_port = ILLEGAL_PORT; diff --git a/shell/common/shell.h b/shell/common/shell.h index 92c315dcf2aa7..cbe4827c6f1f3 100644 --- a/shell/common/shell.h +++ b/shell/common/shell.h @@ -88,14 +88,6 @@ class Shell { std::string* isolate_name, fxl::AutoResetWaitableEvent* latch); - void SetAssetBundlePathInPlatformViewUIThread( - uintptr_t view_id, - const std::string& main, - bool* view_existed, - int64_t* dart_isolate_id, - std::string* isolate_name, - fxl::AutoResetWaitableEvent* latch); - FXL_DISALLOW_COPY_AND_ASSIGN(Shell); }; diff --git a/shell/platform/android/io/flutter/view/FlutterNativeView.java b/shell/platform/android/io/flutter/view/FlutterNativeView.java index 034a86d09d91f..de36ff2f2e624 100644 --- a/shell/platform/android/io/flutter/view/FlutterNativeView.java +++ b/shell/platform/android/io/flutter/view/FlutterNativeView.java @@ -79,9 +79,9 @@ public void runFromSource(final String assetsDirectory, final String main, final nativeRunBundleAndSource(mNativePlatformView, assetsDirectory, main, packages); } - public void setAssetBundlePathOnUI(final String assetsDirectory) { + public void setAssetBundlePath(final String assetsDirectory) { assertAttached(); - nativeSetAssetBundlePathOnUI(mNativePlatformView, assetsDirectory); + nativeSetAssetBundlePath(mNativePlatformView, assetsDirectory); } public static String getObservatoryUri() { @@ -205,7 +205,7 @@ private static native void nativeRunBundleAndSource(long nativePlatformViewAndro String main, String packages); - private static native void nativeSetAssetBundlePathOnUI(long nativePlatformViewAndroid, + private static native void nativeSetAssetBundlePath(long nativePlatformViewAndroid, String bundlePath); private static native String nativeGetObservatoryUri(); diff --git a/shell/platform/android/io/flutter/view/FlutterView.java b/shell/platform/android/io/flutter/view/FlutterView.java index 6b8f6b47f6fd4..31ea48b7dd78f 100644 --- a/shell/platform/android/io/flutter/view/FlutterView.java +++ b/shell/platform/android/io/flutter/view/FlutterView.java @@ -659,7 +659,7 @@ private void setAssetBundlePath(final String assetsDirectory) { Runnable runnable = new Runnable() { public void run() { assertAttached(); - mNativeView.setAssetBundlePathOnUI(assetsDirectory); + mNativeView.setAssetBundlePath(assetsDirectory); synchronized (this) { notify(); } diff --git a/shell/platform/android/platform_view_android.cc b/shell/platform/android/platform_view_android.cc index 8f677045fb1f5..23e9792ca6863 100644 --- a/shell/platform/android/platform_view_android.cc +++ b/shell/platform/android/platform_view_android.cc @@ -250,14 +250,6 @@ void PlatformViewAndroid::RunBundleAndSource(std::string bundle_path, }); } -void PlatformViewAndroid::SetAssetBundlePathOnUI(std::string bundle_path) { - blink::Threads::UI()->PostTask( - [ engine = engine_->GetWeakPtr(), bundle_path = std::move(bundle_path) ] { - if (engine) - engine->SetAssetBundlePath(std::move(bundle_path)); - }); -} - void PlatformViewAndroid::SetViewportMetrics(jfloat device_pixel_ratio, jint physical_width, jint physical_height, @@ -599,40 +591,6 @@ void PlatformViewAndroid::RunFromSource(const std::string& assets_directory, fml::jni::DetachFromVM(); } -void PlatformViewAndroid::SetAssetBundlePath( - const std::string& assets_directory) { - JNIEnv* env = fml::jni::AttachCurrentThread(); - FXL_CHECK(env); - - { - fml::jni::ScopedJavaLocalRef local_flutter_view = - flutter_view_.get(env); - if (local_flutter_view.is_null()) { - // Collected. - return; - } - - // Grab the class of the flutter view. - jclass flutter_view_class = env->GetObjectClass(local_flutter_view.obj()); - FXL_CHECK(flutter_view_class); - - // Grab the setAssetBundlePath method id. - jmethodID method_id = env->GetMethodID( - flutter_view_class, "setAssetBundlePathOnUI", "(Ljava/lang/String;)V"); - FXL_CHECK(method_id); - - // Invoke setAssetBundlePath on the Android UI thread. - jstring java_assets_directory = env->NewStringUTF(assets_directory.c_str()); - FXL_CHECK(java_assets_directory); - - env->CallVoidMethod(local_flutter_view.obj(), method_id, - java_assets_directory); - } - - // Detaching from the VM deletes any stray local references. - fml::jni::DetachFromVM(); -} - void PlatformViewAndroid::RegisterExternalTexture( int64_t texture_id, const fml::jni::JavaObjectWeakGlobalRef& surface_texture) { diff --git a/shell/platform/android/platform_view_android.h b/shell/platform/android/platform_view_android.h index 4779ea16ab4a7..8abbe4754c193 100644 --- a/shell/platform/android/platform_view_android.h +++ b/shell/platform/android/platform_view_android.h @@ -108,10 +108,6 @@ class PlatformViewAndroid : public PlatformView { const std::string& main, const std::string& packages) override; - void SetAssetBundlePathOnUI(std::string bundle_path); - - void SetAssetBundlePath(const std::string& assets_directory) override; - void RegisterExternalTexture( int64_t texture_id, const fml::jni::JavaObjectWeakGlobalRef& surface_texture); diff --git a/shell/platform/android/platform_view_android_jni.cc b/shell/platform/android/platform_view_android_jni.cc index c819f3bfb5e41..520062031edc5 100644 --- a/shell/platform/android/platform_view_android_jni.cc +++ b/shell/platform/android/platform_view_android_jni.cc @@ -184,11 +184,12 @@ void RunBundleAndSource(JNIEnv* env, fml::jni::JavaStringToString(env, packages)); } -void SetAssetBundlePathOnUI(JNIEnv* env, - jobject jcaller, - jlong platform_view, - jstring bundlePath) { - return PLATFORM_VIEW->SetAssetBundlePathOnUI( +void SetAssetBundlePath(JNIEnv* env, + jobject jcaller, + jlong platform_view, + jstring bundlePath) { + FXL_LOG(ERROR) << "platform_view_android_jni.cc::SetAssetBundlePath is calling SetAssetBundlePath that should be platform_view_android.cc"; + return PLATFORM_VIEW->SetAssetBundlePath( fml::jni::JavaStringToString(env, bundlePath)); } @@ -364,9 +365,9 @@ bool PlatformViewAndroid::Register(JNIEnv* env) { .fnPtr = reinterpret_cast(&shell::RunBundleAndSource), }, { - .name = "nativeSetAssetBundlePathOnUI", + .name = "nativeSetAssetBundlePath", .signature = "(JLjava/lang/String;)V", - .fnPtr = reinterpret_cast(&shell::SetAssetBundlePathOnUI), + .fnPtr = reinterpret_cast(&shell::SetAssetBundlePath), }, { .name = "nativeDetach", diff --git a/shell/platform/darwin/desktop/platform_view_mac.h b/shell/platform/darwin/desktop/platform_view_mac.h index 501400b6803d4..737f04cd77316 100644 --- a/shell/platform/darwin/desktop/platform_view_mac.h +++ b/shell/platform/darwin/desktop/platform_view_mac.h @@ -41,8 +41,6 @@ class PlatformViewMac : public PlatformView, public GPUSurfaceGLDelegate { const std::string& main, const std::string& packages) override; - void SetAssetBundlePath(const std::string& assets_directory) override; - private: fml::scoped_nsobject opengl_view_; fml::scoped_nsobject resource_loading_context_; @@ -53,8 +51,6 @@ class PlatformViewMac : public PlatformView, public GPUSurfaceGLDelegate { const std::string& main, const std::string& packages); - void SetAssetBundlePathOnUI(const std::string& assets_directory); - FXL_DISALLOW_COPY_AND_ASSIGN(PlatformViewMac); }; diff --git a/shell/platform/darwin/desktop/platform_view_mac.mm b/shell/platform/darwin/desktop/platform_view_mac.mm index 42948386fbcd4..89a1b5c93aed6 100644 --- a/shell/platform/darwin/desktop/platform_view_mac.mm +++ b/shell/platform/darwin/desktop/platform_view_mac.mm @@ -73,13 +73,6 @@ }); } -void PlatformViewMac::SetAssetBundlePathOnUI(const std::string& assets_directory) { - blink::Threads::UI()->PostTask([ engine = engine().GetWeakPtr(), assets_directory ] { - if (engine) - engine->SetAssetBundlePath(assets_directory); - }); -} - intptr_t PlatformViewMac::GLContextFBO() const { // Default window bound framebuffer FBO 0. return 0; @@ -160,16 +153,4 @@ delete latch; } -void PlatformViewMac::SetAssetBundlePath(const std::string& assets_directory) { - auto latch = new fxl::ManualResetWaitableEvent(); - - dispatch_async(dispatch_get_main_queue(), ^{ - SetAssetBundlePathOnUI(assets_directory); - latch->Signal(); - }); - - latch->Wait(); - delete latch; -} - } // namespace shell diff --git a/shell/platform/darwin/ios/platform_view_ios.h b/shell/platform/darwin/ios/platform_view_ios.h index dab19a0f9ce1a..c6680323aca06 100644 --- a/shell/platform/darwin/ios/platform_view_ios.h +++ b/shell/platform/darwin/ios/platform_view_ios.h @@ -60,8 +60,6 @@ class PlatformViewIOS : public PlatformView { const std::string& main, const std::string& packages) override; - void SetAssetBundlePath(const std::string& assets_directory) override; - /** * Exposes the `FlutterTextInputPlugin` singleton for the * `AccessibilityBridge` to be able to interact with the text entry system. @@ -96,8 +94,6 @@ class PlatformViewIOS : public PlatformView { const std::string& main, const std::string& packages); - void SetAssetBundlePathOnUI(const std::string& assets_directory); - FXL_DISALLOW_COPY_AND_ASSIGN(PlatformViewIOS); }; diff --git a/shell/platform/darwin/ios/platform_view_ios.mm b/shell/platform/darwin/ios/platform_view_ios.mm index 19c5dd4e663a1..abb5760404163 100644 --- a/shell/platform/darwin/ios/platform_view_ios.mm +++ b/shell/platform/darwin/ios/platform_view_ios.mm @@ -69,13 +69,6 @@ }); } -void PlatformViewIOS::SetAssetBundlePathOnUI(const std::string& assets_directory) { - blink::Threads::UI()->PostTask([ engine = engine().GetWeakPtr(), assets_directory ] { - if (engine) - engine->SetAssetBundlePath(assets_directory); - }); -} - fml::WeakPtr PlatformViewIOS::GetWeakPtr() { return weak_factory_.GetWeakPtr(); } @@ -127,16 +120,4 @@ delete latch; } -void PlatformViewIOS::SetAssetBundlePath(const std::string& assets_directory) { - auto latch = new fxl::ManualResetWaitableEvent(); - - dispatch_async(dispatch_get_main_queue(), ^{ - SetAssetBundlePathOnUI(assets_directory); - latch->Signal(); - }); - - latch->Wait(); - delete latch; -} - } // namespace shell From 199cdfe3c9ef74adc65845f4966fd4d477544336 Mon Sep 17 00:00:00 2001 From: Alexander Aprelev Date: Thu, 5 Apr 2018 11:16:09 -0700 Subject: [PATCH 3/4] Remove debug FXL_LOG --- shell/platform/android/platform_view_android_jni.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/shell/platform/android/platform_view_android_jni.cc b/shell/platform/android/platform_view_android_jni.cc index 520062031edc5..a3de443d881da 100644 --- a/shell/platform/android/platform_view_android_jni.cc +++ b/shell/platform/android/platform_view_android_jni.cc @@ -188,7 +188,6 @@ void SetAssetBundlePath(JNIEnv* env, jobject jcaller, jlong platform_view, jstring bundlePath) { - FXL_LOG(ERROR) << "platform_view_android_jni.cc::SetAssetBundlePath is calling SetAssetBundlePath that should be platform_view_android.cc"; return PLATFORM_VIEW->SetAssetBundlePath( fml::jni::JavaStringToString(env, bundlePath)); } From 563e9e30b3498f4986da9d62155b067a8c790387 Mon Sep 17 00:00:00 2001 From: Alexander Aprelev Date: Thu, 5 Apr 2018 13:13:01 -0700 Subject: [PATCH 4/4] Add comments regarding UI thread posting. --- shell/common/platform_view.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/shell/common/platform_view.cc b/shell/common/platform_view.cc index 72410c3792d28..d1ec225330af4 100644 --- a/shell/common/platform_view.cc +++ b/shell/common/platform_view.cc @@ -203,6 +203,9 @@ void PlatformView::SetupResourceContextOnIOThreadPerform( } void PlatformView::SetAssetBundlePath(const std::string& assets_directory) { + // Since this code is executed as vm root service handler, it doesn't run on + // UI thread. Assets are consumed on UI thread, so this has to be scheduled + // there. // Don't wait for this task to complete as UI thread can be blocked with // application suspended at a breakpoint. blink::Threads::UI()->PostTask([engine = engine_->GetWeakPtr(),