Bug 204262 – Resource Load Statistics: Count third-party script loads under top frame

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

[John Wilander]

Third-party scripts running in the first-party context are a significant privacy and security risk. We should capture the number of such script loads to allow ITP to take action.

[Radar WebKit Bug Importer]

<rdar://problem/57244945>

[John Wilander]

Created attachment 383670 [details]
Patch

[John Wilander]

Kate, please have a look at the changes to the DB store. I'm adding a new data category and want to know if I got it right. Thanks!

[John Wilander]

Created attachment 383675 [details]
Patch

[John Wilander]

Added a negative test case.

[John Wilander]

iOS-wk2 test failure is unrelated.

[Kate Cheney]

(In reply to John Wilander from comment #3)
> Kate, please have a look at the changes to the DB store. I'm adding a new
> data category and want to know if I got it right. Thanks!

This looks good to me re: database changes!

[Alex Christensen]

Comment on attachment 383675 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadObserver.h:41
> +    enum class IsScriptLikeSubresource : bool { Yes, No };

I don't think this is a good name.  I think instead isScriptLikeDestination should be renamed to something more descriptive of what is going on.
Also, its implementation says it has something to do with service workers, but I don't see any tests involving service workers.

[John Wilander]

(In reply to Alex Christensen from comment #8)
> Comment on attachment 383675 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383675&action=review
> 
> > Source/WebCore/loader/ResourceLoadObserver.h:41
> > +    enum class IsScriptLikeSubresource : bool { Yes, No };
> 
> I don't think this is a good name.  I think instead isScriptLikeDestination
> should be renamed to something more descriptive of what is going on.

I assume you mean IsScriptLikeSubresource should be renamed. Any suggestions? At least the direction you would like it to go.

> Also, its implementation says it has something to do with service workers,
> but I don't see any tests involving service workers.

I created such a test case but we don’t allow cross-origin ServiceWorkers in the top frame context. Throws a security error.

[Alex Christensen]

Comment on attachment 383675 [details]
Patch

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

r=me other than that.

>>> Source/WebCore/loader/ResourceLoadObserver.h:41
>>> +    enum class IsScriptLikeSubresource : bool { Yes, No };
>> 
>> I don't think this is a good name.  I think instead isScriptLikeDestination should be renamed to something more descriptive of what is going on.
>> Also, its implementation says it has something to do with service workers, but I don't see any tests involving service workers.
> 
> I assume you mean IsScriptLikeSubresource should be renamed. Any suggestions? At least the direction you would like it to go.

isScriptLikeDestination -> fetchDestinationIsScriptLike to indicate it is for fetch.  Maybe a link to https://fetch.spec.whatwg.org/#request-destination-script-like somewhere.
IsScriptLikeSubresource isn't so bad once I understood that.

[John Wilander]

(In reply to Alex Christensen from comment #10)
> Comment on attachment 383675 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383675&action=review
> 
> r=me other than that.
> 
> >>> Source/WebCore/loader/ResourceLoadObserver.h:41
> >>> +    enum class IsScriptLikeSubresource : bool { Yes, No };
> >> 
> >> I don't think this is a good name.  I think instead isScriptLikeDestination should be renamed to something more descriptive of what is going on.
> >> Also, its implementation says it has something to do with service workers, but I don't see any tests involving service workers.
> > 
> > I assume you mean IsScriptLikeSubresource should be renamed. Any suggestions? At least the direction you would like it to go.
> 
> isScriptLikeDestination -> fetchDestinationIsScriptLike to indicate it is
> for fetch.  Maybe a link to
> https://fetch.spec.whatwg.org/#request-destination-script-like somewhere.
> IsScriptLikeSubresource isn't so bad once I understood that.

Got it. I use IsScriptLikeSubresource in two call sites – in FrameLoader::loadResourceSynchronously() and in SubresourceLoader::willSendRequestInternal() – and in both places I'm in code that will perform a subresource load. That's why I named it that way, so that future call sites would understand. But I also considered sending the full options().destination in instead. Would that be better?

[John Wilander]

Created attachment 383889 [details]
Patch for landing

[John Wilander]

Thanks, Kate and Alex!

[WebKit Commit Bot]

Comment on attachment 383889 [details]
Patch for landing

Clearing flags on attachment: 383889

Committed r252641: <https://trac.webkit.org/changeset/252641>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.