Skip to content

Fix daemon device discovery crash when Xcode isn't installed#60546

Merged
jmagman merged 1 commit into
flutter:masterfrom
jmagman:xcode-device-events
Jun 29, 2020
Merged

Fix daemon device discovery crash when Xcode isn't installed#60546
jmagman merged 1 commit into
flutter:masterfrom
jmagman:xcode-device-events

Conversation

@jmagman

@jmagman jmagman commented Jun 29, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Description

When Xcode isn't installed, no Android devices shown in Intellij/Android Studio plugins. Crash:

java.io.IOException: error from device.enable: "NoSuchMethodError: The method 'listen' was called on null.\nReceiver: null\nTried calling: listen(Closure: (Map<XCDeviceEvent, String>) => Future<void> from Function '_onDeviceEvent@132191375':., cancelOnError: true, onDone: Closure: () => Null, onError: Closure: (dynamic, StackTrace) => Null)"

Related Issues

Fallout from #58137, which was cherry-picked as 1.17.4.

flutter/flutter-intellij#4642
#60473

Tests

  • start polling without Xcode installed
    This test crashes on master, passes on this PR.

Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I signed the [CLA].
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read [Handling breaking changes].

  • No, no existing tests failed, so this is not a breaking change.
  • Yes, this is a breaking change. If not, delete the remainder of this section.

@jmagman jmagman added c: crash Stack traces logged to the console tool Affects the "flutter" command-line tool. See also t: labels. t: xcode "xcodebuild" on iOS and general Xcode project management labels Jun 29, 2020
@jmagman jmagman self-assigned this Jun 29, 2020
@jmagman

jmagman commented Jun 29, 2020

Copy link
Copy Markdown
Member Author

Thanks to @helin24 for tracking this down.

jonahwilliams
jonahwilliams reviewed Jun 29, 2020
View reviewed changes
Comment thread packages/flutter_tools/lib/src/ios/devices.dart

// cancel any outstanding subscriptions.
await _observedDeviceEventsSubscription?.cancel();
_observedDeviceEventsSubscription = _xcdevice.observedDeviceEvents().listen(

@jonahwilliams jonahwilliams Jun 29, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is the early return above not sufficient here?

@jmagman jmagman Jun 29, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It is sufficient for this crash, but _xcdevice.observedDeviceEvents() is nullable, callers shouldn't need to understand why it can return null (that it can only return null if Xcode isn't installed).

jonahwilliams
jonahwilliams approved these changes Jun 29, 2020
View reviewed changes

@jonahwilliams jonahwilliams left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

christopherfujino
christopherfujino approved these changes Jun 29, 2020
View reviewed changes

@christopherfujino christopherfujino left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@jmagman jmagman merged commit d5225a5 into flutter:master Jun 29, 2020
@jmagman jmagman deleted the xcode-device-events branch June 29, 2020 21:54
@devoncarew devoncarew mentioned this pull request Jun 29, 2020
Closed
pcsosinski pushed a commit to pcsosinski/flutter that referenced this pull request Jun 30, 2020
@jmagman
Fix daemon device discovery crash when Xcode isn't installed (flutter…
de6b02e 
…#60546)
pcsosinski pushed a commit that referenced this pull request Jun 30, 2020
@jmagman
1.17.5 CP: Fix daemon device discovery crash when Xcode isn't install…
8af6b2f 
…ed (#60546) (#60611)

* Fix daemon device discovery crash when Xcode isn't installed (#60546)

* getAvailableIOSDevices -> getAvailableTetheredIOSDevices

Co-authored-by: Jenn Magder <magder@google.com>
pcsosinski pushed a commit to pcsosinski/flutter that referenced this pull request Jul 1, 2020
@jmagman
Fix daemon device discovery crash when Xcode isn't installed (flutter…
f17b709 
…#60546)
pcsosinski pushed a commit that referenced this pull request Jul 1, 2020
@jmagman
Fix daemon device discovery crash when Xcode isn't installed (#60546) (…
8fe7655 
…#60717)

Co-authored-by: Jenn Magder <magder@google.com>
@jmagman jmagman added the platform-ios iOS applications specifically label Aug 21, 2020
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
@jmagman @mingwandroid
Fix daemon device discovery crash when Xcode isn't installed (flutter…
44f07ce 
…#60546)
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@christopherfujino christopherfujino christopherfujino approved these changes
@jonahwilliams jonahwilliams jonahwilliams approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@jmagman jmagman

Labels

c: crash Stack traces logged to the console platform-ios iOS applications specifically t: xcode "xcodebuild" on iOS and general Xcode project management tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jmagman @christopherfujino @jonahwilliams @googlebot