Fix mDNS for iOS13 #13451

dnfield · 2019-10-31T00:02:33Z

Tested manually.

iOS 13 has broken the way we use NSNetService. This uses the C API introduced in iOS 9.3 and works locally.

I've tried to use availability guards to make this still support older versions, but I'm not clear at all on whether this is actually the right way to do things for iOS < 9.3 - and I can't track down a device to test it with. Would be happy for pointers on that.

Also, this code cannot make it into a release build - so if it is wrong, it should only impact developers trying to test on iOS < 9.3.

@stuartmorgan might have good ideas here but he's OOO for a while to come.

stuartmorgan-g

I'm not really familiar with these APIs so I don't think I can be of much help.

I will say at a high level that if you are going to have this much of the implementation in branches that use the same condition (OS version in this case) it would be much cleaner to have two concrete implementations of an internal interface, one for each path, and then just have the primary class instantiate the right one at initialization, eliminating all the other branching. That avoids weirdness like ivars that are always there but never used in one implementation.

dnfield · 2019-10-31T05:36:26Z

Adding @jmagman for some extra Objective C review.

Sorry the diff is so ugly now. I refactored this to avoid all of the #ifdef and if (@available) branching.

zanderso · 2019-10-31T15:20:29Z

shell/platform/darwin/ios/framework/Source/FlutterObservatoryPublisher.mm

-#include <net/if.h>  // nogncheck
-#endif               // TARGET_IPHONE_SIMULATOR
+#if FLUTTER_RUNTIME_MODE == FLUTTER_RUNTIME_MODE_RELEASE || \
+    FLUTTER_RUNTIME_MODE == FLUTTER_RUNTIME_MODE_DYNAMIC_RELEASE


Does FLUTTER_RUNTIME_MODE_DYNAMIC_RELEASE still exist? Should this be FLUTTER_RUNTIME_MODE_JIT_RELEASE? It looks like FLUTTER_RELEASE might do the right thing. That is:

#if FLUTTER_RELEASE ... #else ... #endif

Ahh nice one. No, I don't think dynamic is anymore.

I've updated config.gni to set those defines (which is where we set all the other runtime related defines already) instead of in a separate header. /cc @jonahwilliams

dnfield · 2019-10-31T16:32:16Z

I've potentially located an iOS 8.1 device to test with.

Looking at analytics, we have a very very small number of people who are trying to use flutter run with an iOS 8.0-9.2.x device. (We also apparently have some people trying it with iOS < 8.0?!). It's much smaller than the number of people who are impacted by this bug.

jmagman · 2019-10-31T19:52:58Z

I've potentially located an iOS 8.1 device to test with.

I don't think you should need a real device. For availability purposes you can build against that SDK and see if you get any warnings/errors.
I believe iOS 8 SDK was in Xcode 6: https://download.developer.apple.com/Developer_Tools/Xcode_6.3.1/Xcode_6.3.1.dmg

You can copy that SDK to your current version of Xcode, or actually run in Xcode 6, if that even works anymore.

dnfield · 2019-10-31T23:10:44Z

@jmagman I wasn't quite able to get that to work, but I did get things running with an old iPhone on 8.1 - I do see this in the console:

assertion failed: 12B411: libxpc.dylib + 51955 [04694BEB-256F-3132-A00F-0C82B79BC689]: 0x7d

But otherwise things are good. Except that mDNS doesn't work at all on that phone (either with the current approach or the previous one), but log scanning does.

jmagman · 2019-10-31T22:50:56Z

shell/platform/darwin/ios/framework/Source/FlutterObservatoryPublisher.mm

