Changeset 247342 in webkit

Timestamp:

Jul 10, 2019 9:09:52 PM (5 years ago)

Author:

timothy_horton@apple.com

Message:

Long pressing on attachments will crash the WebContent process
​https://bugs.webkit.org/show_bug.cgi?id=199696
<rdar://problem/52920241>

Reviewed by Dean Jackson.

Source/WebKit:

• WebProcess/WebPage/ios/WebPageIOS.mm:

(WebKit::linkIndicatorPositionInformation):
(WebKit::elementPositionInformation):
(WebKit::selectionPositionInformation):
(WebKit::WebPage::positionInformation):
Instead of one-off creating a node snapshot for <attachment>, just
use TextIndicator. This way, we get an estimated background color,
paint at the right resolution, etc.

Also, hitNode was often null where we were previously calling
shareableBitmapSnapshotForNode, because it depends on the element
having click event handlers. selectionPositionInformation() re-hit-tests
more permissively to find the <attachment>, so moving this code
inside that function ensures that we don't try to snapshot a null node.

Tools:

• TestWebKitAPI/Tests/WebKitCocoa/WKRequestActivatedElementInfo.mm:

(TestWebKitAPI::TEST):
Add a test that previously crashed.

Location:

trunk

Files:

• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm (modified) (4 diffs)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/WKRequestActivatedElementInfo.mm (modified) (2 diffs)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-07-10  Tim Horton  <timothy_horton@apple.com>
+
+        Long pressing on attachments will crash the WebContent process
+        https://bugs.webkit.org/show_bug.cgi?id=199696
+        <rdar://problem/52920241>
+
+        Reviewed by Dean Jackson.
+
+        * WebProcess/WebPage/ios/WebPageIOS.mm:
+        (WebKit::linkIndicatorPositionInformation):
+        (WebKit::elementPositionInformation):
+        (WebKit::selectionPositionInformation):
+        (WebKit::WebPage::positionInformation):
+        Instead of one-off creating a node snapshot for <attachment>, just
+        use TextIndicator. This way, we get an estimated background color,
+        paint at the right resolution, etc.
+
+        Also, hitNode was often null where we were previously calling
+        shareableBitmapSnapshotForNode, because it depends on the element
+        having click event handlers. selectionPositionInformation() re-hit-tests
+        more permissively to find the <attachment>, so moving this code
+        inside that function ensures that we don't try to snapshot a null node.
+
 2019-07-10  Dean Jackson  <dino@apple.com>
 

trunk/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm

@@ -2524 +2524 @@
 }
 
-static void linkIndicatorPositionInformation(WebPage& page, Element& element, Element& linkElement, const InteractionInformationRequest& request, InteractionInformationAtPosition& info)
+static void linkIndicatorPositionInformation(WebPage& page, Element& linkElement, const InteractionInformationRequest& request, InteractionInformationAtPosition& info)
 {
     if (!request.includeLinkIndicator)
@@ -2636 +2636 @@
         info.url = linkElement->document().completeURL(stripLeadingAndTrailingHTMLSpaces(linkElement->getAttribute(HTMLNames::hrefAttr)));
 
-        linkIndicatorPositionInformation(page, element, *linkElement, request, info);
+        linkIndicatorPositionInformation(page, *linkElement, request, info);
 #if ENABLE(DATA_DETECTION)
         dataDetectorLinkPositionInformation(element, info);
@@ -2663 +2663 @@
     if (is<HTMLAttachmentElement>(*hitNode)) {
         info.isAttachment = true;
-        const HTMLAttachmentElement& attachment = downcast<HTMLAttachmentElement>(*hitNode);
+        HTMLAttachmentElement& attachment = downcast<HTMLAttachmentElement>(*hitNode);
         info.title = attachment.attachmentTitle();
+        linkIndicatorPositionInformation(page, attachment, request, info);
         if (attachment.file())
             info.url = URL::fileURLWithFileSystemPath(downcast<HTMLAttachmentElement>(*hitNode).file()->path());
@@ -2726 +2727 @@
     }
 
-    if (!(info.isLink || info.isImage)) {
+    if (!(info.isLink || info.isImage))
         selectionPositionInformation(*this, request, info);
-
-        if (info.isAttachment && request.includeSnapshot) {
-            Element& element = downcast<Element>(*hitNode);
-            info.image = shareableBitmapSnapshotForNode(element);
-        }
-    }
 
     // Prevent the callout bar from showing when tapping on the datalist button.

trunk/Tools/ChangeLog

@@ +1 @@
+2019-07-10  Tim Horton  <timothy_horton@apple.com>
+
+        Long pressing on attachments will crash the WebContent process
+        https://bugs.webkit.org/show_bug.cgi?id=199696
+        <rdar://problem/52920241>
+
+        Reviewed by Dean Jackson.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/WKRequestActivatedElementInfo.mm:
+        (TestWebKitAPI::TEST):
+        Add a test that previously crashed.
+
 2019-07-10  Dean Jackson  <dino@apple.com>
 

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

@@ -31 +31 @@
 #import "TestWKWebView.h"
 #import <WebKit/WKWebView.h>
+#import <WebKit/WKWebViewConfigurationPrivate.h>
 #import <WebKit/_WKActivatedElementInfo.h>
 #import <wtf/RetainPtr.h>
@@ -135 +136 @@
 }
 
+TEST(WebKit, RequestActivatedElementInfoForAttachment)
+{
+    auto configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
+    [configuration _setAttachmentElementEnabled:YES];
+
+    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500) configuration:configuration.get()]);
+    [webView loadHTMLString:@"<html><head><meta name='viewport' content='initial-scale=1'></head><body style='margin: 0px;'><attachment  /></body></html>" baseURL:nil];
+    [webView _test_waitForDidFinishNavigation];
+
+    __block bool finished = false;
+    [webView _requestActivatedElementAtPosition:CGPointMake(20, 20) completionBlock:^(_WKActivatedElementInfo *elementInfo) {
+        EXPECT_TRUE(elementInfo.type == _WKActivatedElementTypeAttachment);
+
+        finished = true;
+    }];
+
+    TestWebKitAPI::Util::run(&finished);
+}
+
 TEST(WebKit, RequestActivatedElementInfoWithNestedSynchronousUpdates)
 {