Avoid to freeze the system if hot reloading during debug #3833

B3rn475 · 2017-06-27T23:47:27Z

Trying to hot reload during debug freezes the system, due to the fact that WaitForPlatformViewIds tries to execute code on the main thread while it is locked, this in turn freezes the Service Isolate making impossible to continue debug or try to solve the situation.

B3rn475 · 2017-06-27T23:48:45Z

@jason-simmons @eseidelGoogle

jason-simmons · 2017-06-28T00:05:44Z

I don't think this will be safe.

platform_views_ was only accessed on the UI thread due to the thread safety rules of ftl::WeakPtr
(https://fuchsia.googlesource.com/ftl/+/master/memory/weak_ptr.h)

In particular, the code in Shell::WaitForPlatformViewIds that checks whether the PlatformView is still alive and then calls into the view's engine must only run on the UI thread.

B3rn475 · 2017-06-28T00:44:05Z

The WeakPtr are already not used in the same thread of the WeakPtrFactory.

They are created and accessed in the same thread, but they are potentially invalidated in other threads.

engine/shell/platform/android/platform_view_android_jni.cc

Line 63 in c457ec6

return PLATFORM_VIEW->Detach();

What is synchronized on destruction is the Purge, but not the actual delete.

B3rn475 · 2017-06-28T02:49:23Z

May moving from ftl::WeakPtr to std::weak_ptr solve the situation?

jason-simmons · 2017-06-28T16:52:17Z

std::weak_ptr and std::shared_ptr have nicer thread safety properties than ftl::WeakPtr and might be worth trying.

You'll need to update all the supported platforms to hold the PlatformView in a shared_ptr instead of a raw pointer or unique_ptr. Also, if the end goal is to get information out of the Engine/RuntimeHolder on a thread other than the UI thread, then you'll need a safe way to do that. Engine is generally not safe to access outside the UI thread.

B3rn475 · 2017-06-29T01:21:38Z

I will update all the other platforms ASAP.

For the thread safety of the Engine access I think that it is safe:

We have a shared_ptr that keeps a platform_view alive.
The platform_view has a unique_ptr that keeps alive a Engine object, it cannot be released.
The Engine has a unique_ptr that keeps alive a RuntimeController object, it cannot be released.
The RuntimeController has a unique_ptr that keeps alive a DartController object, it cannot be released.
The DartController has a raw pointer that keeps alive a UIDartState object, it cannot be released.
The UIDartState has direct access to the Dart_Port and to the debug_name that are both atomic values that cannot be changed from outside.

jason-simmons

LGTM

jason-simmons · 2017-07-11T22:30:00Z

shell/common/shell.h

-  std::vector<ftl::WeakPtr<PlatformView>> platform_views_;
+  std::vector<std::weak_ptr<PlatformView>> platform_views_;
+
+  std::recursive_mutex mutex_;


rename this platform_views_mutex_ to indicate what it guards

Ah, now i see that it's also being used to guard the rasterizers list. I'd suggest using two separate locks for platform_views and rasterizers.

Also. does this need to be a recursive_mutex, or can it be a plain std::mutex?

It is needed otherwise the lock inside WaitForPlatformViewIds will acquire it and the nested invocation of GetPlatformViews will freeze.

WaitForPlatformViewIds can access platform_views_ directly while holding the mutex instead of calling GetPlatformViews. This will also avoid the list copy done by GetPlatformViews.

jason-simmons · 2017-07-11T22:34:25Z

shell/platform/android/platform_view_android_jni.cc


-#define PLATFORM_VIEW reinterpret_cast<PlatformViewAndroid*>(platform_view)
+template <typename T>
+class HeapSharedPtr {


what is the purpose of this wrapper?

Trough the JNI we can move just Java Objects or Atomic values.
The std::shared_ptr cannot be passed directly, but the std::make_shared method will create it on the stack.
We wrap it into an object in the heap that will be manually managed.

Can the make_shared() call and the wrapper be replaced by new std::shared_ptr(new PlatformViewAndroid())?

jason-simmons · 2017-07-11T22:37:20Z

shell/platform/android/platform_view_android.cc

 void PlatformViewAndroid::Detach() {
  ReleaseSurface();
-  delete this;
+  //delete this;


remove this line before submitting

jason-simmons · 2017-07-11T22:45:49Z

shell/common/shell.cc

 }

 void Shell::GetRasterizers(std::vector<ftl::WeakPtr<Rasterizer>>* rasterizers) {
-  FTL_DCHECK(gpu_thread_checker_ &&


The check for the GPU thread was replaced by a mutex, but the pointers are still ftl::WeakPtrs that are bound to the GPU thread.

Is there a need to access the rasterizers list outside the GPU thread? If not, then I'd prefer to revert to using the GPU thread checker

You are right, it is an error, I will restore those thread based checks

googlebot · 2017-07-12T00:12:56Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

googlebot · 2017-07-12T00:12:56Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

googlebot · 2017-07-12T00:23:30Z

CLAs look good, thanks!

googlebot · 2017-07-12T00:23:30Z

CLAs look good, thanks!

B3rn475 · 2017-07-12T00:27:13Z

I've squashed all the commits to one

ianloic · 2017-07-12T17:49:38Z

This broke my build...

ianloic · 2017-07-12T17:58:33Z

../../flutter/shell/testing/test_runner.cc:17:19: error: no member named 'Attach' in 'shell::PlatformView'
 platform_view_->Attach();
 ~~~~~~~~~~~~~~  ^

Looks like the test_runner host build broke.

In #3833 the `_flutter.listViews` RPC moved from thread based to lock based synchronization. The thread based synchronization side effect was used by flutter benchmarks in https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/vmservice.dart#L1223 and https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/run_hot.dart#L156 to ensure the completeness of the restart/reload and so correct timing. A new RPC `_flutter.flushUIThreadTasks` is introduced to allow the flutter benchmarks to reintroduce thread based synchronization. Related flutter/flutter#11241

googlebot added the cla: yes label Jun 27, 2017

B3rn475 mentioned this pull request Jun 29, 2017

After sending hot reload with pause: true isolate unexpectedly resumes immediately after PausePostRequest flutter/flutter#10934

Closed

B3rn475 mentioned this pull request Jul 7, 2017

Unable to resume execution after hitting breakpoint in Flutter apps flutter/flutter#10615

Closed

jason-simmons reviewed Jul 11, 2017

View reviewed changes

googlebot added cla: no and removed cla: yes labels Jul 12, 2017

googlebot added cla: yes and removed cla: no labels Jul 12, 2017

Avoid to freeze the system if hot reloading during debug

c5c799c

jason-simmons approved these changes Jul 12, 2017

View reviewed changes

jason-simmons merged commit 8ba522e into flutter:master Jul 12, 2017

ianloic mentioned this pull request Jul 12, 2017

Changes that break test_runner aren't caught in presubmit checks flutter/flutter#11176

Closed

B3rn475 mentioned this pull request Jul 12, 2017

Add missing virtual method in PlatformView #3867

Merged

chinmaygarde mentioned this pull request Jul 18, 2017

Fix Mac desktop platform for use with std::weak_ptr in the PlatformView. #3897

Merged

B3rn475 mentioned this pull request Jul 18, 2017

Add flushUIThreadTasks service RPC #3898

Merged

Avoid to freeze the system if hot reloading during debug #3833

Avoid to freeze the system if hot reloading during debug #3833

Uh oh!

Conversation

B3rn475 commented Jun 27, 2017

Uh oh!

B3rn475 commented Jun 27, 2017

Uh oh!

jason-simmons commented Jun 28, 2017

Uh oh!

B3rn475 commented Jun 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

B3rn475 commented Jun 28, 2017

Uh oh!

jason-simmons commented Jun 28, 2017

Uh oh!

B3rn475 commented Jun 29, 2017

Uh oh!

jason-simmons left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

B3rn475 Jul 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googlebot commented Jul 12, 2017

Uh oh!

googlebot commented Jul 12, 2017

Uh oh!

googlebot commented Jul 12, 2017

Uh oh!

googlebot commented Jul 12, 2017

Uh oh!

B3rn475 commented Jul 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ianloic commented Jul 12, 2017

Uh oh!

ianloic commented Jul 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

B3rn475 commented Jun 28, 2017 •

edited

Loading

B3rn475 Jul 12, 2017 •

edited

Loading

B3rn475 commented Jul 12, 2017 •

edited

Loading