Bug 146128 – [Content Extensions] Add SPI to reload without content blocking

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

[Alex Christensen]

Sometimes we want to load a site without content blocking.  Let's add a reloadWithoutContentBlockers api that will disable content blockers until we commit another load.
I'm not sure how to test this, but I think it needs a test.

[Alex Christensen]

Created attachment 255148 [details]
Patch

Comment on attachment 255148 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [Content Extensions] Add api to reload without content blocking.

API

> Source/WebCore/ChangeLog:17
> +        * loader/FrameLoader.cpp:
> +        (WebCore::FrameLoader::dispatchDidCommitLoad):
> +        * page/UserContentController.h:
> +        (WebCore::UserContentController::enableContentBlocking):
> +        (WebCore::UserContentController::disableContentBlocking):
> +        * replay/UserInputBridge.cpp:
> +        (WebCore::UserInputBridge::loadRequest):
> +        (WebCore::UserInputBridge::reloadFrame):
> +        * replay/UserInputBridge.h:

Where are the comments?

> Source/WebCore/loader/FrameLoader.cpp:3373
> +    // Content blocking could have been disabled by a _reloadWithoutContentBlockers or WKPageReloadWithoutContentBlockers call.

It’s not great to call out specific WebKit APIs in WebCore.

> Source/WebCore/loader/FrameLoader.cpp:3376
> +    // If this happens, we want to only disable content blockers until the next load is committed.
> +    if (m_frame.page() && m_frame.page()->userContentController())
> +        m_frame.page()->userContentController()->enableContentBlocking();

This doesn’t seem right. For one, if you reload-without-content-blockers and an error occurs in the provisional stage, you will not reach this function, and content blockers will remain disabled for the next navigation, which may not be a reload but something completely different. But beyond that, this isn’t very sound design. I think it implicitly relies on the fact that the main frame is the first frame to be committed upon reload. It doesn’t seem to handle client-side redirects. I don’t see how it handles any resources triggered by script or other deferral mechanisms after the main frame finished loading (but I don’t know exactly how the user content controller works).

To me, it seems like something like “do this load without content blocking” would be best implemented as a bit of state on DocumentLoader. To do a load (whether it’s a reload or not) without content blocking, you’d just set a bit on the DocumentLoader, and everything else would consult the DocumentLoader (perhaps via the frame or the page). Then you wouldn’t need to reset anything upon the load succeeding or failing, because the next load would use another DocumentLoader anyway. And you wouldn’t need a bit on UserContentController called m_contentBlockingEnabled which really means “content blocking not temporarily disabled”.

> Source/WebCore/page/UserContentController.h:95
> +    void enableContentBlocking() { m_contentBlockingEnabled = true; }
> +    void disableContentBlocking() { m_contentBlockingEnabled = false; }

I think we normally have one setter that takes a boolean.

> Source/WebCore/page/UserContentController.h:115
> +    bool m_contentBlockingEnabled { true };

So this isn’t used yet?

[Sam Weinig]

I don't like this approach.  I have a patch that adds a flag on WKWebView that lets you disable content extensions for that WKWebView. If one wanted to reload without content extension extensions, they set the flag and call reload. This would allow a client to keep content extensions off for a while if that is what they want.

