Repository: firebase/firebase-ios-sdk (language: cpp)

Issue:
IAM ignores messageDisplaySuppressed on slow networks
<!-- DO NOT DELETE
validate_template=true
template_path=.github/ISSUE_TEMPLATE/bug_report.md
-->

### [REQUIRED] Step 1: Describe your environment

  * Xcode version: 12.3
  * Firebase SDK version: 7.4.0
  * Installation method: `CocoaPods`
  * Firebase Component: IAM

### [REQUIRED] Step 2: Describe the problem

On slow networks, the In-App Messaging component appears to ignore the `messageDisplaySuppressed` flag.

Consider the following scenario:

1. An in-app message is configured to display as soon as the user has opened the chat screen;
2. When the on-screen keyboard is displayed, messages must never appear. To implement that behavior, the app receives `keyboardWillShowNotification` and `keyboardDidHideNotification`, and sets IAM’s `messageDisplaySuppressed` property to `true` and `false`, respectively;
3. On slow networks, it takes IAM significant time to fetch a message’s image;
4. In that time, the user is able to open the keyboard, which results in `messageDisplaySuppressed` changed to `true` (`Message display suppress set to 1` is printed to the console);
5. When the image is fetched, IAM ignores the updated `messageDisplaySuppressed` value and displays the message anyway.

These steps result in the message—and most crucially the button used to dismiss it—half-hidden beneath the keyboard. The keyboard itself cannot be dismissed, too, since the message’s semi-transparent background layer does not pass taps through to the chat view controller. This leaves force-quitting the app as the only way to resolve the issue.

<img width="483" src="https://user-images.githubusercontent.com/4648742/104718956-c766f600-573c-11eb-9154-df9aa05a00c4.png">

The expected / acceptable outcomes are:
* The message is triggered *and displayed* before the user is able to open the keyboard (that’s how it currently works on fast networks);
* If the user opens the keyboard before the message appears, the latter should not display.

The quick fix I see is adding another check on `messageDisplaySuppressed`, performed *just before* the message appears on screen. (I think I can implement and submit such a fix.) An alternative solution, which seems nicer, would be extending `InAppMessagingDisplayDelegate ` with a `shouldDisplayMessage(_:) -> Bool` method called when the message is about to display.


Discussion / hints:
I found a few problems with this issue:
  * I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.
  * This issue does not seem to follow the issue template. Make sure you provide all the required information.

Fix patch:
diff --git a/FirebaseInAppMessaging/Sources/Flows/FIRIAMDisplayExecutor.m b/FirebaseInAppMessaging/Sources/Flows/FIRIAMDisplayExecutor.m
index e3e766a1d03..4a0759681fa 100644
--- a/FirebaseInAppMessaging/Sources/Flows/FIRIAMDisplayExecutor.m
+++ b/FirebaseInAppMessaging/Sources/Flows/FIRIAMDisplayExecutor.m
@@ -639,6 +639,14 @@ - (void)displayForMessage:(FIRIAMMessageDefinition *)message
           }
         }
 
+        // On slow networks, image loading may take significant time,
+        // in which the value of `suppressMessageDisplay` could change.
+        if (self.suppressMessageDisplay) {
+          FIRLogDebug(kFIRLoggerInAppMessaging, @"I-IAM400042",
+                      @"Message display suppressed by developer at message display time.");
+          return;
+        }
+
         self.impressionRecorded = NO;
         self.isMsgBeingDisplayed = YES;
 
diff --git a/FirebaseInAppMessaging/Tests/Unit/FIRIAMDisplayExecutorTests.m b/FirebaseInAppMessaging/Tests/Unit/FIRIAMDisplayExecutorTests.m
index cc993ac2aec..8e075fe5222 100644
--- a/FirebaseInAppMessaging/Tests/Unit/FIRIAMDisplayExecutorTests.m
+++ b/FirebaseInAppMessaging/Tests/Unit/FIRIAMDisplayExecutorTests.m
@@ -37,6 +37,7 @@ @interface FIRIAMMessageContentDataForTesting : NSObject <FIRIAMMessageContentDa
 @property(nonatomic, nullable) NSURL *imageURL;
 @property(nonatomic, nullable) NSURL *landscapeImageURL;
 @property BOOL errorEncountered;
+@property BOOL loadImagesAsynchronously;
 
 - (instancetype)initWithMessageTitle:(NSString *)title
                          messageBody:(NSString *)body
@@ -46,7 +47,8 @@ - (instancetype)initWithMessageTitle:(NSString *)title
                   secondaryActionURL:(nullable NSURL *)secondaryActionURL
                             imageURL:(nullable NSURL *)imageURL
                    landscapeImageURL:(nullable NSURL *)landscapeImageURL
-                       hasImageError:(BOOL)hasImageError;
+                       hasImageError:(BOOL)hasImageError
+            loadImagesAsynchronously:(BOOL)loadImagesAsynchronously;
 @end
 
 @implementation FIRIAMMessageContentDataForTesting