+}
+
+- (NSString*)serviceName {
+  return [[[NSBundle mainBundle] infoDictionary] objectForKey:@"CFBundleIdentifier"];


Does NSBundle.mainBundle.bundleIdentifier have what you need?

jmagman · 2019-10-31T23:19:49Z

shell/platform/darwin/ios/framework/Source/FlutterObservatoryPublisher.mm

+    // Remove leading "/"
+    path = path.substr(1);


Do we need to use std::string here? Since it's Obj-C -> Obj-C it would be nice to not need to translate between unless there's a good reason.

NSString *path = [url.path stringByReplacingOccurrencesOfString:@"/" withString:@"" options:NSLiteralSearch range:NSMakeRange(0, 1)]; // OR if you don't want any trailing / either NSString *path = [url.path stringByTrimmingCharactersInSet:[NSCharacterSet characterSetWithCharactersInString:@"/"]]; NSData *data = [path dataUsingEncoding:NSUTF8StringEncoding]; NSDictionary<NSString*,NSData*> *authCode = @{ @"authCode" : data }; return [NSNetService dataFromTXTRecordDictionary:authCode];

Done - slightly different than your suggestion but yeah, that was legacy code from something else.

jmagman · 2019-10-31T23:23:43Z

shell/platform/darwin/ios/framework/Source/FlutterObservatoryPublisher.mm

+#if FLUTTER_RELEASE

-#import "FlutterObservatoryPublisher.h"
+@implementation FlutterObservatoryPublisher {


Remove empty {}

jmagman · 2019-10-31T23:28:11Z

shell/platform/darwin/ios/framework/Source/FlutterObservatoryPublisher.mm

  const char* registrationType = "_dartobservatory._tcp";
  const char* domain = "local.";  // default domain
-  uint16_t port = [[_url port] intValue];
+  uint16_t port = [[url port] intValue];


-unsignedShortValue

jmagman · 2019-11-01T18:19:39Z

shell/platform/darwin/ios/framework/Source/FlutterObservatoryPublisher.mm

+  // it as a txt record so flutter tools can establish a connection.
+  NSString* path = [[url path] substringFromIndex:MIN(1, [[url path] length])];
+  NSData* pathData = [path dataUsingEncoding:NSUTF8StringEncoding];
+  NSDictionary* txtDict = @{


Let's use generics on collections:
NSDictionary<NSString*, NSData*>

jmagman · 2019-11-01T18:31:28Z

shell/platform/darwin/ios/framework/Source/FlutterObservatoryPublisher.mm

 }

- (instancetype)init {
+@synthesize url;


Do you need this @synthesize?

AFAIK yes - because it's from a protocol and not autosynthesized. At any rate, doesn't compile without this.

Oh that make sense.

I think this needs an ivar, how is this not crashing when you use url?

@implementation ObservatoryDNSServiceDelegate { fml::scoped_nsobject<FlutterObservatoryPublisher> _owner; DNSServiceRef _dnsServiceRef; fml::scoped_nsobject<NSURL> _url; } @synthesize url = _url;

self.url.reset([[[NSURL alloc] initWithString:uri] retain]);

From our conversation offline - this doesn't work.

I think this is because fml::scoped_nsobject is a stack allocated C++ object, rather than a pointer or a NSObject. It makes things look really funky compared to how this would be done if we were using ARC (where the property/ivar would just be something like a raw NSURL*). But this syntax compiles and runs.

jmagman · 2019-11-01T18:31:58Z

shell/platform/darwin/ios/framework/Source/FlutterObservatoryPublisher.mm

-    @"authCode" : pathData,
-  };
-  NSData* txtData = [NSNetService dataFromTXTRecordDictionary:txtDict];
+  url.reset([[[NSURL alloc] initWithString:uri] retain]);


self.url, or am I missing where this url is coming from?

Undone haha.

url is the synethsized property from the protocol right?

jmagman · 2019-11-01T18:34:20Z

shell/platform/darwin/ios/framework/Source/FlutterObservatoryPublisher.mm

+      [weak = _weakFactory->GetWeakPtr(),
+       runner = fml::MessageLoop::GetCurrent().GetTaskRunner()](const std::string& uri) {
+        if (!uri.empty()) {
+          runner->PostTask([weak, uri]() {


Make a strong pointer in the block so it doesn't dealloc between the null check and the usage

This is the pattern we use all over the place for fml::WeakPtrs - I think it's ok. We can't really get a "strong" reference because we're weakly holding it.

jmagman · 2019-11-01T18:36:55Z

shell/platform/darwin/ios/framework/Source/FlutterObservatoryPublisher.mm

+}
+
+- (NSURL*)url {
+  return [_delegate.get().url copy];


I don't think you need a copy since it's not mutable. [_delegate.get().url autorelease]?

And I think it causes an analyzer warning?

Potential leak of an object of type 'id'
Object leaked: allocated object of type 'id' is returned from a method whose name ('url') does not start with 'copy', 'mutableCopy', 'alloc' or 'new'. This violates the naming convention rules given in the Memory Management Guide for Cocoa

jmagman

I still find it difficult to read the juxtaposition between Obj-C and C++ memory management, so thanks for your patience, @dnfield. But as far as I can tell, the .mm part LGTM.

Fix mDNS for iOS13

7042256

dnfield requested review from cbracken, chinmaygarde and zanderso October 31, 2019 00:02

googlebot added the cla: yes label Oct 31, 2019

stuartmorgan-g reviewed Oct 31, 2019

View reviewed changes

dnfield added 2 commits October 30, 2019 22:31

refactor

0c66f0d

format

ad2c5e5

dnfield requested a review from jmagman October 31, 2019 05:35

dnfield added 2 commits October 30, 2019 23:10

guard includes

2aca16b

debugging notes

61ef2e4

zanderso reviewed Oct 31, 2019

View reviewed changes

fix up release def

064221f

jmagman reviewed Oct 31, 2019

View reviewed changes

review

ac1a036

jmagman reviewed Nov 1, 2019

View reviewed changes

dnfield added 2 commits November 1, 2019 13:00

review++

4ecc71e

format

ea4ffb6

jmagman approved these changes Nov 1, 2019

View reviewed changes

dnfield added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Nov 1, 2019

dnfield merged commit 0a8bd9d into flutter:master Nov 1, 2019

dnfield deleted the dns_fix branch November 1, 2019 22:29

dnfield mentioned this pull request Nov 4, 2019

Manually roll engine to unred the tree flutter/flutter#44150

Merged

This was referenced Nov 5, 2019

Occasional mDNS lookup failure on iOS 13 flutter/flutter#41085

Closed

iOS 13 Device, app stuck at slash screen when use VSCode, but not with terminal.(mDNS lookup failed, attempting fallback to reading device log ) flutter/flutter#43570

Closed

Fix mDNS for iOS13 #13451

Fix mDNS for iOS13 #13451

Uh oh!

Conversation

dnfield commented Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

dnfield commented Oct 31, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield commented Oct 31, 2019

Uh oh!

jmagman commented Oct 31, 2019

Uh oh!

dnfield commented Oct 31, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

dnfield commented Oct 31, 2019 •

edited

Loading