diff --git a/devices/devicekit/active-app.go b/devices/devicekit/active-app.go index 1e1791d..dfc9b69 100644 --- a/devices/devicekit/active-app.go +++ b/devices/devicekit/active-app.go @@ -16,7 +16,20 @@ type ActiveAppInfo struct { } func (c *DeviceKitClient) GetActiveAppInfo() (*ActiveAppInfo, error) { - result, err := c.CallRPC("device.apps.foreground", nil) + return c.getActiveAppInfo(true) +} + +// GetActiveAppIdentity names the foreground app without asking which view +// controller is on screen. The agent finds that by snapshotting the hierarchy, +// which on an app with a large one costs as much as a UI dump; callers that +// only need to know which app is in front should not pay for it. +func (c *DeviceKitClient) GetActiveAppIdentity() (*ActiveAppInfo, error) { + return c.getActiveAppInfo(false) +} + +func (c *DeviceKitClient) getActiveAppInfo(withViewController bool) (*ActiveAppInfo, error) { + // older agents ignore the parameter and always report the view controller + result, err := c.CallRPC("device.apps.foreground", map[string]bool{"viewController": withViewController}) if err != nil { return nil, fmt.Errorf("failed to get active app info: %w", err) } diff --git a/devices/ios_device_flutter.go b/devices/ios_device_flutter.go index 4c6b05c..390bb01 100644 --- a/devices/ios_device_flutter.go +++ b/devices/ios_device_flutter.go @@ -68,7 +68,7 @@ func advertisesDartVMService(app installationproxy.AppInfo) bool { // determined it says yes with no bundle id, which is the old probe-over-LLDB // behaviour. func (d *IOSDevice) foregroundFlutterCandidate() (bundleID string, mayBeFlutter bool) { - activeApp, err := d.deviceKitClient.GetActiveAppInfo() + activeApp, err := d.deviceKitClient.GetActiveAppIdentity() if err != nil || activeApp.ProcessID == 0 { utils.Verbose("flutter: cannot tell the foreground app, probing anyway: %v", err) return "", true diff --git a/devices/simulator_flutter.go b/devices/simulator_flutter.go index dac6d3a..a097479 100644 --- a/devices/simulator_flutter.go +++ b/devices/simulator_flutter.go @@ -9,6 +9,7 @@ import ( "path/filepath" "regexp" "strings" + "sync" "time" "github.com/mobile-next/mobilecli/types" @@ -33,12 +34,8 @@ var vmServiceLineURL = regexp.MustCompile(`http://127\.0\.0\.1:\d+/[A-Za-z0-9_=- // tryDumpFlutterSource returns the Flutter render tree for the foreground app, // or ok=false to signal the caller should use the accessibility dump. func (s *SimulatorDevice) tryDumpFlutterSource() ([]types.ScreenElement, bool) { - foreground, err := s.GetForegroundApp() - if err != nil { - return nil, false - } - bundleID := foreground.PackageName - if !s.isFlutterAppBundle(bundleID) { + bundleID, mayBeFlutter := s.foregroundFlutterCandidate() + if !mayBeFlutter { return nil, false } uri := s.flutterVMServiceURI(bundleID) @@ -56,6 +53,69 @@ func (s *SimulatorDevice) tryDumpFlutterSource() ([]types.ScreenElement, bool) { return elements, true } +// simulatorFlutterCandidates caches the verdict of foregroundFlutterCandidate, +// keyed by simulator udid. SimulatorDevice methods run against a copy of the +// device, so the cache cannot live in the struct the way it does for a real +// device. +var ( + simulatorFlutterMu sync.Mutex + simulatorFlutterCandidates = map[string]flutterCandidate{} +) + +// cachedFlutterCandidate returns the verdict remembered for the process running +// on a simulator, and whether there is one. A different pid means the app was +// relaunched or reinstalled, which is a miss: the bundle may be another build. +func cachedFlutterCandidate(udid string, pid int) (isCandidate bool, ok bool) { + if pid == 0 { + return false, false + } + + simulatorFlutterMu.Lock() + defer simulatorFlutterMu.Unlock() + + cached, exists := simulatorFlutterCandidates[udid] + if !exists || cached.pid != pid { + return false, false + } + return cached.isCandidate, true +} + +// rememberFlutterCandidate stores the verdict for a process. A pid of 0 means +// the agent did not report one, and a verdict that cannot be invalidated is +// worse than no verdict at all, so it is dropped. +func rememberFlutterCandidate(udid string, pid int, isCandidate bool) { + if pid == 0 { + return + } + + simulatorFlutterMu.Lock() + defer simulatorFlutterMu.Unlock() + simulatorFlutterCandidates[udid] = flutterCandidate{pid: pid, isCandidate: isCandidate} +} + +// foregroundFlutterCandidate decides whether looking for a Dart VM is worth it, +// and names the foreground app. Reading the app bundle costs a simctl call, more +// than the whole accessibility dump of a light screen, and every dump would pay +// it; the answer cannot change while the process runs, so it is remembered per +// process id, as on a real device. A relaunched or reinstalled app gets a new +// pid and is looked at again. +func (s *SimulatorDevice) foregroundFlutterCandidate() (bundleID string, mayBeFlutter bool) { + activeApp, err := s.deviceKitClient.GetActiveAppIdentity() + if err != nil || activeApp.BundleID == "" { + utils.Verbose("flutter: cannot tell the foreground app, skipping the flutter dump: %v", err) + return "", false + } + + if isCandidate, ok := cachedFlutterCandidate(s.UDID, activeApp.ProcessID); ok { + return activeApp.BundleID, isCandidate + } + + isCandidate := s.isFlutterAppBundle(activeApp.BundleID) + rememberFlutterCandidate(s.UDID, activeApp.ProcessID, isCandidate) + + return activeApp.BundleID, isCandidate +} + // isFlutterAppBundle reports whether the installed app embeds Flutter.framework. func (s *SimulatorDevice) isFlutterAppBundle(bundleID string) bool { out, err := runSimctl("get_app_container", s.UDID, bundleID, "app") diff --git a/devices/simulator_flutter_test.go b/devices/simulator_flutter_test.go new file mode 100644 index 0000000..e23c27a --- /dev/null +++ b/devices/simulator_flutter_test.go @@ -0,0 +1,65 @@ +package devices + +import "testing" + +// A verdict that outlives the process it was taken for is the only way this +// cache can be wrong, and it would be wrong silently: a Flutter app would be +// dumped through the accessibility tree, or a native app would be probed for a +// Dart VM on every dump. The tests below pin the rules that prevent that. +func TestFlutterCandidateIsRememberedForTheSameProcess(t *testing.T) { + t.Cleanup(func() { delete(simulatorFlutterCandidates, "udid-1") }) + + if _, ok := cachedFlutterCandidate("udid-1", 100); ok { + t.Fatal("an unknown simulator must not have a verdict") + } + + rememberFlutterCandidate("udid-1", 100, true) + + isCandidate, ok := cachedFlutterCandidate("udid-1", 100) + if !ok || !isCandidate { + t.Errorf("the verdict for the same process must be reused, got (%v, %v)", isCandidate, ok) + } +} + +// A reinstall can replace a native build with a Flutter one under the same +// bundle id, so the pid, not the bundle id, is what the verdict is tied to. +func TestFlutterCandidateIsLookedAtAgainAfterTheAppRestarts(t *testing.T) { + t.Cleanup(func() { delete(simulatorFlutterCandidates, "udid-2") }) + + rememberFlutterCandidate("udid-2", 100, false) + + if _, ok := cachedFlutterCandidate("udid-2", 101); ok { + t.Error("a relaunched or reinstalled app has a new pid and must be looked at again") + } +} + +// The agent reports pid 0 when it cannot name the foreground process. Storing +// a verdict against it would pin an answer that no app relaunch could ever +// clear, so paying the simctl call again is the cheaper mistake. +func TestFlutterCandidateIsNotRememberedWithoutAProcessID(t *testing.T) { + t.Cleanup(func() { delete(simulatorFlutterCandidates, "udid-3") }) + + rememberFlutterCandidate("udid-3", 0, true) + + if _, ok := cachedFlutterCandidate("udid-3", 0); ok { + t.Error("a verdict that cannot be invalidated must not be stored") + } + if _, ok := cachedFlutterCandidate("udid-3", 100); ok { + t.Error("a verdict stored without a pid must not be served to a real process") + } +} + +// Pids are per simulator, so two booted simulators running different apps can +// hand out the same number; the udid has to be part of the key. +func TestFlutterCandidatesAreNotSharedBetweenSimulators(t *testing.T) { + t.Cleanup(func() { + delete(simulatorFlutterCandidates, "udid-4") + delete(simulatorFlutterCandidates, "udid-5") + }) + + rememberFlutterCandidate("udid-4", 100, true) + + if _, ok := cachedFlutterCandidate("udid-5", 100); ok { + t.Error("two simulators can run the same pid; a verdict must not cross over") + } +}