LIFX / LIFXKit

The LIFX SDK for Objective-C.
MIT License
122 stars 76 forks source link

LIFX White 800 on network breaks all 3rd party apps #24

Closed lightbow closed 9 years ago

lightbow commented 9 years ago

I have a user reporting a crash that happens every time, shortly after launch, only if a LIFX White 800 is on their network. It looks like it is happening on the main thread, and entirely within LIFXKit. My LIFX White 800 bulbs are still in transit with FedEx, so I haven't verified this yet in-person, but I hope I'm wrong and this problem isn't widespread. Posting here in case any other developers can confirm or deny reproducibility. LIFXKit is seven months old, but the main LIFX app is clearly using a newer version of this code.

-[LFXMessageRateManager connect]: unrecognized selector sent to instance 0x15ed5ab0

Here's the offending chunk of code:

        LFXGatewayConnection *newConnection = [LFXGatewayConnection gatewayConnectionWithGatewayDescriptor:gateway messageRateManager:self.networkContext.messageRateManager delegate:self];
        self.gatewayConnections[gateway] = newConnection;
        [newConnection connect];

where newConnection isn't actually a LFXGatewayConnection, but somehow a LFXMessageRateManager. Looking at the code, that doesn't seem possible, so I wonder if it's a memory management issue and it's messaging garbage?

5   Lightbow                             0x001622c3 -[LFXLocalTransportManager gatewayDiscoveryController:didUpdateEntry:entryIsNew:] (LFXLocalTransportManager.m:351)
6   Lightbow                             0x0015c68b -[LFXGatewayDiscoveryController handleStatePANGatewayMessage:] (LFXGatewayDiscoveryController.m:98)
7   Lightbow                             0x0015bf6f __94+[LFXGatewayDiscoveryController gatewayDiscoveryControllerWithLocalTransportManager:delegate:]_block_invoke (LFXGatewayDiscoveryController.m:39)
8   Lightbow                             0x0016f16b -[LFXTransportManager sendObserverCallbacksForMessage:] (LFXTransportManager.m:126)
9   Lightbow                             0x0016201b -[LFXLocalTransportManager gatewayConnection:didReceiveMessage:fromHost:] (LFXLocalTransportManager.m:313)
10  Lightbow                             0x00170a83 -[LFXUDPGatewayConnection udpSocket:didReceiveData:fromAddress:withFilterContext:] (LFXUDPGatewayConnection.m:266)
11  Lightbow                             0x0014c86b __72-[GCDAsyncUdpSocket notifyDidReceiveData:fromAddress:withFilterContext:]_block_invoke (GCDAsyncUdpSocket.m:978)
12  libdispatch.dylib                    0x31c052e3 _dispatch_call_block_and_release + 8
13  libdispatch.dylib                    0x31c052cf _dispatch_client_callout + 20
14  libdispatch.dylib                    0x31c08d2f _dispatch_main_queue_callback_4CF + 1328
15  CoreFoundation                       0x230ec609 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 6
16  CoreFoundation                       0x230ead09 __CFRunLoopRun + 1510
17  CoreFoundation                       0x23037201 CFRunLoopRunSpecific + 474
18  CoreFoundation                       0x23037013 CFRunLoopRunInMode + 104
19  GraphicsServices                     0x2a8d0201 GSEventRunModal + 134
20  UIKit                                0x267dba59 UIApplicationMain + 1438
21  Lightbow                             0x001fc46b main (main.m:16)
22  libdyld.dylib                        0x31c26aaf start + 0
lightbow commented 9 years ago

Just confirmed this with my own hardware setup. If there's a LIFX White 800 on the network, this SDK will crash as described above. Since it's down in LIFXKit, in a code path that's run automatically, this renders all 3rd party apps broken (crash on startup) for all these users installing their new bulbs. Anyone have a direct contact at LIFX to help report this issue? This github page seems pretty quiet recently, and this seems like an issue that can't really wait a few weeks.

lightbow commented 9 years ago

