Changeset 259819 in webkit

Timestamp:

Apr 9, 2020, 1:16:38 PM (6 years ago)

Author:

Wenson Hsieh

Message:

Add an API test for <​https://trac.webkit.org/r259766>
​https://bugs.webkit.org/show_bug.cgi?id=210294

Reviewed by Tim Horton.

Source/WebCore:

Avoid trying to place the missing value into paragraphSets in TextManipulationController by bailing if either
the start or end positions are null (while the missing value requires both the start and end to be null, it is
sufficient to bail if either are null because observeParagraphs will be a no-op anyways).

See Tools/ChangeLog for more details.

Test: TextManipulation.CompleteTextManipulationAvoidCrashingWhenContentIsRemoved

• editing/TextManipulationController.cpp:

(WebCore::TextManipulationController::scheduleObservartionUpdate):

Tools:

Exercise the pathological case fixed in r259766 by inserting and then immediately removing a paragraph element
after starting text manipulation. This test also revealed an existing issue in TextManipulationController, where
we will end up hitting a debug assertion when trying to insert { null position, null position } into a HashMap
underneath TextManipulationController::scheduleObservartionUpdate.

• TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:

(TestWebKitAPI::TEST):

Location:

trunk

Files:

• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/editing/TextManipulationController.cpp (modified) (1 diff)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2020-04-09  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Add an API test for <https://trac.webkit.org/r259766>
+        https://bugs.webkit.org/show_bug.cgi?id=210294
+
+        Reviewed by Tim Horton.
+
+        Avoid trying to place the missing value into paragraphSets in TextManipulationController by bailing if either
+        the start or end positions are null (while the missing value requires both the start and end to be null, it is
+        sufficient to bail if either are null because `observeParagraphs` will be a no-op anyways).
+
+        See Tools/ChangeLog for more details.
+
+        Test: TextManipulation.CompleteTextManipulationAvoidCrashingWhenContentIsRemoved
+
+        * editing/TextManipulationController.cpp:
+        (WebCore::TextManipulationController::scheduleObservartionUpdate):
+
 2020-04-09  Ryan Haddad  <ryanhaddad@apple.com>
 

trunk/Source/WebCore/editing/TextManipulationController.cpp

@@ -377 +377 @@
             auto end = endOfParagraph(lastPositionInOrAfterNode(element.ptr()));
 
+            if (start.isNull() || end.isNull())
+                continue;
+
             auto key = makeHashablePositionRange(start, end);
             if (!paragraphSets.add(key).isNewEntry)

trunk/Tools/ChangeLog

@@ +1 @@
+2020-04-09  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Add an API test for <https://trac.webkit.org/r259766>
+        https://bugs.webkit.org/show_bug.cgi?id=210294
+
+        Reviewed by Tim Horton.
+
+        Exercise the pathological case fixed in r259766 by inserting and then immediately removing a paragraph element
+        after starting text manipulation. This test also revealed an existing issue in TextManipulationController, where
+        we will end up hitting a debug assertion when trying to insert { null position, null position } into a HashMap
+        underneath `TextManipulationController::scheduleObservartionUpdate`.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:
+        (TestWebKitAPI::TEST):
+
 2020-04-09  Simon Fraser  <simon.fraser@apple.com>
 

trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm

@@ -844 +844 @@
 }
 
+TEST(TextManipulation, CompleteTextManipulationAvoidCrashingWhenContentIsRemoved)
+{
+    auto delegate = adoptNS([[TextManipulationDelegate alloc] init]);
+    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 400, 400)]);
+    [webView _setTextManipulationDelegate:delegate.get()];
+
+    [webView synchronouslyLoadTestPageNamed:@"simple"];
+
+    done = false;
+    [webView _startTextManipulationsWithConfiguration:nil completion:^{
+        done = true;
+    }];
+    TestWebKitAPI::Util::run(&done);
+
+    auto *items = [delegate items];
+    EXPECT_EQ(items.count, 1UL);
+    auto *tokens = items[0].tokens;
+    EXPECT_EQ(tokens.count, 1UL);
+
+    __block bool done = false;
+    [webView performAfterReceivingMessage:@"DoneRemovingParagraph" action:^{
+        done = true;
+    }];
+
+    [webView stringByEvaluatingJavaScript:
+        @"const paragraph = document.createElement('p');"
+        "paragraph.textContent = 'Hello world';"
+        "document.body.appendChild(paragraph);"
+        "setTimeout(() => { paragraph.remove(); webkit.messageHandlers.testHandler.postMessage('DoneRemovingParagraph') })"];
+
+    done = false;
+    [webView _completeTextManipulationForItems:@[(_WKTextManipulationItem *)createItem(items[0].identifier, {
+        { tokens[0].identifier, @"Simple HTML file!" },
+    })] completion:^(NSArray<NSError *> *errors) {
+        EXPECT_EQ(errors, nil);
+        done = true;
+    }];
+
+    EXPECT_WK_STREQ("Simple HTML file!", [webView stringByEvaluatingJavaScript:@"document.body.textContent"]);
+}
+
 TEST(TextManipulation, CompleteTextManipulationShouldPreserveImagesAsExcludedTokens)
 {