@@ -58,7 +60,8 @@ - (instancetype)initWithMessageTitle:(NSString *)title
                   secondaryActionURL:(nullable NSURL *)secondaryActionURL
                             imageURL:(nullable NSURL *)imageURL
                    landscapeImageURL:(nullable NSURL *)landscapeImageURL
-                       hasImageError:(BOOL)hasImageError {
+                       hasImageError:(BOOL)hasImageError
+            loadImagesAsynchronously:(BOOL)loadImagesAsynchronously {
   if (self = [super init]) {
     _titleText = title;
     _bodyText = body;
@@ -69,6 +72,7 @@ - (instancetype)initWithMessageTitle:(NSString *)title
     _actionURL = actionURL;
     _secondaryActionURL = secondaryActionURL;
     _errorEncountered = hasImageError;
+    _loadImagesAsynchronously = loadImagesAsynchronously;
   }
   return self;
 }
@@ -77,14 +81,38 @@ - (void)loadImageDataWithBlock:(void (^)(NSData *_Nullable imageData,
                                          NSData *_Nullable landscapeImageData,
                                          NSError *_Nullable error))block {
   if (self.errorEncountered) {
-    block(nil, nil, [NSError errorWithDomain:@"image error" code:0 userInfo:nil]);
+    NSError *error = [NSError errorWithDomain:@"image error" code:0 userInfo:nil];
+
+    if (_loadImagesAsynchronously) {
+      [self performOnMainQueueAfterDelay:0.01
+                                   block:^{
+                                     block(nil, nil, error);
+                                   }];
+    } else {
+      block(nil, nil, error);
+    }
   } else {
     NSData *imageData = [@"image data" dataUsingEncoding:NSUTF8StringEncoding];
     NSData *landscapeImageData = [@"landscape image data" dataUsingEncoding:NSUTF8StringEncoding];
 
-    block(imageData, landscapeImageData, nil);
+    if (_loadImagesAsynchronously) {
+      [self performOnMainQueueAfterDelay:0.01
+                                   block:^{
+                                     block(imageData, landscapeImageData, nil);
+                                   }];
+    } else {
+      block(imageData, landscapeImageData, nil);
+    }
   }
 }
+
+- (void)performOnMainQueueAfterDelay:(NSTimeInterval)delay block:(void (^)(void))block {
+  dispatch_after(dispatch_time(DISPATCH_TIME_NOW, delay * NSEC_PER_SEC), dispatch_get_main_queue(),
+                 ^{
+                   block();
+                 });
+}
+
 @end
 
 // Defines how the message display component triggers the delegate in unit testing.
@@ -207,8 +235,8 @@ @interface FIRIAMDisplayExecutorTests : XCTestCase
 
 @property id<FIRInAppMessagingDisplay> mockMessageDisplayComponent;
 
-// four pre-defined messages
-@property FIRIAMMessageDefinition *m1, *m2, *m3, *m4, *m5;
+// Pre-defined messages
+@property FIRIAMMessageDefinition *m1, *m2, *m3, *m4, *m5, *m6;
 @end
 
 @implementation FIRIAMDisplayExecutorTests
@@ -222,7 +250,7 @@ - (void)setupMessageTexture {
   FIRIAMDisplayTriggerDefinition *contextualTriggerDefinition =
       [[FIRIAMDisplayTriggerDefinition alloc] initWithFirebaseAnalyticEvent:@"test_event"];
 
-  // m2, m4, and m5 will be of app open trigger
+  // m2, m4, m5, and m6 will be of app open trigger
   FIRIAMDisplayTriggerDefinition *appOpentriggerDefinition =
       [[FIRIAMDisplayTriggerDefinition alloc] initForAppForegroundTrigger];
 
@@ -235,7 +263,8 @@ - (void)setupMessageTexture {
              secondaryActionURL:nil
                        imageURL:[NSURL URLWithString:@"https://google.com/image"]
               landscapeImageURL:nil
-                  hasImageError:NO];
+                  hasImageError:NO
+       loadImagesAsynchronously:NO];
 
   FIRIAMRenderingEffectSetting *renderSetting1 =
       [FIRIAMRenderingEffectSetting getDefaultRenderingEffectSetting];
@@ -261,7 +290,8 @@ - (void)setupMessageTexture {
              secondaryActionURL:nil
                        imageURL:[NSURL URLWithString:@"https://unsplash.it/300/400"]
               landscapeImageURL:nil
-                  hasImageError:NO];
+                  hasImageError:NO
+       loadImagesAsynchronously:NO];
 
   FIRIAMRenderingEffectSetting *renderSetting2 =
       [FIRIAMRenderingEffectSetting getDefaultRenderingEffectSetting];
