Bug 219001 – Allow WKContentRuleList to block only in frames or only in main frame

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Add top-document and iframe-document resource types to WKContentRuleList syntax

[Alex Christensen]

Created attachment 414271 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/71747736>

[Alex Christensen]

Created attachment 416645 [details]
Patch

[Benjamin Poulain]

I think we can fulfill the original request while making something more powerful.

Instead of creating a new resource type, add a new field that specify the context: All, MainFrame, Iframe, SubFrame. Something like that.

The whole rule would be ignored if not in the right context.

This would allow any rule to be applied specifically based on the context. I assume it is common to block things when in frames but not in the top document (say tracking cookies from Facebook for example).

The downside is more useless matching if we filter a posteriori. If that becomes an issue, it can be fixed by splitting the filter by context a priori.

What do you think?

[Alex Christensen]

Sounds like something to discuss next year.  That might be necessary to fix rdar://problem/72203352 too.

[Alex Christensen]

That would not allow us the same functionality as having a nested-frame resource type.  An iframe's main resource is loaded in the context of its parent frame, so we would be unable to specify that we would like to block iframe main resources with your suggestion.

[Alex Christensen]

Created attachment 424175 [details]
Patch

[Geoffrey Garen]

Comment on attachment 424175 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadInfo.cpp:132
> +    if (name == "iframe")
> +        return { LoadContext::IFrame };

I think 'iframe' is the wrong name here because 'iframe' implies only <iframe> elements, but child frames can also be <frame>, <object>, or <embed>. 

Here's an alternative that matches web speak a little more: { top-frame, child-frame }

Here's an alternative that matches our parlance a little more: { main-frame, sub-frame }

I'd be happy with either of those, or something else that avoids 'iframe'. I don't think we should do 'iframe'.

[Alex Christensen]

Created attachment 424204 [details]
Patch

[Benjamin Poulain]

Comment on attachment 424204 [details]
Patch

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

> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:271
> +    ResourceLoadInfo resourceLoadInfo = { url, mainDocumentURL, { ResourceType::Ping, ResourceType::Other } };

Why not add ping to the "other" resource type in readResourceType?
Wouldn't this be more consistent with the other loaded resources?

> Source/WebCore/loader/PingLoader.cpp:125
> +    if (processContentRuleListsForLoad(frame, request, { ContentExtensions::ResourceType::Other, ContentExtensions::ResourceType::Ping }))

Ditto here regarding ping flags.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentRuleListNotification.mm:307
> +    auto handler = [[TestURLSchemeHandler new] autorelease];

I hope TEST has an autorelease pool :-D

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentRuleListNotification.mm:373
> +    [userContentController addContentRuleList:listWithLoadContext("child-frame").get()];
> +    [webView reload];
> +    EXPECT_WK_STREQ([webView _test_waitForAlert], "main frame fetched successfully");
> +    EXPECT_WK_STREQ([webView _test_waitForAlert], "iframe fetch failed");
> +
> +    [userContentController removeAllContentRuleLists];
> +    [userContentController addContentRuleList:listWithLoadContext("top-frame").get()];
> +    [webView reload];
> +    EXPECT_WK_STREQ([webView _test_waitForAlert], "main frame fetch failed");
> +    EXPECT_WK_STREQ([webView _test_waitForAlert], "iframe fetched successfully");

I like those tests!

[Alex Christensen]

Created attachment 424314 [details]
Patch

[EWS]

Ben Poulain found in /Volumes/Data/worker/Commit-Queue/build/Source/WebCore/ChangeLog does not appear to be a valid reviewer according to contributors.json.
/Volumes/Data/worker/Commit-Queue/build/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Benjamin Poulain]

Update looks good to me!

(In reply to EWS from comment #12)
> Ben Poulain found in
> /Volumes/Data/worker/Commit-Queue/build/Source/WebCore/ChangeLog does not
> appear to be a valid reviewer according to contributors.json.
> /Volumes/Data/worker/Commit-Queue/build/Source/WebCore/ChangeLog neither
> lists a valid reviewer nor contains the string "Unreviewed" or "Rubber
> stamp" (case insensitive).

You may need to use my full name.

[Alex Christensen]

r275078

[Alex Christensen]

*** Bug 153559 has been marked as a duplicate of this bug. ***