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

Release shim bindings when detaching #13432

Merged
merged 1 commit into from
Nov 2, 2019
Merged

Release shim bindings when detaching #13432

merged 1 commit into from
Nov 2, 2019

Conversation

@xster
Copy link
Member

@xster xster commented Oct 30, 2019

flutter/flutter#36898 doesn't repro, but there's a conceptual leak nevertheless where the shim registry and registrar hold onto bindings (which holds onto a bunch of other stuff) after detaching.

@xster xster requested a review from matthew-carroll October 30, 2019 04:46
matthew-carroll
matthew-carroll approved these changes Oct 30, 2019
View reviewed changes
Copy link
Contributor

@matthew-carroll matthew-carroll left a comment

Choose a reason for hiding this comment

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

LGTM. Is there still more investigation that needs to happen into the reported leak?

shell/platform/android/io/flutter/embedding/engine/plugins/shim/ShimRegistrar.java
}

pluginBinding = null;
activityPluginBinding = null;
Copy link
Contributor

@matthew-carroll matthew-carroll Oct 30, 2019

Choose a reason for hiding this comment

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

FYI, this isn't technically necessary because the engine will already be detached from the Activity before this executes.

Copy link
Member Author

@xster xster Nov 2, 2019

Choose a reason for hiding this comment

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

Discussed offline. Understood.

Though I'd keep them here since the current docs don't refer have the engine and activity callbacks refer to each other in terms of timing. So it would be a knowledge leak from the implementer side here to assume there is a sequence.

(though not a bad idea to actually add that to the javadoc, e.g. onDetachedFromEngine is guaranteed to call after all the ui component bindings had a chance to detach first and vice versa).

@xster
Copy link
Member Author

xster commented Oct 31, 2019

I couldn't repro the reported leak via leakcanary. I don't think there are further actions.

@xster xster merged commit 7aa4055 into flutter:master Nov 2, 2019
@xster xster deleted the shim-aggregate-leak branch November 2, 2019 00:53
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 5, 2019
@engine-flutter-autoroll
7aa4055 Release shim bindings when detaching (flutter/engine#13432)
792e04e
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 5, 2019
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 5, 2019
@engine-flutter-autoroll
7aa4055 Release shim bindings when detaching (flutter/engine#13432)
51f3f3e
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 5, 2019
Merged
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Nov 5, 2019
@engine-flutter-autoroll
Roll engine 0a8bd9d..e73c9c8 (13 commits) (#44156)
220c6fb 
git@github.com:flutter/engine.git/compare/0a8bd9dd6fbb...e73c9c8

git log 0a8bd9d..e73c9c8 --no-merges --oneline
2019-11-04 skia-flutter-autoroll@skia.org Roll src/third_party/skia 8e083eee8ece..4cc2dc64ff13 (1 commits) (flutter/engine#13543)
2019-11-04 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from ggsQ5... to XcaoM... (flutter/engine#13535)
2019-11-04 yjbanov@google.com fix getBoxesForRange for zero-length ranges (flutter/engine#13483)
2019-11-04 bkonyi@google.com Roll src/third_party/dart bbe2ac28c9..ab5cf0f854 (73 commits) (flutter/engine#13614)
2019-11-02 skia-flutter-autoroll@skia.org Roll src/third_party/skia 283ec65f632a..8e083eee8ece (32 commits) (flutter/engine#13489)
2019-11-02 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from FKqQ_... to ggsQ5... (flutter/engine#13490)
2019-11-02 xster@google.com Release shim bindings when detaching (flutter/engine#13432)
2019-11-01 gw280@google.com Add fuchsia.intl.PropertyProvider to our services on Fuchsia (flutter/engine#13486)
2019-11-01 fmil@google.com [dart] Makes the intl services available (flutter/engine#13460)
2019-11-01 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from 866GG... to dhwMR... (flutter/engine#13475)
2019-11-01 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from JngMB... to FKqQ_... (flutter/engine#13477)
2019-11-01 skia-flutter-autoroll@skia.org Roll src/third_party/skia 809ec77893be..283ec65f632a (14 commits) (flutter/engine#13472)
2019-11-01 dnfield@google.com Request a reattach when creating the text input plugin on Android (flutter/engine#13474)


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 cbracken@google.com 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 0a8bd9d..e73c9c8 (13 commits) (flutter#44156)
5dab5c8 
git@github.com:flutter/engine.git/compare/0a8bd9dd6fbb...e73c9c8

git log 0a8bd9d..e73c9c8 --no-merges --oneline
2019-11-04 skia-flutter-autoroll@skia.org Roll src/third_party/skia 8e083eee8ece..4cc2dc64ff13 (1 commits) (flutter/engine#13543)
2019-11-04 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from ggsQ5... to XcaoM... (flutter/engine#13535)
2019-11-04 yjbanov@google.com fix getBoxesForRange for zero-length ranges (flutter/engine#13483)
2019-11-04 bkonyi@google.com Roll src/third_party/dart bbe2ac28c9..ab5cf0f854 (73 commits) (flutter/engine#13614)
2019-11-02 skia-flutter-autoroll@skia.org Roll src/third_party/skia 283ec65f632a..8e083eee8ece (32 commits) (flutter/engine#13489)
2019-11-02 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from FKqQ_... to ggsQ5... (flutter/engine#13490)
2019-11-02 xster@google.com Release shim bindings when detaching (flutter/engine#13432)
2019-11-01 gw280@google.com Add fuchsia.intl.PropertyProvider to our services on Fuchsia (flutter/engine#13486)
2019-11-01 fmil@google.com [dart] Makes the intl services available (flutter/engine#13460)
2019-11-01 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from 866GG... to dhwMR... (flutter/engine#13475)
2019-11-01 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from JngMB... to FKqQ_... (flutter/engine#13477)
2019-11-01 skia-flutter-autoroll@skia.org Roll src/third_party/skia 809ec77893be..283ec65f632a (14 commits) (flutter/engine#13472)
2019-11-01 dnfield@google.com Request a reattach when creating the text input plugin on Android (flutter/engine#13474)


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 cbracken@google.com 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

1 more reviewer

@matthew-carroll matthew-carroll matthew-carroll 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.

3 participants

@xster @matthew-carroll @googlebot