@@ -287,7 +317,8 @@ - (void)setupMessageTexture {
              secondaryActionURL:nil
                        imageURL:[NSURL URLWithString:@"https://google.com/image"]
               landscapeImageURL:nil
-                  hasImageError:NO];
+                  hasImageError:NO
+       loadImagesAsynchronously:NO];
 
   FIRIAMRenderingEffectSetting *renderSetting3 =
       [FIRIAMRenderingEffectSetting getDefaultRenderingEffectSetting];
@@ -313,7 +344,8 @@ - (void)setupMessageTexture {
              secondaryActionURL:nil
                        imageURL:[NSURL URLWithString:@"https://google.com/image"]
               landscapeImageURL:nil
-                  hasImageError:NO];
+                  hasImageError:NO
+       loadImagesAsynchronously:NO];
 
   FIRIAMRenderingEffectSetting *renderSetting4 =
       [FIRIAMRenderingEffectSetting getDefaultRenderingEffectSetting];
@@ -353,7 +385,8 @@ - (void)setupMessageTexture {
              secondaryActionURL:nil
                        imageURL:[NSURL URLWithString:@"https://google.com/image"]
               landscapeImageURL:nil
-                  hasImageError:NO];
+                  hasImageError:NO
+       loadImagesAsynchronously:NO];
 
   FIRIAMRenderingEffectSetting *renderSetting5 =
       [FIRIAMRenderingEffectSetting getDefaultRenderingEffectSetting];
@@ -372,6 +405,33 @@ - (void)setupMessageTexture {
                                                         appData:nil
                                               experimentPayload:nil
                                                   isTestMessage:NO];
+
+  FIRIAMMessageContentDataForTesting *m6ContentData = [[FIRIAMMessageContentDataForTesting alloc]
+           initWithMessageTitle:@"m6 title"
+                    messageBody:@"message body"
+               actionButtonText:nil
+      secondaryActionButtonText:nil
+                      actionURL:[NSURL URLWithString:@"http://google.com"]
+             secondaryActionURL:nil
+                       imageURL:[NSURL URLWithString:@"https://google.com/image"]
+              landscapeImageURL:nil
+                  hasImageError:NO
+       loadImagesAsynchronously:YES];
+
+  FIRIAMRenderingEffectSetting *renderSetting6 =
+      [FIRIAMRenderingEffectSetting getDefaultRenderingEffectSetting];
+  renderSetting6.viewMode = FIRIAMRenderAsBannerView;
+
+  FIRIAMMessageRenderData *renderData6 =
+      [[FIRIAMMessageRenderData alloc] initWithMessageID:@"m6"
+                                             messageName:@"name"
+                                             contentData:m6ContentData
+                                         renderingEffect:renderSetting6];
+
+  self.m6 = [[FIRIAMMessageDefinition alloc] initWithRenderData:renderData6
+                                                      startTime:activeStartTime
+                                                        endTime:activeEndTime
+                                              triggerDefinition:@[ appOpentriggerDefinition ]];
 }
 
 NSTimeInterval DISPLAY_MIN_INTERVALS = 1;
@@ -888,6 +948,36 @@ - (void)testNoRenderingIfMessageDisplayIsSuppressed {
   XCTAssertEqual(1, remainingMsgCount2);
 }
 
+- (void)testNoRenderingIfMessageDisplayIsSuppressedDuringImageLoading {
+  // This setup allows next message to be displayed from display interval perspective.
+  OCMStub([self.mockTimeFetcher currentTimestampInSeconds])
+      .andReturn(DISPLAY_MIN_INTERVALS * 60 + 100);
+
+  [self.clientMessageCache setMessageData:@[ self.m6 ]];
+
+  FIRIAMMessageDisplayForTesting *display = [[FIRIAMMessageDisplayForTesting alloc]
+      initWithDelegateInteraction:FIRInAppMessagingDelegateInteractionClick];
+  self.displayExecutor.messageDisplayComponent = display;
+  [self.displayExecutor checkAndDisplayNextAppForegroundMessage];
+  self.displayExecutor.suppressMessageDisplay = YES;
+
+  XCTestExpectation *expectation = [[XCTestExpectation alloc] init];
+
+  dispatch_after(dispatch_time(DISPATCH_TIME_NOW, 0.05 * NSEC_PER_SEC), dispatch_get_main_queue(),
+                 ^{
+                   // no message display has happened
+                   XCTAssertNil(display.message);
+
+                   NSInteger remainingMsgCount = [self.clientMessageCache allRegularMessages].count;
+                   // no message is removed from the cache
+                   XCTAssertEqual(1, remainingMsgCount);
+
+                   [expectation fulfill];
+                 });
+
+  [self waitForExpectations:@[ expectation ] timeout:0.1];
+}
+
 // No contextual message rendering if suppress message display flag is turned on
 - (void)testNoContextualMsgRenderingIfMessageDisplayIsSuppressed {
   // This setup allows next message to be displayed from display interval perspective.


Test patch: