[webkit-reviews] review granted: [Bug 174966] Add WKContentRuleList action to add a header to a request : [Attachment 316695] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 174966: Add WKContentRuleList action to add a header to a request
https://bugs.webkit.org/show_bug.cgi?id=174966

Attachment 316695: Patch

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




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

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

> Source/WebCore/contentextensions/ContentExtensionActions.h:48
> +    AddHeader,

Terminology here isn’t great. This adds a header field value, possibly creating
a new header field or possible appending to an existing header field. The
header is the name for the thing that contains all the header fields plus a few
other things.

> Source/WebCore/contentextensions/ContentExtensionActions.h:56
> +    Vector<std::pair<String, String>> headersToAdd;

headerFieldValuesToAdd maybe

> Source/WebCore/contentextensions/ContentExtensionCompiler.cpp:59
> +    actions.grow(actions.size() + sizeof(uint32_t));
> +    *reinterpret_cast<uint32_t*>(&actions[actions.size() -
sizeof(uint32_t)]) = stringLength;

Do we really have to do it like this with a reinterpret_cast? That’s not
normally how we serialize multiple bytes elsewhere.

> Source/WebCore/contentextensions/ContentExtensionCompiler.cpp:63
> +    actions.uncheckedAppend(wideCharacters);

The use of uncheckedAppend here looks incorrect and possibly dangerous. I don’t
see any code above reserving the capacity so this can be unchecked.

> Source/WebCore/contentextensions/ContentExtensionCompiler.cpp:68
> +	   actions.grow(actions.size() + sizeof(UChar) * stringLength);
> +	   for (unsigned i = 0; i < stringLength; ++i)
> +	       *reinterpret_cast<UChar*>(&actions[startIndex + i *
sizeof(UChar)]) = string[i];

Same question.

> Source/WebCore/contentextensions/ContentExtensionCompiler.cpp:170
> +		   case ActionType::IgnorePreviousRules:
> +		   case ActionType::BlockLoad:
> +		   case ActionType::BlockCookies:
> +		   case ActionType::MakeHTTPS:
> +		      
actions.append(static_cast<SerializedActionByte>(actionType));
> +		       break;
> +		   case ActionType::AddHeader:
> +		       serializeStringAction(actions, actionType,
rule.action().stringArgument());
> +		       break;

I would just have a serializeString function. The action byte could be appended
here for all cases unconditionally instead of inside the serializeStringAction.
I think it would be cleaner separate of responsibilities in the code.

> Source/WebCore/contentextensions/ContentExtensionParser.cpp:248
> +	   && header.startsWith("X-"); // Let's just pretend RFC6648 never
happened.

Is this intentionally only checking for capital "X"? Or should it be using two
startsWith calls or startsWithLettersIgnoringASCIICase?

> Source/WebCore/contentextensions/ContentExtensionParser.cpp:294
> +	   return {Action(ActionType::AddHeader, headerString)};

WebKit style puts spaces inside the braces. But I see that this file is not
following that style. I’d also suggest braces after Action rather than
parentheses.

> Source/WebCore/contentextensions/ContentExtensionRule.cpp:69
> -	   uint32_t headerLength = sizeof(ActionType) + sizeof(uint32_t) +
sizeof(bool);
> -	   uint32_t stringStartIndex = location + headerLength;
> +	   uint32_t prefixLength = sizeof(ActionType) + sizeof(uint32_t) +
sizeof(bool);
> +	   uint32_t stringStartIndex = location + prefixLength;
>	   RELEASE_ASSERT(actionsLength >= stringStartIndex);
> -	   uint32_t selectorLength = *reinterpret_cast<const
unsigned*>(&actions[location + sizeof(ActionType)]);
> +	   uint32_t stringLength = *reinterpret_cast<const
unsigned*>(&actions[location + sizeof(ActionType)]);
>	   bool wideCharacters = actions[location + sizeof(ActionType) +
sizeof(uint32_t)];
>	   
>	   if (wideCharacters) {
> -	       RELEASE_ASSERT(actionsLength >= stringStartIndex +
selectorLength * sizeof(UChar));
> -	       return Action(ActionType::CSSDisplayNoneSelector,
String(reinterpret_cast<const UChar*>(&actions[stringStartIndex]),
selectorLength), location);
> +	       RELEASE_ASSERT(actionsLength >= stringStartIndex + stringLength
* sizeof(UChar));
> +	       return Action(actionType, String(reinterpret_cast<const
UChar*>(&actions[stringStartIndex]), stringLength), location);
>	   }
> -	   RELEASE_ASSERT(actionsLength >= stringStartIndex + selectorLength *
sizeof(LChar));
> -	   return Action(ActionType::CSSDisplayNoneSelector,
String(reinterpret_cast<const LChar*>(&actions[stringStartIndex]),
selectorLength), location);
> +	   RELEASE_ASSERT(actionsLength >= stringStartIndex + stringLength *
sizeof(LChar));
> +	   return Action(actionType, String(reinterpret_cast<const
LChar*>(&actions[stringStartIndex]), stringLength), location);

Seems to me this would be cleaner with a deserializeString function that
parallels the function used to serialize a string.

Also seems like we should just consider using UTF-8 to serialize instead of
having a separate UTF-16 case and Latin-1 case.

Generally annoying to have to have yet another set of code that knows how to
serialize strings with all this type casting and math. It’s a task we do
elsewhere and annoying we could not reuse any serialization framework we
already have elsewhere in WebKit.

> Source/WebCore/contentextensions/ContentExtensionRule.h:151
> +	   ASSERT(type != ActionType::CSSDisplayNoneSelector && type !=
ActionType::CSSDisplayNoneStyleSheet && type != ActionType::AddHeader);

Generally we try to have three assertions instead of one with && in it, so we
can see which one failed.

> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:180
> +	       size_t colonLocation = header.find(':');
> +	       ASSERT(colonLocation != notFound);

Why can we assert this? Is there code somewhere that validates it? Seems like
we need to check this rather than asserting it somewhere, since the data is
coming from outside WebKit.

Is it OK for the header field name or header field value to be empty string? If
not, then we should check and assert that too. What about whitespace? Is it
allowed? Should it be stripped?