Bug 142227 – prepare content extensions for css selectors

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Alex Christensen]

In order to be able to use css selectors in content extensions, we need to be able to store actions that are not just enums.

[Alex Christensen]

Created attachment 247771 [details]
Patch

[Alex Christensen]

Created attachment 247776 [details]
Patch

[Alex Christensen]

Created attachment 247781 [details]
Patch

[Alex Christensen]

Created attachment 247786 [details]
Patch

[Alex Christensen]

Comment on attachment 247786 [details]
Patch

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

> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:174
>      for (auto& ruleListSlot : m_ruleLists) {

This would cause problems with multiple extensions installed with ignore-previous-rules, but we can fix that and test it in another patch.

> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:180
> +        actionLocations.reserveInitialCapacity(actionLocations.size());

This should be triggeredActions.size()

[Benjamin Poulain]

Comment on attachment 247786 [details]
Patch

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

Quick review.

> Source/WebCore/contentextensions/ContentExtensionActions.h:39
> +typedef uint8_t ActionDescription;

I am a bit unconvinced about the name.

> Source/WebCore/contentextensions/ContentExtensionActions.h:45
> +    // The enum value contains all the information necessary for applying these actions.
> +    BlockLoad,

Can you please keep the list sorted?

Maybe add a line of comment for each action?

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

What the heck? Change of plans?

> Source/WebCore/contentextensions/ContentExtensionActions.h:50
> +    // When serialized, CSSDisplayNone is followed by the length of the selector (4 bytes) then the selector.

This comment is out of context. The enum is too far from the serialization code.

> Source/WebCore/contentextensions/ContentExtensionRule.cpp:43
> +    : m_type(static_cast<ActionType>(*description))

Please use a switch() for this. Clang can optimize it out, and we can catch incorrect values.

> Source/WebCore/contentextensions/ContentExtensionRule.cpp:48
> +        unsigned length = *reinterpret_cast<const unsigned*>(description + sizeof(uint8_t));
> +        m_cssSelector = String(description + sizeof(uint8_t) + sizeof(unsigned), length);

Can we get description + buffer length as input? It would be good to crash if we ever create a String past the buffer.

> Source/WebCore/contentextensions/ContentExtensionRule.h:53
> +    Action(const ActionDescription*);

I think this should not be a constructor. A deserialize() function somewhere seems cleaner to me.

> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:71
> +            {

I think the WebKit style is 
    case ActionType::CSSDisplayNone: {
    }

I am never sure about the style for switch-case...

[Alex Christensen]

Created attachment 247804 [details]
Patch

[Benjamin Poulain]

Comment on attachment 247804 [details]
Patch

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

> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:47
> +Vector<unsigned> ContentExtensionsBackend::serializeActions(const Vector<ContentExtensionRule>& ruleList, Vector<SerializedActionByte>& actions)

Could you merge similar actions when they are following eachother? This would help making better state machines.

> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:55
> +    for (unsigned ruleIndex = 0; ruleIndex < ruleList.size(); ruleIndex++) {

++ruleIndex

> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:81
> +            for (unsigned i = 0; i < selectorLength; i++)
> +                actions.append(selector[i]);

Selectors can be 8 bits or 16 bits. Here you would nuke the 16bits one into 8 bits bufffer.
You should probably have "length, character-size, bytes".

> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:179
> +            actionLocations.reserveInitialCapacity(actionLocations.size());

actionLocations.size() -> triggeredActions.size()!

> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:186
> +                if (action.type() == ActionType::IgnorePreviousRules)

Maybe ActionType::Block should be one of those early return too? If we have
    1) CSS
    2) Whitelist
    3) CSS
    4) Block
No point in having [3] in the return vector.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:473
> +    // Start with the last action to properly deal with IgnorePreviousRules.

This code does not handle IgnorePreviousRules. It looks like you could execute those actions in any order.

> LayoutTests/ChangeLog:10
> +        * http/tests/usercontentfilter/css-display-none-expected.txt: Added.
> +        * http/tests/usercontentfilter/css-display-none.html: Added.
> +        * http/tests/usercontentfilter/css-display-none.html.json: Added.

We should really get proper testing of the backend at some point :)

[Build Bot]

Comment on attachment 247804 [details]
Patch

Attachment 247804 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5468072710242304

New failing tests:
http/tests/usercontentfilter/block-cookies-basic.html
http/tests/usercontentfilter/basic-filter.html
http/tests/usercontentfilter/block-cookies-send.html

[Build Bot]

Created attachment 247805 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Alex Christensen]

Created attachment 247807 [details]
Patch

[Alex Christensen]

Created attachment 247819 [details]
Patch

[Benjamin Poulain]

Comment on attachment 247819 [details]
Patch

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

> Source/WebCore/contentextensions/ContentExtensionRule.cpp:56
> +        unsigned selectorLength = *reinterpret_cast<const unsigned*>(&actions[location + sizeof(ActionType)]);

You are shifting characters one by one when serializing, then reading them in one read when deserializing. This only works because you shifted them as little endian.

You should use the same method on read and write to make this work everywhere.

> Source/WebCore/contentextensions/ContentExtensionRule.cpp:60
> +            ASSERT(actions.size() >= stringStartIndex + selectorLength * sizeof(UChar));

This should be a release assert IMHO.

> Source/WebCore/contentextensions/ContentExtensionRule.cpp:63
> +        ASSERT(actions.size() >= stringStartIndex + selectorLength * sizeof(LChar));

This should be a release assert IMHO.

> Source/WebCore/contentextensions/ContentExtensionRule.h:45
> +    bool urlFilterIsCaseSensitive = false;

Let's change it to
    urlFilterIsCaseSensitive { false };

People seem to favor this syntax now.

> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:49
> +    // This data is interpreted by Action::Action(const SerializedActionByte*).

This is no longer true. Probably no need for the comment now that there is a name to the function that deserializes.

> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:86
> +                    actions.append(wideCharacter);
> +                    actions.append(wideCharacter >> 8);

Same problem: this force a write in little-endian.

[Alex Christensen]

Comments addressed, committed to http://trac.webkit.org/changeset/180978
Windows build fix committed to http://trac.webkit.org/changeset/180979

Will look into duplicate actions and other optimizations.