better event emitting II: no deadlocks

Summary:D3092867 / 1d3db4c5dc caused deadlock when chrome debugging was turned on, so it was reverted as D3128586 / 144dc30661.
The reason: I was calling `[_bridge dispatchBlock:^{ [self flushEventsQueue]; } queue:RCTJSThread];` from main thread and expecting it will `dispatch_async` to another,
since a held lock was being accessed the dispatched block and was released after the dispatch.
Turns out `RCTWebSocketExecutor` (which is used when chrome debugger is turned on) executes all blocks dispatched this way to `RCTJSThread` synchronously on the main thread.
This resulted in a deadlock. The "dispatched" block was trying to acquired lock which held by the same thread in the dispatching phase.

A fix for this is pretty simple. We will release the lock before dispatching the block.

However it's not super straightforward to see this won't introduce some race condition in a case with two threads where we would end up with events not being processed.
My thinking why that shouldn't happen goes like this: We could get in a bad state if `flushEventsQueue` would run on JS thread while `sendEvent:` is running on MT.
(I don't have a specific example how, maybe it's not possible. However when I show this case is safe we know we are good.)
The way how locking is setup in this diff the only possible scenario where these two threads would execute in these methods concurrently is JS holding the lock and MT going to enqueue another block on JS thread (since that's outside of "locked" zone).
But this scenarion can never happen, since if MT is about to enqueue the block on JS thread it means there cannot be a not yet fully executed block on JS thread.
Therefore nothing bad can happen.

So this diff brings back the reverted diff and adds to it the fix for the deadlock.

Reviewed By: javache

Differential Revision: D3130375

fb-gh-sync-id: 885a166f2f808551d7cd4e4eb98634d26afe6a11
fbshipit-source-id: 885a166f2f808551d7cd4e4eb98634d26afe6a11

Examples/UIExplorer/UIExplorerUnitTests/RCTEventDispatcherTests.m

@@ -17,6 +17,7 @@
 
 #import <OCMock/OCMock.h>
 #import "RCTEventDispatcher.h"
+#import "RCTBridge+Private.h"
 
 @interface RCTTestEvent : NSObject  <RCTEvent>
 @property (atomic, assign, readwrite) BOOL canCoalesce;
@@ -82,7 +83,8 @@
 {
   [super setUp];
 
-  _bridge = [OCMockObject mockForClass:[RCTBridge class]];
+  _bridge = [OCMockObject mockForClass:[RCTBatchedBridge class]];
+
   _eventDispatcher = [RCTEventDispatcher new];
   [_eventDispatcher setValue:_bridge forKey:@"bridge"];
 
@@ -106,61 +108,79 @@
   [_bridge verify];
 }
 
-- (void)testNonCoalescingEventsAreImmediatelyDispatched
+- (void)testNonCoalescingEventIsImmediatelyDispatched
 {
   _testEvent.canCoalesce = NO;
-  [[_bridge expect] enqueueJSCall:_JSMethod
-                             args:[_testEvent arguments]];
+
+  [[_bridge expect] dispatchBlock:OCMOCK_ANY queue:RCTJSThread];
 
   [_eventDispatcher sendEvent:_testEvent];
 
   [_bridge verify];
 }
 
-- (void)testCoalescedEventShouldBeDispatchedOnFrameUpdate
+- (void)testCoalescingEventIsImmediatelyDispatched
 {
+  _testEvent.canCoalesce = YES;
+
+  [[_bridge expect] dispatchBlock:OCMOCK_ANY queue:RCTJSThread];
+
+  [_eventDispatcher sendEvent:_testEvent];
+
+  [_bridge verify];
+}
+
+- (void)testMultipleEventsResultInOnlyOneDispatchAfterTheFirstOne
+{
+  [[_bridge expect] dispatchBlock:OCMOCK_ANY queue:RCTJSThread];
+  [_eventDispatcher sendEvent:_testEvent];
+  [_eventDispatcher sendEvent:_testEvent];
+  [_eventDispatcher sendEvent:_testEvent];
+  [_eventDispatcher sendEvent:_testEvent];
   [_eventDispatcher sendEvent:_testEvent];
   [_bridge verify];
+}
+
+- (void)testRunningTheDispatchedBlockResultInANewOneBeingEnqueued
+{
+  __block dispatch_block_t eventsEmittingBlock;
+  [[_bridge expect] dispatchBlock:[OCMArg checkWithBlock:^(dispatch_block_t block) {
+    eventsEmittingBlock = block;
+    return YES;
+  }] queue:RCTJSThread];
+  [_eventDispatcher sendEvent:_testEvent];
+  [_bridge verify];
+
+
+  // eventsEmittingBlock would be called when js is no longer busy, which will result in emitting events
   [[_bridge expect] enqueueJSCall:@"RCTDeviceEventEmitter.emit"
                              args:[_testEvent arguments]];
-
-  [(id<RCTFrameUpdateObserver>)_eventDispatcher didUpdateFrame:nil];
-
+  eventsEmittingBlock();
   [_bridge verify];
-}
 
-- (void)testNonCoalescingEventForcesColescedEventsToBeImmediatelyDispatched
-{
-  RCTTestEvent *nonCoalescingEvent = [[RCTTestEvent alloc] initWithViewTag:nil
-                                                                 eventName:_eventName
-                                                                      body:@{}
-                                                             coalescingKey:0];
-  nonCoalescingEvent.canCoalesce = NO;
+
+  [[_bridge expect] dispatchBlock:OCMOCK_ANY queue:RCTJSThread];
   [_eventDispatcher sendEvent:_testEvent];
-
-  [[_bridge expect] enqueueJSCall:[[_testEvent class] moduleDotMethod]
-                             args:[_testEvent arguments]];
-  [[_bridge expect] enqueueJSCall:[[nonCoalescingEvent class] moduleDotMethod]
-                             args:[nonCoalescingEvent arguments]];
-
-  [_eventDispatcher sendEvent:nonCoalescingEvent];
   [_bridge verify];
 }
 
 - (void)testBasicCoalescingReturnsLastEvent
 {
+  __block dispatch_block_t eventsEmittingBlock;
+  [[_bridge expect] dispatchBlock:[OCMArg checkWithBlock:^(dispatch_block_t block) {
+    eventsEmittingBlock = block;
+    return YES;
+  }] queue:RCTJSThread];
+  [[_bridge expect] enqueueJSCall:@"RCTDeviceEventEmitter.emit"
+                             args:[_testEvent arguments]];
+
   RCTTestEvent *ignoredEvent = [[RCTTestEvent alloc] initWithViewTag:nil
                                                            eventName:_eventName
                                                                 body:@{ @"other": @"body" }
                                                        coalescingKey:0];
-
   [_eventDispatcher sendEvent:ignoredEvent];
   [_eventDispatcher sendEvent:_testEvent];
-
-  [[_bridge expect] enqueueJSCall:@"RCTDeviceEventEmitter.emit"
-                             args:[_testEvent arguments]];
-
-  [(id<RCTFrameUpdateObserver>)_eventDispatcher didUpdateFrame:nil];
+  eventsEmittingBlock();
 
   [_bridge verify];
 }
@@ -169,20 +189,60 @@
 {
   NSString *firstEventName = RCTNormalizeInputEventName(@"firstEvent");
   RCTTestEvent *firstEvent = [[RCTTestEvent alloc] initWithViewTag:nil
-                                                           eventName:firstEventName
-                                                                body:_body
+                                                         eventName:firstEventName
+                                                              body:_body
                                                      coalescingKey:0];
 
-  [_eventDispatcher sendEvent:firstEvent];
-  [_eventDispatcher sendEvent:_testEvent];
-
+  __block dispatch_block_t eventsEmittingBlock;
+  [[_bridge expect] dispatchBlock:[OCMArg checkWithBlock:^(dispatch_block_t block) {
+    eventsEmittingBlock = block;
+    return YES;
+  }] queue:RCTJSThread];
   [[_bridge expect] enqueueJSCall:@"RCTDeviceEventEmitter.emit"
                              args:[firstEvent arguments]];
-
   [[_bridge expect] enqueueJSCall:@"RCTDeviceEventEmitter.emit"
                              args:[_testEvent arguments]];
 
-  [(id<RCTFrameUpdateObserver>)_eventDispatcher didUpdateFrame:nil];
+
+  [_eventDispatcher sendEvent:firstEvent];
+  [_eventDispatcher sendEvent:_testEvent];
+  eventsEmittingBlock();
+
+  [_bridge verify];
+}
+
+- (void)testSameEventTypesWithDifferentCoalesceKeysDontCoalesce
+{
+  NSString *eventName = RCTNormalizeInputEventName(@"firstEvent");
+  RCTTestEvent *firstEvent = [[RCTTestEvent alloc] initWithViewTag:nil
+                                                         eventName:eventName
+                                                              body:_body
+                                                     coalescingKey:0];
+  RCTTestEvent *secondEvent = [[RCTTestEvent alloc] initWithViewTag:nil
+                                                         eventName:eventName
+                                                              body:_body
+                                                     coalescingKey:1];
+
+  __block dispatch_block_t eventsEmittingBlock;
+  [[_bridge expect] dispatchBlock:[OCMArg checkWithBlock:^(dispatch_block_t block) {
+    eventsEmittingBlock = block;
+    return YES;
+  }] queue:RCTJSThread];
+  [[_bridge expect] enqueueJSCall:@"RCTDeviceEventEmitter.emit"
+                             args:[firstEvent arguments]];
+  [[_bridge expect] enqueueJSCall:@"RCTDeviceEventEmitter.emit"
+                             args:[secondEvent arguments]];
+
+
+  [_eventDispatcher sendEvent:firstEvent];
+  [_eventDispatcher sendEvent:secondEvent];
+  [_eventDispatcher sendEvent:firstEvent];
+  [_eventDispatcher sendEvent:secondEvent];
+  [_eventDispatcher sendEvent:secondEvent];
+  [_eventDispatcher sendEvent:firstEvent];
+  [_eventDispatcher sendEvent:firstEvent];
+
+  eventsEmittingBlock();
 
   [_bridge verify];
 }

React/Base/RCTEventDispatcher.h

@@ -97,13 +97,8 @@ RCT_EXTERN NSString *RCTNormalizeInputEventName(NSString *eventName);
 /**
  * Send a pre-prepared event object.
  *
- * If the event can be coalesced it is added to a pool of events that are sent at the beginning of the next js frame.
- * Otherwise if the event cannot be coalesced we first flush the pool of coalesced events and the new event after that.
- *
- * Why it works this way?
- * Making sure js gets events in the right order is crucial for correctly interpreting gestures.
- * Unfortunately we cannot emit all events as they come. If we would do that we would have to emit scroll and touch moved event on every frame,
- * which is too much data to transfer and process on older devices. This is especially bad when js starts lagging behind main thread.
+ * Events are sent to JS as soon as the thread is free to process them.
+ * If an event can be coalesced and there is another compatible event waiting, the coalescing will happen immediately.
  */
 - (void)sendEvent:(id<RCTEvent>)event;
 

React/Base/RCTEventDispatcher.m

@@ -11,7 +11,9 @@
 
 #import "RCTAssert.h"
 #import "RCTBridge.h"
+#import "RCTBridge+Private.h"
 #import "RCTUtils.h"
+#import "RCTProfile.h"
 
 const NSInteger RCTTextUpdateLagWarningThreshold = 3;
 
@@ -35,38 +37,24 @@ static NSNumber *RCTGetEventID(id<RCTEvent> event)
   );
 }
 
