Copying, pasting, and then deleting an attachment element breaks attachment data...
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 8 Jan 2018 22:19:15 +0000 (22:19 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 8 Jan 2018 22:19:15 +0000 (22:19 +0000)
https://bugs.webkit.org/show_bug.cgi?id=181365
<rdar://problem/36340647>

Reviewed by Tim Horton.

Source/WebCore:

Currently, copying and pasting an attachment element within the same document and then deleting backwards to
remove the pasted attachment element causes the original attachment element to be inaccessible via SPI. This is
because there are now two different attachment elements with the same unique identifier, such that Document,
which keeps a map of all unique attachment identifiers to attachment elements, will lose track of the original
attachment element.

To fix this, we ensure that attachment elements should always have unique identifiers when they are inserted
into the document. We make several small adjustments to accomplish this:

1.  First, refactor HTMLAttachmentElement's unique identifier so that it no longer depends on the value of the
    "webkitattachmentid" attribute, and is instead just a member of HTMLAttachmentElement that is not exposed to
    DOM bindings. This means setting and querying an attachment element's uniqueIdentifier can be done without
    triggering any side effects, such as layout or mutation events.

2.  Next, make "webkitattachmentid" a temporary attribute similar to "webkitattachmentpath" and
    "webkitattachmentbloburl", so that it is added only when generating a markup fragment for editing, and
    removed upon deserialization.

3.  Lastly, shift the responsibility of assigning a unique identifier to an attachment away from places where we
    create attachment elements, and instead have Document enforce this when an attachment element is inserted.

Tests:  WKAttachmentTests.InsertAndRemoveDuplicateAttachment
        WKAttachmentTests.InsertDuplicateAttachmentAndUpdateData

* dom/Document.cpp:
(WebCore::Document::didInsertAttachmentElement):

Assign the unique identifier of an attachment element that has been inserted. If the identifier already tracks
an existing attachment element in the document or is missing, reassign the identifier to a new value.

* editing/cocoa/WebContentReaderCocoa.mm:
(WebCore::createFragmentForImageAttachment):
(WebCore::replaceRichContentWithAttachments):
(WebCore::WebContentReader::readFilePaths):

Remove calls to setUniqueIdentifier here, since Document will assign a unique identifier upon insertion.

* editing/markup.cpp:
(WebCore::StyledMarkupAccumulator::appendCustomAttributes):
(WebCore::createFragmentFromMarkup):

Set the attachment's unique identifier to the value of the "webkitattachmentid" attribute. When moving existing
attachments around in the DOM without duplication, this ensures that the attachment will be removed and
reinserted in the document without triggering removal and insertion client delegate methods.

When pasting an attachment element that has the same identifier as an existing attachment, we let Document
realize that the attachment identifier already exists, and reassign it to a unique value.

* html/HTMLAttachmentElement.cpp:
(WebCore::HTMLAttachmentElement::uniqueIdentifier const): Deleted.
(WebCore::HTMLAttachmentElement::setUniqueIdentifier): Deleted.
* html/HTMLAttachmentElement.h:

Tools:

Adds two new attachment API tests to verify that copying and pasting an existing attachment inserts an
attachment element that may be edited independently of the original attachment. See WebCore/ChangeLog for more
detail.

* TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm:
(TestWebKitAPI::TEST):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@226539 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm
Source/WebCore/editing/markup.cpp
Source/WebCore/html/HTMLAttachmentElement.cpp
Source/WebCore/html/HTMLAttachmentElement.h
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm

index 9372047..7aff8cf 100644 (file)
@@ -1,3 +1,64 @@
+2018-01-08  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Copying, pasting, and then deleting an attachment element breaks attachment data requests
+        https://bugs.webkit.org/show_bug.cgi?id=181365
+        <rdar://problem/36340647>
+
+        Reviewed by Tim Horton.
+
+        Currently, copying and pasting an attachment element within the same document and then deleting backwards to
+        remove the pasted attachment element causes the original attachment element to be inaccessible via SPI. This is
+        because there are now two different attachment elements with the same unique identifier, such that Document,
+        which keeps a map of all unique attachment identifiers to attachment elements, will lose track of the original
+        attachment element.
+
+        To fix this, we ensure that attachment elements should always have unique identifiers when they are inserted
+        into the document. We make several small adjustments to accomplish this:
+
+        1.  First, refactor HTMLAttachmentElement's unique identifier so that it no longer depends on the value of the
+            "webkitattachmentid" attribute, and is instead just a member of HTMLAttachmentElement that is not exposed to
+            DOM bindings. This means setting and querying an attachment element's uniqueIdentifier can be done without
+            triggering any side effects, such as layout or mutation events.
+
+        2.  Next, make "webkitattachmentid" a temporary attribute similar to "webkitattachmentpath" and
+            "webkitattachmentbloburl", so that it is added only when generating a markup fragment for editing, and
+            removed upon deserialization.
+
+        3.  Lastly, shift the responsibility of assigning a unique identifier to an attachment away from places where we
+            create attachment elements, and instead have Document enforce this when an attachment element is inserted.
+
+        Tests:  WKAttachmentTests.InsertAndRemoveDuplicateAttachment
+                WKAttachmentTests.InsertDuplicateAttachmentAndUpdateData
+
+        * dom/Document.cpp:
+        (WebCore::Document::didInsertAttachmentElement):
+
+        Assign the unique identifier of an attachment element that has been inserted. If the identifier already tracks
+        an existing attachment element in the document or is missing, reassign the identifier to a new value.
+
+        * editing/cocoa/WebContentReaderCocoa.mm:
+        (WebCore::createFragmentForImageAttachment):
+        (WebCore::replaceRichContentWithAttachments):
+        (WebCore::WebContentReader::readFilePaths):
+
+        Remove calls to setUniqueIdentifier here, since Document will assign a unique identifier upon insertion.
+
+        * editing/markup.cpp:
+        (WebCore::StyledMarkupAccumulator::appendCustomAttributes):
+        (WebCore::createFragmentFromMarkup):
+
+        Set the attachment's unique identifier to the value of the "webkitattachmentid" attribute. When moving existing
+        attachments around in the DOM without duplication, this ensures that the attachment will be removed and
+        reinserted in the document without triggering removal and insertion client delegate methods.
+
+        When pasting an attachment element that has the same identifier as an existing attachment, we let Document
+        realize that the attachment identifier already exists, and reassign it to a unique value.
+
+        * html/HTMLAttachmentElement.cpp:
+        (WebCore::HTMLAttachmentElement::uniqueIdentifier const): Deleted.
+        (WebCore::HTMLAttachmentElement::setUniqueIdentifier): Deleted.
+        * html/HTMLAttachmentElement.h:
+
 2018-01-08  Zalan Bujtas  <zalan@apple.com>
 
         [RenderTreeBuilder] Move RenderBlockFlow addChild logic to RenderTreeBuilder
index 7d2f9d2..62ce513 100644 (file)
@@ -7600,8 +7600,10 @@ Vector<RefPtr<WebAnimation>> Document::getAnimations()
 void Document::didInsertAttachmentElement(HTMLAttachmentElement& attachment)
 {
     auto identifier = attachment.uniqueIdentifier();
-    if (!identifier)
-        return;
+    if (identifier.isEmpty() || m_attachmentIdentifierToElementMap.contains(identifier)) {
+        identifier = createCanonicalUUIDString();
+        attachment.setUniqueIdentifier(identifier);
+    }
 
     m_attachmentIdentifierToElementMap.set(identifier, attachment);
 
index 204903d..1b9ae3f 100644 (file)
@@ -59,7 +59,6 @@
 #import "markup.h"
 #import <pal/spi/cocoa/NSAttributedStringSPI.h>
 #import <wtf/SoftLinking.h>
-#import <wtf/UUID.h>
 
 #if (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300)
 @interface NSAttributedString ()
@@ -194,7 +193,6 @@ static Ref<DocumentFragment> createFragmentForImageAttachment(Document& document
 {
 #if ENABLE(ATTACHMENT_ELEMENT)
     auto attachment = HTMLAttachmentElement::create(HTMLNames::attachmentTag, document);
-    attachment->setUniqueIdentifier(createCanonicalUUIDString());
     attachment->setFile(File::create(blob, AtomicString("image")), HTMLAttachmentElement::UpdateDisplayAttributes::Yes);
     attachment->updateDisplayMode(AttachmentDisplayMode::InPlace);
 
@@ -275,7 +273,6 @@ static void replaceRichContentWithAttachments(DocumentFragment& fragment, const
             continue;
 
         auto attachment = HTMLAttachmentElement::create(HTMLNames::attachmentTag, fragment.document());
-        attachment->setUniqueIdentifier(createCanonicalUUIDString());
         attachment->setFile(WTFMove(file), HTMLAttachmentElement::UpdateDisplayAttributes::Yes);
         attachment->updateDisplayMode(info.displayMode);
         parent->replaceChild(attachment, elementToReplace);
@@ -607,7 +604,6 @@ bool WebContentReader::readFilePaths(const Vector<String>& paths)
 #if ENABLE(ATTACHMENT_ELEMENT)
         if (RuntimeEnabledFeatures::sharedFeatures().attachmentElementEnabled()) {
             auto attachment = HTMLAttachmentElement::create(HTMLNames::attachmentTag, document);
-            attachment->setUniqueIdentifier(createCanonicalUUIDString());
             attachment->setFile(File::create(path), HTMLAttachmentElement::UpdateDisplayAttributes::Yes);
             ensureFragment().appendChild(attachment);
             readAnyFilePath = true;
index 6a6f765..7b3b193 100644 (file)
@@ -378,6 +378,7 @@ void StyledMarkupAccumulator::appendCustomAttributes(StringBuilder& out, const E
         return;
     
     const HTMLAttachmentElement& attachment = downcast<HTMLAttachmentElement>(element);
+    appendAttribute(out, element, { webkitattachmentidAttr, attachment.uniqueIdentifier() }, namespaces);
     if (auto* file = attachment.file()) {
         // These attributes are only intended for File deserialization, and are removed from the generated attachment
         // element after we've deserialized and set its backing File.
@@ -767,12 +768,16 @@ Ref<DocumentFragment> createFragmentFromMarkup(Document& document, const String&
         attachments.append(attachment);
 
     for (auto& attachment : attachments) {
+        attachment->setUniqueIdentifier(attachment->attributeWithoutSynchronization(webkitattachmentidAttr));
+
         auto attachmentPath = attachment->attachmentPath();
         auto blobURL = attachment->blobURL();
         if (!attachmentPath.isEmpty())
             attachment->setFile(File::create(attachmentPath));
         else if (!blobURL.isEmpty())
             attachment->setFile(File::deserialize({ }, blobURL, attachment->attachmentType(), attachment->attachmentTitle()));
+
+        attachment->removeAttribute(webkitattachmentidAttr);
         attachment->removeAttribute(webkitattachmentpathAttr);
         attachment->removeAttribute(webkitattachmentbloburlAttr);
     }
index 1f445e8..d3708fa 100644 (file)
@@ -182,16 +182,6 @@ void HTMLAttachmentElement::parseAttribute(const QualifiedName& name, const Atom
     HTMLElement::parseAttribute(name, value);
 }
 
-String HTMLAttachmentElement::uniqueIdentifier() const
-{
-    return attributeWithoutSynchronization(HTMLNames::webkitattachmentidAttr);
-}
-
-void HTMLAttachmentElement::setUniqueIdentifier(const String& identifier)
-{
-    setAttributeWithoutSynchronization(HTMLNames::webkitattachmentidAttr, identifier);
-}
-
 String HTMLAttachmentElement::attachmentTitle() const
 {
     auto& title = attributeWithoutSynchronization(titleAttr);
index 1795d1f..b3223fa 100644 (file)
@@ -49,8 +49,8 @@ public:
     enum class UpdateDisplayAttributes { No, Yes };
     void setFile(RefPtr<File>&&, UpdateDisplayAttributes = UpdateDisplayAttributes::No);
 
-    WEBCORE_EXPORT String uniqueIdentifier() const;
-    void setUniqueIdentifier(const String&);
+    String uniqueIdentifier() const { return m_uniqueIdentifier; }
+    void setUniqueIdentifier(const String& uniqueIdentifier) { m_uniqueIdentifier = uniqueIdentifier; }
 
     WEBCORE_EXPORT void updateDisplayMode(AttachmentDisplayMode);
     WEBCORE_EXPORT void updateFileWithData(Ref<SharedBuffer>&& data, std::optional<String>&& newContentType = std::nullopt, std::optional<String>&& newFilename = std::nullopt);
@@ -100,6 +100,7 @@ private:
     
     RefPtr<File> m_file;
     Vector<std::unique_ptr<AttachmentDataReader>> m_attachmentReaders;
+    String m_uniqueIdentifier;
 };
 
 } // namespace WebCore
index 9c687f0..d2c68ba 100644 (file)
@@ -1,3 +1,18 @@
+2018-01-08  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Copying, pasting, and then deleting an attachment element breaks attachment data requests
+        https://bugs.webkit.org/show_bug.cgi?id=181365
+        <rdar://problem/36340647>
+
+        Reviewed by Tim Horton.
+
+        Adds two new attachment API tests to verify that copying and pasting an existing attachment inserts an
+        attachment element that may be edited independently of the original attachment. See WebCore/ChangeLog for more
+        detail.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm:
+        (TestWebKitAPI::TEST):
+
 2018-01-08  Youenn Fablet  <youenn@apple.com>
 
         navigator.onLine does not work inside service workers
index 743547d..6b0f055 100644 (file)
@@ -447,6 +447,7 @@ TEST(WKAttachmentTests, AttachmentElementInsertion)
     [secondAttachment expectRequestedDataToBe:testImageData()];
     EXPECT_FALSE([webView hasAttribute:@"webkitattachmentbloburl" forQuerySelector:@"attachment"]);
     EXPECT_FALSE([webView hasAttribute:@"webkitattachmentpath" forQuerySelector:@"attachment"]);
+    EXPECT_FALSE([webView hasAttribute:@"webkitattachmentid" forQuerySelector:@"attachment"]);
 }
 
 TEST(WKAttachmentTests, AttachmentUpdatesWhenInsertingAndDeletingNewline)
@@ -514,6 +515,7 @@ TEST(WKAttachmentTests, AttachmentUpdatesWhenChangingFontStyles)
     [attachment expectRequestedDataToBe:testHTMLData()];
     EXPECT_FALSE([webView hasAttribute:@"webkitattachmentbloburl" forQuerySelector:@"attachment"]);
     EXPECT_FALSE([webView hasAttribute:@"webkitattachmentpath" forQuerySelector:@"attachment"]);
+    EXPECT_FALSE([webView hasAttribute:@"webkitattachmentid" forQuerySelector:@"attachment"]);
 
     // Inserting text should delete the current selection, removing the attachment in the process.
     [webView expectUpdatesAfterCommand:@"InsertText" withArgument:@"foo" expectedRemovals:@[attachment.get()] expectedInsertions:@[]];
@@ -537,6 +539,7 @@ TEST(WKAttachmentTests, AttachmentUpdatesWhenInsertingLists)
     [attachment expectRequestedDataToBe:testHTMLData()];
     EXPECT_FALSE([webView hasAttribute:@"webkitattachmentbloburl" forQuerySelector:@"attachment"]);
     EXPECT_FALSE([webView hasAttribute:@"webkitattachmentpath" forQuerySelector:@"attachment"]);
+    EXPECT_FALSE([webView hasAttribute:@"webkitattachmentid" forQuerySelector:@"attachment"]);
 }
 
 TEST(WKAttachmentTests, AttachmentUpdatesWhenInsertingRichMarkup)
@@ -545,12 +548,13 @@ TEST(WKAttachmentTests, AttachmentUpdatesWhenInsertingRichMarkup)
     RetainPtr<_WKAttachment> attachment;
     {
         ObserveAttachmentUpdatesForScope observer(webView.get());
-        [webView _synchronouslyExecuteEditCommand:@"InsertHTML" argument:@"<div><strong><attachment title='a' webkitattachmentid='a06fec41-9aa0-4c2c-ba3a-0149b54aad99'></attachment></strong></div>"];
+        [webView _synchronouslyExecuteEditCommand:@"InsertHTML" argument:@"<div><strong><attachment title='a'></attachment></strong></div>"];
         attachment = observer.observer().inserted[0];
         observer.expectAttachmentUpdates(@[ ], @[attachment.get()]);
     }
     EXPECT_FALSE([webView hasAttribute:@"webkitattachmentbloburl" forQuerySelector:@"attachment"]);
     EXPECT_FALSE([webView hasAttribute:@"webkitattachmentpath" forQuerySelector:@"attachment"]);
+    EXPECT_FALSE([webView hasAttribute:@"webkitattachmentid" forQuerySelector:@"attachment"]);
     {
         ObserveAttachmentUpdatesForScope observer(webView.get());
         [webView stringByEvaluatingJavaScript:@"document.querySelector('attachment').remove()"];
@@ -585,6 +589,7 @@ TEST(WKAttachmentTests, AttachmentUpdatesWhenCuttingAndPasting)
     [attachment expectRequestedDataToBe:testHTMLData()];
     EXPECT_FALSE([webView hasAttribute:@"webkitattachmentbloburl" forQuerySelector:@"attachment"]);
     EXPECT_FALSE([webView hasAttribute:@"webkitattachmentpath" forQuerySelector:@"attachment"]);
+    EXPECT_FALSE([webView hasAttribute:@"webkitattachmentid" forQuerySelector:@"attachment"]);
 }
 
 TEST(WKAttachmentTests, AttachmentDataForEmptyFile)
@@ -925,6 +930,71 @@ TEST(WKAttachmentTests, DoNotInsertDataURLImagesAsAttachments)
     EXPECT_WK_STREQ("This is an apple", [webView stringByEvaluatingJavaScript:@"document.body.textContent"]);
 }
 
+TEST(WKAttachmentTests, InsertAndRemoveDuplicateAttachment)
+{
+    auto webView = webViewForTestingAttachments();
+    RetainPtr<NSData> data = testHTMLData();
+    RetainPtr<_WKAttachment> originalAttachment;
+    RetainPtr<_WKAttachment> pastedAttachment;
+    {
+        ObserveAttachmentUpdatesForScope observer(webView.get());
+        originalAttachment = [webView synchronouslyInsertAttachmentWithFilename:@"foo.txt" contentType:@"text/plain" data:data.get() options:nil];
+        EXPECT_EQ(0U, observer.observer().removed.count);
+        observer.expectAttachmentUpdates(@[], @[originalAttachment.get()]);
+    }
+    [webView selectAll:nil];
+    [webView _executeEditCommand:@"Copy" argument:nil completion:nil];
+    [webView evaluateJavaScript:@"getSelection().collapseToEnd()" completionHandler:nil];
+    {
+        ObserveAttachmentUpdatesForScope observer(webView.get());
+        [webView _synchronouslyExecuteEditCommand:@"Paste" argument:nil];
+        EXPECT_EQ(0U, observer.observer().removed.count);
+        EXPECT_EQ(1U, observer.observer().inserted.count);
+        pastedAttachment = observer.observer().inserted.firstObject;
+        EXPECT_FALSE([pastedAttachment isEqual:originalAttachment.get()]);
+    }
+    {
+        ObserveAttachmentUpdatesForScope observer(webView.get());
+        [webView _synchronouslyExecuteEditCommand:@"DeleteBackward" argument:nil];
+        observer.expectAttachmentUpdates(@[pastedAttachment.get()], @[]);
+        [originalAttachment expectRequestedDataToBe:data.get()];
+    }
+    {
+        ObserveAttachmentUpdatesForScope observer(webView.get());
+        [webView _synchronouslyExecuteEditCommand:@"DeleteBackward" argument:nil];
+        observer.expectAttachmentUpdates(@[originalAttachment.get()], @[]);
+    }
+}
+
+TEST(WKAttachmentTests, InsertDuplicateAttachmentAndUpdateData)
+{
+    auto webView = webViewForTestingAttachments();
+    RetainPtr<NSData> originalData = testHTMLData();
+    RetainPtr<_WKAttachment> originalAttachment;
+    RetainPtr<_WKAttachment> pastedAttachment;
+    {
+        ObserveAttachmentUpdatesForScope observer(webView.get());
+        originalAttachment = [webView synchronouslyInsertAttachmentWithFilename:@"foo.txt" contentType:@"text/plain" data:originalData.get() options:nil];
+        EXPECT_EQ(0U, observer.observer().removed.count);
+        observer.expectAttachmentUpdates(@[], @[originalAttachment.get()]);
+    }
+    [webView selectAll:nil];
+    [webView _executeEditCommand:@"Copy" argument:nil completion:nil];
+    [webView evaluateJavaScript:@"getSelection().collapseToEnd()" completionHandler:nil];
+    {
+        ObserveAttachmentUpdatesForScope observer(webView.get());
+        [webView _synchronouslyExecuteEditCommand:@"Paste" argument:nil];
+        EXPECT_EQ(0U, observer.observer().removed.count);
+        EXPECT_EQ(1U, observer.observer().inserted.count);
+        pastedAttachment = observer.observer().inserted.firstObject;
+        EXPECT_FALSE([pastedAttachment isEqual:originalAttachment.get()]);
+    }
+    auto updatedData = retainPtr([@"HELLO WORLD" dataUsingEncoding:NSUTF8StringEncoding]);
+    [originalAttachment synchronouslySetData:updatedData.get() newContentType:nil newFilename:nil error:nil];
+    [originalAttachment expectRequestedDataToBe:updatedData.get()];
+    [pastedAttachment expectRequestedDataToBe:originalData.get()];
+}
+
 #pragma mark - Platform-specific tests
 
 #if PLATFORM(MAC)