mirror of
https://github.com/flutter/flutter.git
synced 2025-06-03 00:51:18 +00:00
make ChromiumDevice.stopApp
a no-op if it has already been called (#156778)
(Hopefully) fixes https://github.com/flutter/flutter/issues/156700 To summarize that thread, there are a few ways to trigger cleanup logic in `flutter run -d chrome`. For example, pressing `q` will close Chrome, but Chrome closing triggers cleanup logic in the resident runner (which includes trying to close Chrome, which is redundant). A simple way to fix this is to not call `Chrome.close` in `ChromiumDevice.stopApp` when we've already called it once. This solution isn't ideal—consider a scenario where two calls to `stopApp` are made concurrently. The second one would probably complete before the browser is actually closed. However, I don't foresee anything ever depending on `stopApp` finishing implying that the browser is closed. I feel like there might be a yak to shave here (`ResidentWebRunner` could keep track of its state and not initiate redundant cleanup operations), but I am not sure its worth the continued effort. <details> <summary> Pre-launch checklist </summary> - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. </details> <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
This commit is contained in:
parent
3405f11e5c
commit
3512745071
@ -160,7 +160,9 @@ abstract class ChromiumDevice extends Device {
|
||||
ApplicationPackage? app, {
|
||||
String? userIdentifier,
|
||||
}) async {
|
||||
await _chrome?.close();
|
||||
final Future<void>? future = _chrome?.close();
|
||||
_chrome = null;
|
||||
await future;
|
||||
return true;
|
||||
}
|
||||
|
||||
|
@ -12,6 +12,7 @@ import 'package:flutter_tools/src/build_info.dart';
|
||||
import 'package:flutter_tools/src/device.dart';
|
||||
import 'package:flutter_tools/src/web/chrome.dart';
|
||||
import 'package:flutter_tools/src/web/web_device.dart';
|
||||
import 'package:test/fake.dart';
|
||||
|
||||
import '../../src/common.dart';
|
||||
import '../../src/fake_process_manager.dart';
|
||||
@ -32,6 +33,29 @@ void main() {
|
||||
expect(await webDevices.pollingGetDevices(), isEmpty);
|
||||
});
|
||||
|
||||
testWithoutContext('Successive calls of ChromiumDevice.stopApp() do not try to close chrome', () async {
|
||||
final TestChromiumLauncher launcher = TestChromiumLauncher(
|
||||
launcher: () => _OnceClosableChromium(),
|
||||
);
|
||||
|
||||
final _FakeChromiumDevice chromiumDevice = _FakeChromiumDevice(
|
||||
chromiumLauncher: launcher,
|
||||
fileSystem: MemoryFileSystem.test(),
|
||||
logger: BufferLogger.test(),
|
||||
);
|
||||
|
||||
await chromiumDevice.startApp(
|
||||
null,
|
||||
debuggingOptions: DebuggingOptions.enabled(BuildInfo.debug),
|
||||
platformArgs: <String, Object?>{
|
||||
'uri': '/',
|
||||
}
|
||||
);
|
||||
|
||||
await chromiumDevice.stopApp(null);
|
||||
await chromiumDevice.stopApp(null);
|
||||
});
|
||||
|
||||
testWithoutContext('GoogleChromeDevice defaults', () async {
|
||||
final TestChromiumLauncher launcher = TestChromiumLauncher();
|
||||
|
||||
@ -392,7 +416,11 @@ void main() {
|
||||
|
||||
/// A test implementation of the [ChromiumLauncher] that launches a fixed instance.
|
||||
class TestChromiumLauncher implements ChromiumLauncher {
|
||||
TestChromiumLauncher();
|
||||
TestChromiumLauncher({Chromium Function()? launcher}) {
|
||||
_launcher = launcher ?? () => _UnimplementedChromium();
|
||||
}
|
||||
|
||||
late Chromium Function() _launcher;
|
||||
|
||||
@override
|
||||
Completer<Chromium> currentCompleter = Completer<Chromium>();
|
||||
@ -422,6 +450,7 @@ class TestChromiumLauncher implements ChromiumLauncher {
|
||||
Directory? cacheDir,
|
||||
List<String> webBrowserFlags = const <String>[],
|
||||
}) async {
|
||||
currentCompleter.complete(_launcher());
|
||||
return currentCompleter.future;
|
||||
}
|
||||
|
||||
@ -430,3 +459,34 @@ class TestChromiumLauncher implements ChromiumLauncher {
|
||||
return currentCompleter.future;
|
||||
}
|
||||
}
|
||||
|
||||
class _FakeChromiumDevice extends ChromiumDevice {
|
||||
_FakeChromiumDevice(
|
||||
{required ChromiumLauncher chromiumLauncher,
|
||||
required super.fileSystem,
|
||||
required super.logger})
|
||||
: super(
|
||||
name: 'fake name',
|
||||
chromeLauncher: chromiumLauncher,
|
||||
);
|
||||
|
||||
@override
|
||||
Future<String> get sdkNameAndVersion async => 'fake sdkNameAndVersion';
|
||||
|
||||
@override
|
||||
String get name => 'fake name';
|
||||
}
|
||||
|
||||
class _OnceClosableChromium extends Fake implements Chromium {
|
||||
bool _closed = false;
|
||||
|
||||
@override
|
||||
Future<void> close() async {
|
||||
if (_closed) {
|
||||
throw Exception('Already closed');
|
||||
}
|
||||
_closed = true;
|
||||
}
|
||||
}
|
||||
|
||||
class _UnimplementedChromium extends Fake implements Chromium { }
|
||||
|
Loading…
Reference in New Issue
Block a user