Bug 204262 - Resource Load Statistics: Count third-party script loads under top frame
Summary: Resource Load Statistics: Count third-party script loads under top frame
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-11-15 16:33 PST by John Wilander
Modified: 2019-11-19 12:35 PST (History)
9 users (show)

See Also:


Attachments
Patch (50.95 KB, patch)
2019-11-15 17:07 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (57.83 KB, patch)
2019-11-15 17:26 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch for landing (58.03 KB, patch)
2019-11-19 11:49 PST, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 2019-11-15 16:33:24 PST
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.
Comment 1 Radar WebKit Bug Importer 2019-11-15 16:33:52 PST
<rdar://problem/57244945>
Comment 2 John Wilander 2019-11-15 17:07:55 PST
Created attachment 383670 [details]
Patch
Comment 3 John Wilander 2019-11-15 17:09:28 PST
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!
Comment 4 John Wilander 2019-11-15 17:26:12 PST
Created attachment 383675 [details]
Patch
Comment 5 John Wilander 2019-11-15 17:26:31 PST
Added a negative test case.
Comment 6 John Wilander 2019-11-16 10:47:34 PST
iOS-wk2 test failure is unrelated.
Comment 7 katherine_cheney 2019-11-17 10:31:35 PST
(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!
Comment 8 Alex Christensen 2019-11-18 14:14:30 PST
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.
Comment 9 John Wilander 2019-11-18 14:24:42 PST
(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.
Comment 10 Alex Christensen 2019-11-18 16:17:01 PST
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.
Comment 11 John Wilander 2019-11-18 16:35:36 PST
(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?
Comment 12 John Wilander 2019-11-19 11:49:08 PST
Created attachment 383889 [details]
Patch for landing
Comment 13 John Wilander 2019-11-19 12:02:15 PST
Thanks, Kate and Alex!
Comment 14 WebKit Commit Bot 2019-11-19 12:35:28 PST
Comment on attachment 383889 [details]
Patch for landing

Clearing flags on attachment: 383889

Committed r252641: <https://trac.webkit.org/changeset/252641>
Comment 15 WebKit Commit Bot 2019-11-19 12:35:30 PST
All reviewed patches have been landed.  Closing bug.