(In reply to comment #3)
> I don't like this approach.  I have a patch that adds a flag on WKWebView
> that lets you disable content extensions for that WKWebView. If one wanted
> to reload without content extension extensions, they set the flag and call
> reload. This would allow a client to keep content extensions off for a while
> if that is what they want.

In that model, how does the client know when to turn it back on, if all they want is “load this one webpage without content extensions”? I think it would be nice if “content extensions disabled” were a property of a load (what we externally refer to as a navigation).

[Sam Weinig]

(In reply to comment #4)
> (In reply to comment #3)
> > I don't like this approach.  I have a patch that adds a flag on WKWebView
> > that lets you disable content extensions for that WKWebView. If one wanted
> > to reload without content extension extensions, they set the flag and call
> > reload. This would allow a client to keep content extensions off for a while
> > if that is what they want.
> 
> In that model, how does the client know when to turn it back on, if all they
> want is “load this one webpage without content extensions”? I think it would
> be nice if “content extensions disabled” were a property of a load (what we
> externally refer to as a navigation).

On the next navigation, as denoted via a decidePolicyForNavigationAction delegate message, they could disable it.

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > I don't like this approach.  I have a patch that adds a flag on WKWebView
> > > that lets you disable content extensions for that WKWebView. If one wanted
> > > to reload without content extension extensions, they set the flag and call
> > > reload. This would allow a client to keep content extensions off for a while
> > > if that is what they want.
> > 
> > In that model, how does the client know when to turn it back on, if all they
> > want is “load this one webpage without content extensions”? I think it would
> > be nice if “content extensions disabled” were a property of a load (what we
> > externally refer to as a navigation).
> 
> On the next navigation, as denoted via a decidePolicyForNavigationAction
> delegate message, they could disable it.

That’s not great, because it introduces a mode that the client needs to track:

1. User performs “load without content extensions” action
2. Client enters “content extensions disabled” mode
3. Client calls URL loading API
4. Client receives first policy decision call, notices that it’s in “content extensions disabled” mode, sets the property on the web view, and exits “content extensions disabled” mode

This is race-prone, because the policy decision in (4) can be about a different load, initiated in the Web Content process. So the client needs to track this state on a navigation (or something). If the Web Content process crashes before (4) then the client needs to figure out what to do with the state as well.

5. Client receives another policy decision call, notices that it’s no longer in “content extensions disabled” mode, resets the property on the web view.

This is more straightforward for the client:

1. User performs “load without content extensions” action
2. Client calls URL loading API, specifying an option (in an options dictionary that’s been introduced recently) that content extensions should be disabled for the resulting navigation

[Alex Christensen]

(In reply to comment #3)
> I don't like this approach.  I have a patch that adds a flag on WKWebView
> that lets you disable content extensions for that WKWebView. If one wanted
> to reload without content extension extensions, they set the flag and call
> reload. This would allow a client to keep content extensions off for a while
> if that is what they want.
This sounds better.  Where is this patch?

[Anders Carlsson]

(In reply to comment #7)
> (In reply to comment #3)
> > I don't like this approach.  I have a patch that adds a flag on WKWebView
> > that lets you disable content extensions for that WKWebView. If one wanted
> > to reload without content extension extensions, they set the flag and call
> > reload. This would allow a client to keep content extensions off for a while
> > if that is what they want.
> This sounds better.  Where is this patch?

I disagree that it sounds better; I'm (unsurprisingly) with Dan on this one.

[Alex Christensen]

What if a client wants to disable content blocking for more than one load?  Shouldn't the client be able to decide how long content blockers are disabled for?

[Darin Adler]

Comment on attachment 255148 [details]
Patch

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

>> Source/WebCore/loader/FrameLoader.cpp:3376
>> +        m_frame.page()->userContentController()->enableContentBlocking();
> 
> This doesn’t seem right. For one, if you reload-without-content-blockers and an error occurs in the provisional stage, you will not reach this function, and content blockers will remain disabled for the next navigation, which may not be a reload but something completely different. But beyond that, this isn’t very sound design. I think it implicitly relies on the fact that the main frame is the first frame to be committed upon reload. It doesn’t seem to handle client-side redirects. I don’t see how it handles any resources triggered by script or other deferral mechanisms after the main frame finished loading (but I don’t know exactly how the user content controller works).
> 
> To me, it seems like something like “do this load without content blocking” would be best implemented as a bit of state on DocumentLoader. To do a load (whether it’s a reload or not) without content blocking, you’d just set a bit on the DocumentLoader, and everything else would consult the DocumentLoader (perhaps via the frame or the page). Then you wouldn’t need to reset anything upon the load succeeding or failing, because the next load would use another DocumentLoader anyway. And you wouldn’t need a bit on UserContentController called m_contentBlockingEnabled which really means “content blocking not temporarily disabled”.

Mitz’s suggestion about using a flag on the DocumentLoader is a very good one.

[Darin Adler]

Comment on attachment 255148 [details]
Patch

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

>>> Source/WebCore/loader/FrameLoader.cpp:3376
>>> +        m_frame.page()->userContentController()->enableContentBlocking();
>> 
>> This doesn’t seem right. For one, if you reload-without-content-blockers and an error occurs in the provisional stage, you will not reach this function, and content blockers will remain disabled for the next navigation, which may not be a reload but something completely different. But beyond that, this isn’t very sound design. I think it implicitly relies on the fact that the main frame is the first frame to be committed upon reload. It doesn’t seem to handle client-side redirects. I don’t see how it handles any resources triggered by script or other deferral mechanisms after the main frame finished loading (but I don’t know exactly how the user content controller works).
>> 
>> To me, it seems like something like “do this load without content blocking” would be best implemented as a bit of state on DocumentLoader. To do a load (whether it’s a reload or not) without content blocking, you’d just set a bit on the DocumentLoader, and everything else would consult the DocumentLoader (perhaps via the frame or the page). Then you wouldn’t need to reset anything upon the load succeeding or failing, because the next load would use another DocumentLoader anyway. And you wouldn’t need a bit on UserContentController called m_contentBlockingEnabled which really means “content blocking not temporarily disabled”.
> 
> Mitz’s suggestion about using a flag on the DocumentLoader is a very good one.

A tricky part of that is checking for any case where we make an extra document loader and need to move the flag from the previous to the next one. I’m not sure that exists, but we should double check.

[Alex Christensen]

Created attachment 255240 [details]
Patch

Comment on attachment 255240 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:76
> +@property (nonatomic, setter=_setUserContentExtensionsEnabled:) BOOL _userContentExtensionsEnabled;

Please add WK_AVAILABLE(WK_MAC_TBA, WK_IOS_TBA) here!

[Sam Weinig]

<rdar://problem/20351903>

[Alex Christensen]

http://trac.webkit.org/changeset/185840