这是indexloc提供的服务，不要输入任何密码
Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Guard against Dart timeline API calls invoked during Dart_Cleanup #24007

Merged
merged 1 commit into from
Feb 2, 2021
Merged

Guard against Dart timeline API calls invoked during Dart_Cleanup #24007

merged 1 commit into from
Feb 2, 2021
+127 −14

Conversation

jason-simmons
Copy link
Member

The Dart timeline is not thread safe if an engine thread that is not
controlled by Dart calls Dart_TimelineEvent while another thread is
calling Dart_Cleanup.

Fixes flutter/flutter#74134

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@google-cla google-cla bot added the cla: yes label Jan 28, 2021
chinmaygarde
chinmaygarde approved these changes Jan 28, 2021
View reviewed changes
Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

Other than the bit about the use of the shared-exclusive lock, lgtm.

runtime/dart_vm_initializer.cc Outdated
// APIs. This mutex must be held during calls to Dart_Initialize and
// Dart_Cleanup so that Dart API calls on other threads can be protected
// against a concurrent call to Dart_Cleanup.
static std::mutex gDartInitializerMutex;
Copy link
Member

Choose a reason for hiding this comment

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

We won't have a global lock on timeline traces with a shared-exclusive lock here. Shared access on tracking and exclusive access in the Initialize and Cleanup. We can't use std::shared_mutex because of iOS libc++ but we have fml/synchronization/SharedLock for that purpose.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@chinmaygarde
Copy link
Member

Presubmits are mad about GN formatting.

@chinmaygarde chinmaygarde mentioned this pull request Jan 28, 2021
Closed
@chinmaygarde
Copy link
Member

There is one flake that I filed a report for. The Windows failure is real and indicates you forgot to import <functional>

@jason-simmons jason-simmons force-pushed the bug_74134 branch 3 times, most recently from 5253253 to 5c39fab Compare January 30, 2021 02:13
@jason-simmons
Copy link
Member Author

Looked at the runtime_unittest failures and found that they were caused by deadlocks.

This PR attempted to use a lock around the calls to Dart_Cleanup and Dart_TimelineEvent. This is unsafe because Dart_Cleanup waits for various Dart-owned threads to exit. At the same time, those threads may invoke callbacks provided by the engine, and the callbacks may try to call Dart_TimelineEvent.

SInce the thread calling Dart_Cleanup owns the lock, the other thread can not obtain the lock and is prevented from calling Dart_TimelineEvent. But if Dart_Cleanup is waiting on that other thread, the result is a deadlock.

I changed this PR to use a flag to indicate whether Dart_TimelineEvent is safe. The flag will be cleared before calling Dart_Cleanup, and after that any future attempts to log tracing events will skip the Dart_TimelineEvent call.

This is a race - if a thread sees that the flag is set and proceeds to call Dart_TimelineEvent, that call may fail if another thread calls Dart_Cleanup before the Dart_TimelineEvent call completes. In practice this is unlikely to be a problem because Dart_Cleanup does significant work before deleting the timeline recorder. Any Dart_TimelineEvent calls in progress will probably complete before that happens, and no other Dart_TimelineEvent calls will be made once Dart_Cleanup begins.

I don't think there is a way to make this completely safe given the current design of the Dart APIs. It might be valuable for Dart to provide an API for setup and cleanup of the timeline that is separate from other Dart VM subsystems.

@zanderso zanderso mentioned this pull request Jan 30, 2021
Closed
@zanderso
Copy link
Member

I filed dart-lang/sdk#44814

@jason-simmons
Avoid calling Dart timeline APIs during Dart_Cleanup
fa8d9c2 
The Dart timeline is not thread safe if an engine thread that is not
controlled by Dart calls Dart_TimelineEvent while another thread is
calling Dart_Cleanup.

Fixes flutter/flutter#74134
chinmaygarde
chinmaygarde approved these changes Feb 2, 2021
View reviewed changes
Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

As you mentioned in the comment, that flag is inherently racy. I fear we may just be kicking the can down the road. But, if we are following up on a fix for it and this ameliorates some of the impact caused by flakes, lets land this for now.

@chinmaygarde chinmaygarde mentioned this pull request Feb 2, 2021
Merged
@jason-simmons jason-simmons merged commit df4e79d into flutter:master Feb 2, 2021
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 2, 2021
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 2, 2021
@engine-flutter-autoroll
df4e79d Avoid calling Dart timeline APIs during Dart_Cleanup (flutter…
d2b40d6 
…/engine#24007)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 2, 2021
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 2, 2021
@engine-flutter-autoroll
df4e79d Avoid calling Dart timeline APIs during Dart_Cleanup (flutter…
3d8f9a3 
…/engine#24007)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 2, 2021
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 2, 2021
@engine-flutter-autoroll
df4e79d Avoid calling Dart timeline APIs during Dart_Cleanup (flutter…
b8268ac 
…/engine#24007)
hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021
@jason-simmons @hjfreyer
Avoid calling Dart timeline APIs during Dart_Cleanup (flutter#24007)
68e0a76 
The Dart timeline is not thread safe if an engine thread that is not
controlled by Dart calls Dart_TimelineEvent while another thread is
calling Dart_Cleanup.

Fixes flutter/flutter#74134
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@chinmaygarde chinmaygarde chinmaygarde approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Flaky pure virtual function called in engine host tests
3 participants
@jason-simmons @chinmaygarde @zanderso