Bug 52236 – Strip NUL character when copying text on Windows

• Component: HTML Editing
• Status: RESOLVED FIXED

[Erik Arvidsson]

Windows does not allow NUL characters on the clipboard since they are used to terminate the clipboard data.

We should strip NUL characters from the text before sending the text to the windows clipboard.

[Erik Arvidsson]

Created attachment 78579 [details]
Test case

[Tony Chang]

Some testing:

On Chromium Linux, Chromium Win, Safari Win, we copy up to the NUL character (i.e., the copied text is truncated).

On Safari Mac and Chromium Mac, we ignore the NUL (when you paste, you get text without the NUL character).

On IE9, Firefox Win and Firefox Linux, the DOM has the unicode replacement character.

On Firefox Mac, the NUL is converted into a unicode replacement character (U+FFFD) in the pasted text.  Actually, it may already be replaced in the DOM but perhaps there's no glyph for it.

[Ryosuke Niwa]

(In reply to comment #2)
> On IE9, Firefox Win and Firefox Linux, the DOM has the unicode replacement character.

We should most certainly replace with the unicode replace character.

[Tony Chang]

(In reply to comment #3)
> (In reply to comment #2)
> > On IE9, Firefox Win and Firefox Linux, the DOM has the unicode replacement character.
> 
> We should most certainly replace with the unicode replace character.

Yup, that's what the HTML5 spec says:
http://www.whatwg.org/specs/web-apps/current-work/multipage/tokenization.html#plaintext-state

[Tony Chang]

Actually, arv says that if he changes the test case to use innerHTML, the NUL character is stripped (he'll upload a test case).

Seems like we need to have setting textContent go through the HTML parser and the HTML parser needs to insert the replacement character.

[Adam Barth]

> Seems like we need to have setting textContent go through the HTML parser and the HTML parser needs to insert the replacement character.

That would be surprising.  textContent is a DOM-level concept.  It should ignore anything HTML specific.  That said, stranger things have happened.

[Erik Arvidsson]

Created attachment 78601 [details]
Shows that NUL is stripped through the parser

[Alexey Proskuryakov]

HTML5 goes to great lengths trying to avoid having null characters in Unicode strings, but I don't think that it generally makes sense. Replacing null with U+FFFD during parsing is particularly weird, because one can always use DOM manipulation to insert a null (as it's done in attached test case), so there is no guarantee of "no null in DOM" anyway.

When copying, we should just make sure that the user gets expected results. As WebKit doesn't display nulls in content, they should be removed in copied text, not replaced with U+FFFD.

> Seems like we need to have setting textContent go through the HTML parser and the HTML parser needs to insert the replacement character.

I don't think that it's the right direction. There are many ways to insert a null via DOM manipulation, and even HTML5 doesn't prohibit those.

[Erik Arvidsson]

(In reply to comment #8)
> When copying, we should just make sure that the user gets expected results. As WebKit doesn't display nulls in content, they should be removed in copied text, not replaced with U+FFFD.

Agreed. That was how this started before it got side tracked...

[Tony Chang]

(In reply to comment #9)
> (In reply to comment #8)
> > When copying, we should just make sure that the user gets expected results. As WebKit doesn't display nulls in content, they should be removed in copied text, not replaced with U+FFFD.
> 
> Agreed. That was how this started before it got side tracked...

OK, patch coming for Chromium.  I don't have a windows box handy, so Safari Win will come later.

[Ryosuke Niwa]

(In reply to comment #10)
> OK, patch coming for Chromium.  I don't have a windows box handy, so Safari Win will come later.

Why don't we do this when we're serializing DOM instead of doing it in each platform?  It's a simple modification to MarkupAccumulator.

[Tony Chang]

Created attachment 78856 [details]
Patch

[Tony Chang]

(In reply to comment #12)
> Created an attachment (id=78856) [details]
> Patch

I talked to Ryosuke offline and the code for putting plain text on the clipboard doesn't go through MarkupAccumulator, it goes thorough Editor::selectedText().  Adding the code there fixes Chromium Linux, but I need to do some more testing on a Windows machine to see if we need to make a similar fix in MarkupAccumulator (for the rich text case).

[Ryosuke Niwa]

(In reply to comment #13)
> (In reply to comment #12)
> > Created an attachment (id=78856) [details] [details]
> > Patch
> 
> I talked to Ryosuke offline and the code for putting plain text on the clipboard doesn't go through MarkupAccumulator, it goes thorough Editor::selectedText().  Adding the code there fixes Chromium Linux, but I need to do some more testing on a Windows machine to see if we need to make a similar fix in MarkupAccumulator (for the rich text case).

Per discussion in person, Tony doesn't have access to a Windows machine, so I'll build & test his patch on my Windows machine.

[Ryosuke Niwa]

(In reply to comment #14)
> Per discussion in person, Tony doesn't have access to a Windows machine, so I'll build & test his patch on my Windows machine.

I confirmed that the bug does not produce on Windows with Tony's patch.

[Ryosuke Niwa]

Comment on attachment 78856 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78856&action=review

r=me provided the answers to the following two questions are both yes.

> LayoutTests/editing/pasteboard/copy-null-characters.html:27
> +    var source = document.getElementById("source");
> +    var textWithNull = "Copy\0 paste me";
> +    source.textContent = textWithNull;
> +    sel.setPosition(source, 0);
> +    document.execCommand("SelectAll");
> +    document.execCommand("Copy");
> +    // Remove the source text so we don't end up with a null character in the
> +    // expected output file.
> +    source.parentNode.removeChild(source);
> +
> +    var destinationRichText = document.getElementById("destination-rich-text");
> +    sel.setPosition(destinationRichText, 0);
> +    document.execCommand("Paste");
> +
> +    var destinationPlainText = document.getElementById("destination-plain-text");
> +    destinationPlainText.focus();
> +    document.execCommand("Paste");

Could you also add a test case where the copied contents are richly formatted?

> Source/WebCore/platform/mac/PasteboardMac.mm:194
> -        String text = selectedRange->text();
> +        String text = frame->editor()->selectedText();

If I'm understanding this right, you'd expect expect Range::text() to include null char because it's a DOM API. Am I right?

[Tony Chang]

Created attachment 79001 [details]
Patch for landing

[Tony Chang]

(In reply to comment #16)
> Could you also add a test case where the copied contents are richly formatted?

Done.

> > Source/WebCore/platform/mac/PasteboardMac.mm:194
> > -        String text = selectedRange->text();
> > +        String text = frame->editor()->selectedText();
> 
> If I'm understanding this right, you'd expect expect Range::text() to include null char because it's a DOM API. Am I right?

Yes, that's right.  This has the side benefit that now all platforms use the same code path to get the plain text that is placed on the clipboard/pasteboard.

[Alexey Proskuryakov]

Comment on attachment 79001 [details]
Patch for landing

+static String removeNULLCharacters(const String& str)
+{
+    String copy = str;
+    return copy.removeCharacters(&isNULLCharacter);
+}

Wow, that's a pretty slow way to remove nulls. Are we sure that we want to land it?

[Alexey Proskuryakov]

User-initiated copying may not be the hottest code, but one function call per character is still a lot.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 79001 [details]:

http/tests/appcache/update-cache.html bug 52483 (author: [e-mail])
The commit-queue is continuing to process your patch.

[Tony Chang]

(In reply to comment #20)
> User-initiated copying may not be the hottest code, but one function call per character is still a lot.

I suspect the compiler would inline the function, but if you prefer, I can update the patch to use StringImpl::replace(UChar, StringImpl*).

[Tony Chang]

(In reply to comment #22)
> (In reply to comment #20)
> > User-initiated copying may not be the hottest code, but one function call per character is still a lot.
> 
> I suspect the compiler would inline the function, but if you prefer, I can update the patch to use StringImpl::replace(UChar, StringImpl*).

I could also use StringImpl::createStrippingNullCharactersSlowCase.

[Alexey Proskuryakov]

> I suspect the compiler would inline the function, but if you prefer, I can update the patch to use StringImpl::replace(UChar, StringImpl*).

The compiler cannot inline the function, because String::removeCharacters() is not inline itself. The function's definition is not visible in StringImpl.cpp (which is in JavaScriptCore, so even cross-unit inlining wouldn't work).

[Maciej Stachowiak]

Better way to do it (untested):

static String removeNULLCharacters(const String& str)
{
    return str.replace(0, "");
}

[Ryosuke Niwa]

(In reply to comment #19)
> (From update of attachment 79001 [details])
> +static String removeNULLCharacters(const String& str)
> +{
> +    String copy = str;
> +    return copy.removeCharacters(&isNULLCharacter);
> +}
> 
> Wow, that's a pretty slow way to remove nulls. Are we sure that we want to land it?

Oops, I missed that.  Let's fix this before landing.

[Tony Chang]

Created attachment 79021 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 79021 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79021&action=review

> Source/WebCore/editing/Editor.cpp:133
> +static void removeNULLCharacters(String& str)
> +{
> +    str.replace(0, "");
> +}

I didn't comment about the function name, because I assumed that you'd just inline str.replace(0, "") at call site. But NULL isn't great, s it usually refers to C null pointer macro, which is not a single byte, and which we don't use anyway.

[Tony Chang]

Created attachment 79039 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 79039 [details]
Patch for landing

Clearing flags on attachment: 79039

Committed r75861: <http://trac.webkit.org/changeset/75861>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.