[webkit-reviews] review granted: [Bug 205373] [Cocoa] GPU process should log IPC message name before crashing from invalid WebContent message : [Attachment 386007] Patch v4

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted David Kilzer (:ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 205373: [Cocoa] GPU process should log IPC message name before crashing
from invalid WebContent message
https://bugs.webkit.org/show_bug.cgi?id=205373

Attachment 386007: Patch v4

https://bugs.webkit.org/attachment.cgi?id=386007&action=review




--- Comment #13 from Darin Adler <darin at apple.com> ---
Comment on attachment 386007
  --> https://bugs.webkit.org/attachment.cgi?id=386007
Patch v4

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

Looks OK, but I suggest not landing it exactly as-is.

> Source/WTF/wtf/text/StringConcatenate.h:206
> +template<> class StringTypeAdapter<CString, void> : public
StringTypeAdapter<String, void> {
> +public:
> +    StringTypeAdapter(const CString& string)
> +	   : StringTypeAdapter<String, void> { String::fromUTF8(string) }
> +    {
> +    }
> +};

But also, I don’t think this should be included in this patch. The changed code
below uses StringReference concatenation, so this is a related enhancement for
possible future use, but not needed in this patch.

There are two minor but significant problems with this implementation:

1) This always unconditionally creates a WTF::String. That's not what we want
when processing, say, a makeString; the point of StringAdapter is to not create
intermediate WTF::String objects until the end of the entire stringifying
operation. We want something more like StringTypeAdapter<const char*, void>.

2) This treats the bytes in the CString as UTF-8. But the other string adapters
for char and LChar sequences treat characters as Latin-1. We don’t want CString
to be different in this respect. On the other hand, it may be useful in many
contexts to support UTF-8; might take a little thinking to figure out a way to
fit that in with StringAdapter cleanly. Among other things, UTF-8 support means
figuring out how to handle illegal UTF-8 sequences. That issue doesn’t come up
with Latin-1 since there's no such thing as "illegal" Latin-1.

I don’t see any harm in landing this, but we’d want to revisit it afterward to
fix the above issues. And I suggest landing it separately with test coverage.

> Source/WTF/wtf/text/WTFString.cpp:880
>  String String::fromUTF8(const CString& s)
>  {
> -    return fromUTF8(s.data());
> +    return fromUTF8(s.data(), s.length());
>  }

I don’t think this change should be included in this patch.

The change should only affect CString objects that are created for strings with
NUL characters embedded in them. I don’t see why this change is needed for this
patch. Seems a separate bug fix.

I suggest landing it separately with test coverage.

> Source/WebKit/Platform/IPC/StringReferenceConcatenate.h:39
> +template<> class StringTypeAdapter<IPC::StringReference, void> : public
StringTypeAdapter<CString, void> {
> +public:
> +    StringTypeAdapter(const IPC::StringReference& string)
> +	   : StringTypeAdapter<CString, void> { string.toString() }
> +    {
> +    }
> +};

This always unconditionally creates a WTF::String. That's not what we want when
processing, say, a makeString; the point of StringAdapter is to not create
intermediate WTF::String objects until the end of the entire stringifying
operation. We want something more like StringTypeAdapter<StringView, void>
(which can be found in StringView.h).

I don’t see any harm in landing this as is, but we’d want to revisit it
afterward to fix the issue above.

Also seems like we should include a test of this.