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

Hold a reference to the Skia unref queue in UIDartState #13239

Merged
merged 1 commit into from
Oct 21, 2019
Merged

Hold a reference to the Skia unref queue in UIDartState #13239

merged 1 commit into from
Oct 21, 2019

Conversation

jason-simmons
Copy link
Member

Obtaining the SkiaUnrefQueue through the IOManager is unsafe because
UIDartState has a weak pointer to the IOManager that can not be dereferenced
on the UI thread.

@Hixie
Copy link
Contributor

Hixie commented Oct 18, 2019

test?

chinmaygarde
chinmaygarde approved these changes Oct 18, 2019
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.

Very cool. I really like this pattern in shell setup. Thanks @gaaclarke for the idea.

You can close flutter/flutter#42946 too.

@jason-simmons
Copy link
Member Author

The thread safety property involved here is validated by the weak pointer thread checker (see #12257).

When the thread checker was reenabled, a workaround was added toUIDartState that unsafely accessed the SkiaUnrefQueue to avoid triggering the assertions (see https://github.com/flutter/engine/pull/12257/files#diff-5d704fecf8a6d80591469004049997f0R82). This PR removes the workaround and returns a SkiaUnrefQueue without violating any thread safety checks.

@jason-simmons
Hold a reference to the Skia unref queue in UIDartState
0b8e8a1 
Obtaining the SkiaUnrefQueue through the IOManager is unsafe because
UIDartState has a weak pointer to the IOManager that can not be dereferenced
on the UI thread.
@jason-simmons jason-simmons force-pushed the io_thread_skia_queue_2 branch from 4dab82f to 0b8e8a1 Compare October 19, 2019 00:09
gw280
gw280 approved these changes Oct 19, 2019
View reviewed changes
Copy link
Contributor

@gw280 gw280 left a comment

Choose a reason for hiding this comment

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

LGTM

@jason-simmons jason-simmons merged commit 4ecfa62 into flutter:master Oct 21, 2019
@jason-simmons jason-simmons mentioned this pull request Oct 21, 2019
Closed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 21, 2019
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 21, 2019
@engine-flutter-autoroll
4ecfa62 Hold a reference to the Skia unref queue in UIDartState (flut…
868d8d6 
…ter/engine#13239)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 22, 2019
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 22, 2019
@engine-flutter-autoroll
4ecfa62 Hold a reference to the Skia unref queue in UIDartState (flut…
79ac333 
…ter/engine#13239)
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Oct 22, 2019
@engine-flutter-autoroll
Roll engine 8882bf3..39e6901 (21 commits) (#43228)
4dd5097 
[email protected]:flutter/engine.git/compare/8882bf3c73f5...39e6901

git log 8882bf3..39e6901 --no-merges --oneline
2019-10-21 [email protected] Add recipe changelog (flutter/engine#13270)
2019-10-21 [email protected] fix NPE in accessibility bridge (flutter/engine#13255)
2019-10-21 [email protected] Roll src/third_party/skia 9889d509ed9f..56f569d9bec2 (21 commits) (flutter/engine#13266)
2019-10-21 [email protected] Roll fuchsia/sdk/core/mac-amd64 from hc4p_... to hALu4... (flutter/engine#13252)
2019-10-21 [email protected] [web] Support input action (flutter/engine#13268)
2019-10-21 [email protected] [web] Support -j to use goma in felt build (flutter/engine#13259)
2019-10-21 [email protected] Roll fuchsia/sdk/core/linux-amd64 from 30Ua7... to _e7Up... (flutter/engine#13254)
2019-10-21 [email protected] Hold a reference to the Skia unref queue in UIDartState (flutter/engine#13239)
2019-10-21 [email protected] Do not attempt to drain the SkiaUnrefQueue in the destructor (flutter/engine#13237)
2019-10-21 [email protected] Updated license script to ignore testdata directories, which often contain object files and other compilation results (flutter/engine#13261)
2019-10-21 [email protected] Add templates to generate fuchsia host bundles (flutter/engine#13158)
2019-10-21 [email protected] Update ui.instantiateImageCodec docs to reflect what it does. (flutter/engine#13233)
2019-10-21 [email protected] Update CanvasKit to 0.7.0 and flesh out painting (flutter/engine#13240)
2019-10-19 [email protected] Roll fuchsia/sdk/core/linux-amd64 from CYDvx... to 30Ua7... (flutter/engine#13251)
2019-10-19 [email protected] Roll fuchsia/sdk/core/mac-amd64 from 0JpMS... to hc4p_... (flutter/engine#13250)
2019-10-19 [email protected] Roll fuchsia/sdk/core/linux-amd64 from bdTv5... to CYDvx... (flutter/engine#13249)
2019-10-19 [email protected] Roll src/third_party/skia c65eb34d2f37..9889d509ed9f (1 commits) (flutter/engine#13248)
2019-10-19 [email protected] Roll fuchsia/sdk/core/mac-amd64 from SevlL... to 0JpMS... (flutter/engine#13244)
2019-10-19 [email protected] Roll src/third_party/skia 7605c89c00f7..c65eb34d2f37 (3 commits) (flutter/engine#13243)
2019-10-19 [email protected] Ignore *.obj files when gathering licenses (flutter/engine#13241)
2019-10-18 [email protected] Roll buildroot to 994c6 (flutter/engine#13236)


If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected] on the revert to ensure that a human
is aware of the problem.

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
@engine-flutter-autoroll @Inconnu08
Roll engine 8882bf3..39e6901 (21 commits) (flutter#43228)
dbab64e 
[email protected]:flutter/engine.git/compare/8882bf3c73f5...39e6901

git log 8882bf3..39e6901 --no-merges --oneline
2019-10-21 [email protected] Add recipe changelog (flutter/engine#13270)
2019-10-21 [email protected] fix NPE in accessibility bridge (flutter/engine#13255)
2019-10-21 [email protected] Roll src/third_party/skia 9889d509ed9f..56f569d9bec2 (21 commits) (flutter/engine#13266)
2019-10-21 [email protected] Roll fuchsia/sdk/core/mac-amd64 from hc4p_... to hALu4... (flutter/engine#13252)
2019-10-21 [email protected] [web] Support input action (flutter/engine#13268)
2019-10-21 [email protected] [web] Support -j to use goma in felt build (flutter/engine#13259)
2019-10-21 [email protected] Roll fuchsia/sdk/core/linux-amd64 from 30Ua7... to _e7Up... (flutter/engine#13254)
2019-10-21 [email protected] Hold a reference to the Skia unref queue in UIDartState (flutter/engine#13239)
2019-10-21 [email protected] Do not attempt to drain the SkiaUnrefQueue in the destructor (flutter/engine#13237)
2019-10-21 [email protected] Updated license script to ignore testdata directories, which often contain object files and other compilation results (flutter/engine#13261)
2019-10-21 [email protected] Add templates to generate fuchsia host bundles (flutter/engine#13158)
2019-10-21 [email protected] Update ui.instantiateImageCodec docs to reflect what it does. (flutter/engine#13233)
2019-10-21 [email protected] Update CanvasKit to 0.7.0 and flesh out painting (flutter/engine#13240)
2019-10-19 [email protected] Roll fuchsia/sdk/core/linux-amd64 from CYDvx... to 30Ua7... (flutter/engine#13251)
2019-10-19 [email protected] Roll fuchsia/sdk/core/mac-amd64 from 0JpMS... to hc4p_... (flutter/engine#13250)
2019-10-19 [email protected] Roll fuchsia/sdk/core/linux-amd64 from bdTv5... to CYDvx... (flutter/engine#13249)
2019-10-19 [email protected] Roll src/third_party/skia c65eb34d2f37..9889d509ed9f (1 commits) (flutter/engine#13248)
2019-10-19 [email protected] Roll fuchsia/sdk/core/mac-amd64 from SevlL... to 0JpMS... (flutter/engine#13244)
2019-10-19 [email protected] Roll src/third_party/skia 7605c89c00f7..c65eb34d2f37 (3 commits) (flutter/engine#13243)
2019-10-19 [email protected] Ignore *.obj files when gathering licenses (flutter/engine#13241)
2019-10-18 [email protected] Roll buildroot to 994c6 (flutter/engine#13236)


If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected] on the revert to ensure that a human
is aware of the problem.

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
2 more reviewers

@chinmaygarde chinmaygarde chinmaygarde approved these changes

@gw280 gw280 gw280 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jason-simmons @Hixie @chinmaygarde @gw280 @googlebot