[webkit-reviews] review granted: [Bug 213702] Add handling for a case of OOME in CSSTokenizer and CSSParser. : [Attachment 403136] proposed patch.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 213702: Add handling for a case of OOME in CSSTokenizer and CSSParser.
https://bugs.webkit.org/show_bug.cgi?id=213702

Attachment 403136: proposed patch.

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




--- Comment #9 from Darin Adler <darin at apple.com> ---
Comment on attachment 403136
  --> https://bugs.webkit.org/attachment.cgi?id=403136
proposed patch.

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

I like this, better approach.

> Source/WebCore/css/parser/CSSParser.cpp:93
>      CSSParserImpl parser(m_context, condition);
> +    if (!parser.tokenizer())
> +	   return false;

Is this the only place we construct a CSSParserImpl?

> Source/WebCore/css/parser/CSSTokenizer.cpp:54
> +    auto tokenizer = makeUnique<CSSTokenizer>(string, &success);

I think this should use (preprocessString(string), &wrapper, nullptr) so it
uses the inner constructor.

> Source/WebCore/css/parser/CSSTokenizer.cpp:63
> +    auto tokenizer = makeUnique<CSSTokenizer>(string, wrapper, &success);

I think this should use (preprocessString(string), &wrapper, &success) so it
uses the inner constructor.

> Source/WebCore/css/parser/CSSTokenizer.cpp:79
> +inline CSSTokenizer::CSSTokenizer(String&& string, CSSParserObserverWrapper*
wrapper, bool* constructionSuccessPtr)

Should get rid of this "inline"; does no good. Doesn’t even affect inlining.

> Source/WebCore/css/parser/CSSTokenizer.cpp:84
> +    bool localConstructionSuccess;
> +    bool& constructionSuccess = constructionSuccessPtr ?
*constructionSuccessPtr : localConstructionSuccess;
> +    constructionSuccess = true;

This is unnecessarily complicated; the local variable doesn’t make the code any
clearer. Just write:

    if (constructionSuccessPtr)
	*constructionSuccessPtr = true;

And use constructionSuccessPtr below, in the two places it's used, right after
the assertions.

> Source/WebCore/css/parser/CSSTokenizer.cpp:93
> +	   constexpr bool reserveInitialCapacitySucceeded = false;
> +	   RELEASE_ASSERT(constructionSuccessPtr ||
reserveInitialCapacitySucceeded);

Here’s how I would write this:

    // When constructionSuccessPtr is null, our policy is to crash on failure.
    RELEASE_ASSERT(constructionSuccessPtr);

The strange constexpr boolean does not work for me as a comment.

> Source/WebCore/css/parser/CSSTokenizer.cpp:109
> +		   constexpr bool appendSucceeded = false;
> +		   RELEASE_ASSERT(constructionSuccessPtr || appendSucceeded);

Ditto.

> Source/WebCore/css/parser/CSSTokenizer.h:49
> +    static std::unique_ptr<CSSTokenizer> create(const String&);
> +    static std::unique_ptr<CSSTokenizer> create(const String&,
CSSParserObserverWrapper&); // For the inspector

I guess since the default is you can just construct these, then these should be
called tryCreate.

> Source/WebCore/css/parser/CSSTokenizer.h:52
> +    explicit CSSTokenizer(const String&, bool* constructionSuccess =
nullptr);
> +    CSSTokenizer(const String&, CSSParserObserverWrapper&, bool*
constructionSuccess = nullptr); // For the inspector

I suggest not adding bool* constructionSuccess to these. We don’t want that to
be public. The tryCreate functions take care of that. And it’s simple to modify
the implementation of the tryCreate functions so they use the private
constructor.