Bug 161441 - Should have a way for clients to specify which page loads should use content extensions
Summary: Should have a way for clients to specify which page loads should use content ...
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Brian Weinstein
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-08-31 10:35 PDT by Brian Weinstein
Modified: 2016-09-06 09:57 PDT (History)
10 users (show)

See Also:


Attachments
[PATCH] Fix (15.60 KB, patch)
2016-08-31 10:55 PDT, Brian Weinstein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Weinstein 2016-08-31 10:35:26 PDT
Should have a way for clients to specify which page loads should use content extensions.
Comment 1 Radar WebKit Bug Importer 2016-08-31 10:36:39 PDT
<rdar://problem/28098075>
Comment 2 Brian Weinstein 2016-08-31 10:55:53 PDT
Created attachment 287519 [details]
[PATCH] Fix
Comment 3 Alex Christensen 2016-08-31 13:28:24 PDT
Comment on attachment 287519 [details]
[PATCH] Fix

Do we really want this to be synchronous and in the InjectedBundle?  That means that each InjectedBundle would need to have enough information to immediately decide if we want to use content extensions for this URL, or it would need to send synchronous IPC.  We designed content extensions to not need either of these performance reducing things.
Comment 4 Brian Weinstein 2016-08-31 13:36:06 PDT
(In reply to comment #3)
> Comment on attachment 287519 [details]
> [PATCH] Fix
> 
> Do we really want this to be synchronous and in the InjectedBundle?  That
> means that each InjectedBundle would need to have enough information to
> immediately decide if we want to use content extensions for this URL, or it
> would need to send synchronous IPC.  We designed content extensions to not
> need either of these performance reducing things.

In this case, the client is pushing the information to each injected bundle when it is updated, so the injected bundle always has the correct state of the world w.r.t. sites that shouldn’t use content blockers.

Another option is to push all of the information about which pages to disable content blockers on to WebKit/WebCore, but I’m not sure if that’s necessarily cleaner than what we are doing here.
Comment 5 Alex Christensen 2016-08-31 14:46:25 PDT
I'm concerned about pushing long lists of URLs to each new WebProcess every time we make a new tab.
Comment 6 Brian Weinstein 2016-08-31 14:57:07 PDT
(In reply to comment #5)
> I'm concerned about pushing long lists of URLs to each new WebProcess every
> time we make a new tab.

I agree with this concern in theory, but in practice:

- It wouldn't necessarily need a list of URLs, it could use high level domains
- I don't know how many domains users would store in practice.

Is there some code I can look at for how we would share one copy of this data across all web processes?
Comment 7 Sam Weinig 2016-08-31 23:23:29 PDT
Perhaps this could be determined at decidePolicyForNavigationAction decision time in the UIProcess. It might be generally useful to be able to override functionality at that time, so adding a mechanism for it could be useful.
Comment 8 Brian Weinstein 2016-08-31 23:27:12 PDT
(In reply to comment #7)
> Perhaps this could be determined at decidePolicyForNavigationAction decision
> time in the UIProcess. It might be generally useful to be able to override
> functionality at that time, so adding a mechanism for it could be useful.

That is an interesting idea! WKPageSetUserContentExtensionsEnabled seems to exist, but doesn't appear to be implemented.
Comment 9 Brian Weinstein 2016-08-31 23:31:34 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > Perhaps this could be determined at decidePolicyForNavigationAction decision
> > time in the UIProcess. It might be generally useful to be able to override
> > functionality at that time, so adding a mechanism for it could be useful.
> 
> That is an interesting idea! WKPageSetUserContentExtensionsEnabled seems to
> exist, but doesn't appear to be implemented.

But that might not be the way we would want to implement disabling content blockers for that load anyways, just something I noticed when clicking around.
Comment 10 Brian Weinstein 2016-08-31 23:39:37 PDT
Another concern is whether or not the main resource load will have already started by the time the UI Process handles the decidePolicyForNavigationAction call.
Comment 11 Alex Christensen 2016-09-01 10:56:25 PDT
decidePolicyForNavigationAction is initiated from didReceiveResponse in the NetworkProcess, which means it's too late; the main resource request has already been sent with content blockers applied
Comment 12 Sam Weinig 2016-09-02 13:46:06 PDT
(In reply to comment #11)
> decidePolicyForNavigationAction is initiated from didReceiveResponse in the
> NetworkProcess, which means it's too late; the main resource request has
> already been sent with content blockers applied

We do the initial decidePolicyForNavigationAction before talking to the NetworkProcess.
Comment 13 WebKit Commit Bot 2016-09-03 13:00:24 PDT
Comment on attachment 287519 [details]
[PATCH] Fix

Clearing flags on attachment: 287519

Committed r205407: <http://trac.webkit.org/changeset/205407>
Comment 14 WebKit Commit Bot 2016-09-03 13:00:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Sam Weinig 2016-09-05 20:20:38 PDT
I don't this this is the correct direction for this feature. Please roll this out so we can continue our discussion of ways to get this functionality without additional bundle SPI.
Comment 16 Ryan Haddad 2016-09-06 09:57:13 PDT
Reverted r205407 for reason:

Not the correct way to implement this functionality

Committed r205487: <http://trac.webkit.org/changeset/205487>