@nsforge any chance there's an imminent update in the wings?

lightbow commented 9 years ago

At the time of crash

(lldb) p gateway.service
(LXProtocolDeviceService) $7 = 5

Looks like there's a new service in town, and "5" isn't in the current enum:

typedef NS_ENUM(uint8_t, LXProtocolDeviceService) {
    LX_PROTOCOL_DEVICE_SERVICE_UDP = 1,
    LX_PROTOCOL_DEVICE_SERVICE_TCP = 2,
};

This obviously toasts the initializer:

+ (instancetype)gatewayConnectionWithGatewayDescriptor:(LFXGatewayDescriptor *)gateway messageRateManager:(LFXMessageRateManager *)messageRateManager delegate:(id <LFXGatewayConnectionDelegate>)delegate
{
    switch (gateway.service)
    {
        case LX_PROTOCOL_DEVICE_SERVICE_TCP:
            return [[LFXTCPGatewayConnection alloc] initWithGatewayDescriptor:gateway messageRateManager:messageRateManager delegate:delegate];
        case LX_PROTOCOL_DEVICE_SERVICE_UDP:
            return [[LFXUDPGatewayConnection alloc] initWithGatewayDescriptor:gateway messageRateManager:messageRateManager delegate:delegate];
    }
}
nsforge commented 9 years ago

@lightbow sorry, I no longer work at LIFX, and I'm not sure who's responsible for LIFXKit now.

sync commented 9 years ago

@lightbow neither do I sorry

avernon commented 9 years ago

Hey @lightbow can you please email me directly at aaron@lifx.com so that we can discuss this further.

lightbow commented 9 years ago

@chendo I don't think you should just return nil in that case, because then the very next line in LFXLocalTransportManager would crash when it tries to stuff nil object into an NSDictionary.

        LFXGatewayConnection *newConnection = [LFXGatewayConnection gatewayConnectionWithGatewayDescriptor:gateway messageRateManager:self.networkContext.messageRateManager delegate:self];
        self.gatewayConnections[gateway] = newConnection;
chendo commented 9 years ago

That's why I also added a check for nil and returning before putting it in the dictionary.

On Wednesday, April 29, 2015, Lightbow notifications@github.com wrote:

@chendo https://github.com/chendo I don't think you should just return nil in that case, because then the very next line in LFXLocalTransportManager would crash when it tries to stuff nil object into an NSDictionary.

    LFXGatewayConnection *newConnection = [LFXGatewayConnection gatewayConnectionWithGatewayDescriptor:gateway messageRateManager:self.networkContext.messageRateManager delegate:self];
    self.gatewayConnections[gateway] = newConnection;

— Reply to this email directly or view it on GitHub https://github.com/LIFX/LIFXKit/issues/24#issuecomment-97401188.

chendo commented 9 years ago

We've tested the fix with the example browser app with a White 800 on the network and it no longer crashes for us. Please try master on your end and let us know if it resolves the issue.

On Wednesday, April 29, 2015, Jack Chen chendo@lifx.co wrote:

That's why I also added a check for nil and returning before putting it in the dictionary.

On Wednesday, April 29, 2015, Lightbow <notifications@github.com javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

@chendo https://github.com/chendo I don't think you should just return nil in that case, because then the very next line in LFXLocalTransportManager would crash when it tries to stuff nil object into an NSDictionary.

    LFXGatewayConnection *newConnection = [LFXGatewayConnection gatewayConnectionWithGatewayDescriptor:gateway messageRateManager:self.networkContext.messageRateManager delegate:self];
    self.gatewayConnections[gateway] = newConnection;

— Reply to this email directly or view it on GitHub https://github.com/LIFX/LIFXKit/issues/24#issuecomment-97401188.

lightbow commented 9 years ago

Ok, it seems to work for me now. I had assumed skipping that connection would prevent the LIFX White 800 from showing up, but I confirmed Lightbow is back in action with that patch in place. I guess that connection was for some other bulb feature, and not needed for basic control?