-@interface RCTEventDispatcher() <RCTFrameUpdateObserver>
-
-@end
-
 @implementation RCTEventDispatcher
 {
-  NSMutableDictionary *_eventQueue;
+  // We need this lock to protect access to _eventQueue and __eventsDispatchScheduled. It's filled in on main thread and consumed on js thread.
   NSLock *_eventQueueLock;
+  NSMutableDictionary *_eventQueue;
+  BOOL _eventsDispatchScheduled;
 }
 
 @synthesize bridge = _bridge;
-@synthesize paused = _paused;
-@synthesize pauseCallback = _pauseCallback;
 
 RCT_EXPORT_MODULE()
 
 - (void)setBridge:(RCTBridge *)bridge
 {
   _bridge = bridge;
-  _paused = YES;
   _eventQueue = [NSMutableDictionary new];
   _eventQueueLock = [NSLock new];
-}
-
-- (void)setPaused:(BOOL)paused
-{
-  if (_paused != paused) {
-    _paused = paused;
-    if (_pauseCallback) {
-      _pauseCallback();
-    }
-  }
+  _eventsDispatchScheduled = NO;
 }
 
 - (void)sendAppEventWithName:(NSString *)name body:(id)body
@@ -139,25 +127,33 @@ RCT_EXPORT_MODULE()
 
 - (void)sendEvent:(id<RCTEvent>)event
 {
-  if (!event.canCoalesce) {
-    [self flushEventsQueue];
-    [self dispatchEvent:event];
-    return;
-  }
-
   [_eventQueueLock lock];
 
   NSNumber *eventID = RCTGetEventID(event);
-  id<RCTEvent> previousEvent = _eventQueue[eventID];
 
+  id<RCTEvent> previousEvent = _eventQueue[eventID];
   if (previousEvent) {
+    RCTAssert([event canCoalesce], @"Got event %@ which cannot be coalesced, but has the same eventID %@ as the previous event %@", event, eventID, previousEvent);
     event = [previousEvent coalesceWithEvent:event];
   }
-
   _eventQueue[eventID] = event;
-  self.paused = NO;
 
+  BOOL scheduleEventsDispatch = NO;
+  if (!_eventsDispatchScheduled) {
+    _eventsDispatchScheduled = YES;
+    scheduleEventsDispatch = YES;
+  }
+
+  // We have to release the lock before dispatching block with events,
+  // since dispatchBlock: can be executed synchronously on the same queue.
+  // (This is happening when chrome debugging is turned on.)
   [_eventQueueLock unlock];
+
+  if (scheduleEventsDispatch) {
+    [_bridge dispatchBlock:^{
+      [self flushEventsQueue];
+    } queue:RCTJSThread];
+  }
 }
 
 - (void)dispatchEvent:(id<RCTEvent>)event
@@ -170,17 +166,13 @@ RCT_EXPORT_MODULE()
   return RCTJSThread;
 }
 
-- (void)didUpdateFrame:(__unused RCTFrameUpdate *)update
-{
-  [self flushEventsQueue];
-}
-
+// js thread only (which suprisingly can be the main thread, depends on used JS executor)
 - (void)flushEventsQueue
 {
   [_eventQueueLock lock];
   NSDictionary *eventQueue = _eventQueue;
   _eventQueue = [NSMutableDictionary new];
-  self.paused = YES;
+  _eventsDispatchScheduled = NO;
   [_eventQueueLock unlock];
 
   for (id<RCTEvent> event in eventQueue.allValues) {