- Revision
- 226539
- Author
- [email protected]
- Date
- 2018-01-08 14:19:15 -0800 (Mon, 08 Jan 2018)
Log Message
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.
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):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (226538 => 226539)
--- trunk/Source/WebCore/ChangeLog 2018-01-08 22:13:40 UTC (rev 226538)
+++ trunk/Source/WebCore/ChangeLog 2018-01-08 22:19:15 UTC (rev 226539)
@@ -1,3 +1,64 @@
+2018-01-08 Wenson Hsieh <[email protected]>
+
+ 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 <[email protected]>
[RenderTreeBuilder] Move RenderBlockFlow addChild logic to RenderTreeBuilder
Modified: trunk/Source/WebCore/dom/Document.cpp (226538 => 226539)
--- trunk/Source/WebCore/dom/Document.cpp 2018-01-08 22:13:40 UTC (rev 226538)
+++ trunk/Source/WebCore/dom/Document.cpp 2018-01-08 22:19:15 UTC (rev 226539)
@@ -7600,8 +7600,10 @@
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);
Modified: trunk/Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm (226538 => 226539)
--- trunk/Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm 2018-01-08 22:13:40 UTC (rev 226538)
+++ trunk/Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm 2018-01-08 22:19:15 UTC (rev 226539)
@@ -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 @@
{
#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 @@
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 @@
#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;
Modified: trunk/Source/WebCore/editing/markup.cpp (226538 => 226539)
--- trunk/Source/WebCore/editing/markup.cpp 2018-01-08 22:13:40 UTC (rev 226538)
+++ trunk/Source/WebCore/editing/markup.cpp 2018-01-08 22:19:15 UTC (rev 226539)
@@ -378,6 +378,7 @@
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,6 +768,8 @@
attachments.append(attachment);
for (auto& attachment : attachments) {
+ attachment->setUniqueIdentifier(attachment->attributeWithoutSynchronization(webkitattachmentidAttr));
+
auto attachmentPath = attachment->attachmentPath();
auto blobURL = attachment->blobURL();
if (!attachmentPath.isEmpty())
@@ -773,6 +776,8 @@
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);
}
Modified: trunk/Source/WebCore/html/HTMLAttachmentElement.cpp (226538 => 226539)
--- trunk/Source/WebCore/html/HTMLAttachmentElement.cpp 2018-01-08 22:13:40 UTC (rev 226538)
+++ trunk/Source/WebCore/html/HTMLAttachmentElement.cpp 2018-01-08 22:19:15 UTC (rev 226539)
@@ -182,16 +182,6 @@
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);
Modified: trunk/Source/WebCore/html/HTMLAttachmentElement.h (226538 => 226539)
--- trunk/Source/WebCore/html/HTMLAttachmentElement.h 2018-01-08 22:13:40 UTC (rev 226538)
+++ trunk/Source/WebCore/html/HTMLAttachmentElement.h 2018-01-08 22:19:15 UTC (rev 226539)
@@ -49,8 +49,8 @@
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 @@
RefPtr<File> m_file;
Vector<std::unique_ptr<AttachmentDataReader>> m_attachmentReaders;
+ String m_uniqueIdentifier;
};
} // namespace WebCore
Modified: trunk/Tools/ChangeLog (226538 => 226539)
--- trunk/Tools/ChangeLog 2018-01-08 22:13:40 UTC (rev 226538)
+++ trunk/Tools/ChangeLog 2018-01-08 22:19:15 UTC (rev 226539)
@@ -1,3 +1,18 @@
+2018-01-08 Wenson Hsieh <[email protected]>
+
+ 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 <[email protected]>
navigator.onLine does not work inside service workers
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm (226538 => 226539)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm 2018-01-08 22:13:40 UTC (rev 226538)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm 2018-01-08 22:19:15 UTC (rev 226539)
@@ -447,6 +447,7 @@
[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 @@
[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 @@
[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 @@
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 @@
[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 @@
EXPECT_WK_STREQ("This is an apple", [webView stringByEvaluatingJavaScript:@"document.body.textContent"]);
}
+TEST(WKAttachmentTests, InsertAndRemoveDuplicateAttachment)
+{
+ auto webView = webViewForTestingAttachments();
+ RetainPtr<NSData> data = ""
+ 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)
