Bug 207840 – Web socket loads should be captured for logging per-page prevalent domains

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

Web socket and service worker loads should be captured for logging per-page prevalent domains

[Radar WebKit Bug Importer]

<rdar://problem/59511589>

[Radar WebKit Bug Importer]

<rdar://problem/59511746>

[Kate Cheney]

Created attachment 390928 [details]
Patch

[Dean Jackson]

Comment on attachment 390928 [details]
Patch

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

> LayoutTests/http/tests/websocket/web-socket-loads-captured-in-per-page-domains.html:21
> +            var passed = false;
> +            for (var i = 0; i < arrayOfDomains.length; ++i) {
> +                if (arrayOfDomains[i] === "localhost") {
> +                    passed = true;
> +                    break;
> +                }
> +            }
> +            if (passed)

I think this can all be replaced with:

if (arrayOfDomains.includes("localhost"))
  testPassed....
else
..

[youenn fablet]

Comment on attachment 390928 [details]
Patch

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

> Source/WebCore/Modules/websockets/WebSocket.cpp:314
> +        frame->loader().client().addLoadedRegistrableDomain(RegistrableDomain(m_url));

This is a bit too soon.
It might be better to add it after content blockers validation, which can either block the load or change it.

[youenn fablet]

OOps, didn't mean to put a review flag back.

[Chris Dumez]

Comment on attachment 390928 [details]
Patch

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

>> Source/WebCore/Modules/websockets/WebSocket.cpp:314
>> +        frame->loader().client().addLoadedRegistrableDomain(RegistrableDomain(m_url));
> 
> This is a bit too soon.
> It might be better to add it after content blockers validation, which can either block the load or change it.

Also, what about loads inside workers? This branch is only for documents.

[Kate Cheney]

(In reply to youenn fablet from comment #5)
> Comment on attachment 390928 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390928&action=review
> 
> > Source/WebCore/Modules/websockets/WebSocket.cpp:314
> > +        frame->loader().client().addLoadedRegistrableDomain(RegistrableDomain(m_url));
> 
> This is a bit too soon.
> It might be better to add it after content blockers validation, which can
> either block the load or change it.

This is called after ITP logs it, so if it's too soon we should also move that.

[Kate Cheney]

Created attachment 390948 [details]
Patch

[youenn fablet]

Comment on attachment 390948 [details]
Patch

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

> Source/WebCore/Modules/websockets/WebSocket.cpp:329
> +        RefPtr<Frame> frame = document.frame();

Probably do not need to ref it.

> Source/WebCore/Modules/websockets/WebSocket.cpp:330
> +        if (frame)

if (auto frame = document.frame())

> Source/WebCore/Modules/websockets/WebSocket.cpp:333
> +        downcast<WorkerGlobalScope>(context).thread().workerLoaderProxy().postTaskToLoader([this, protectedThis = makeRef(*this)] (ScriptExecutionContext& context) {

s/ (ScriptExecutionContext& context)/(auto& context)

> Source/WebCore/Modules/websockets/WebSocket.cpp:337
> +                frame->loader().client().addLoadedRegistrableDomain(RegistrableDomain(m_url));

It might be best to pass the registrable domain as part of the lambda.

[Chris Dumez]

Comment on attachment 390948 [details]
Patch

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

> Source/WebCore/Modules/websockets/WebSocket.cpp:328
> +        Document& document = downcast<Document>(context);

if (auto* frame = downcast<Document>(context).frame())
    frame->loader().client().addLoadedRegistrableDomain(RegistrableDomain(m_url));

> Source/WebCore/Modules/websockets/WebSocket.cpp:333
> +        downcast<WorkerGlobalScope>(context).thread().workerLoaderProxy().postTaskToLoader([this, protectedThis = makeRef(*this)] (ScriptExecutionContext& context) {

Why are you capturing this and protectedThis? Do you really need them? Maybe just capture the url.

> Source/WebCore/Modules/websockets/WebSocket.cpp:336
> +            if (frame)

if (auto* frame = downcast<Document>(context).frame())
    frame->loader().client().addLoadedRegistrableDomain(RegistrableDomain(m_url));

> Source/WebCore/Modules/websockets/WebSocket.cpp:339
> +    }

To avoid duplication, this could be something like:
auto reportRegistrableDomain = [url = m_url](ScriptExecutionContext& context) {
    if (auto* frame = downcast<Document>(context).frame())
        frame->loader().client().addLoadedRegistrableDomain(RegistrableDomain { url });
};
if (is<Document>(context)
    reportRegistrableDomain(context);
else
    downcast<WorkerGlobalScope>(context).thread().workerLoaderProxy().postTaskToLoader(WTFMove(reportRegistrableDomain));

> LayoutTests/http/tests/websocket/web-socket-loads-captured-in-per-page-domains-expected.txt:3
> +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".

Where is this "TEST COMPLETE" ?

[Kate Cheney]

Created attachment 390966 [details]
Patch

[Chris Dumez]

Comment on attachment 390966 [details]
Patch

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

> LayoutTests/http/tests/websocket/web-socket-loads-captured-in-per-page-domains.html:29
> +                setTimeout(askForPrevalentResources, 100);

This is concerning. Why 100? Will it be flaky? Would 0 suffice?

[Chris Dumez]

Comment on attachment 390966 [details]
Patch

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

> LayoutTests/http/tests/websocket/web-socket-loads-captured-in-per-page-domains.html:29
> +                setTimeout(askForPrevalentResources, 100);

This is concerning. Why 100? Will it be flaky? Would 0 suffice?

[Kate Cheney]

(In reply to Chris Dumez from comment #14)
> Comment on attachment 390966 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390966&action=review
> 
> > LayoutTests/http/tests/websocket/web-socket-loads-captured-in-per-page-domains.html:29
> > +                setTimeout(askForPrevalentResources, 100);
> 
> This is concerning. Why 100? Will it be flaky? Would 0 suffice?

It hasn't been flaky when running locally, I just wanted to make sure the resource had been stored before asking for it, and wasn't sure of a better way. I'll try with 0.

[Chris Dumez]

(In reply to katherine_cheney from comment #15)
> (In reply to Chris Dumez from comment #14)
> > Comment on attachment 390966 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=390966&action=review
> > 
> > > LayoutTests/http/tests/websocket/web-socket-loads-captured-in-per-page-domains.html:29
> > > +                setTimeout(askForPrevalentResources, 100);
> > 
> > This is concerning. Why 100? Will it be flaky? Would 0 suffice?
> 
> It hasn't been flaky when running locally, I just wanted to make sure the
> resource had been stored before asking for it, and wasn't sure of a better
> way. I'll try with 0.

It should be 0 (if it works reliably) or we should add some kind of synchronization mechanism. 100 is bound to be flaky on some bots and we do not want to land that.

[Kate Cheney]

(In reply to Chris Dumez from comment #16)
> (In reply to katherine_cheney from comment #15)
> > (In reply to Chris Dumez from comment #14)
> > > Comment on attachment 390966 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=390966&action=review
> > > 
> > > > LayoutTests/http/tests/websocket/web-socket-loads-captured-in-per-page-domains.html:29
> > > > +                setTimeout(askForPrevalentResources, 100);
> > > 
> > > This is concerning. Why 100? Will it be flaky? Would 0 suffice?
> > 
> > It hasn't been flaky when running locally, I just wanted to make sure the
> > resource had been stored before asking for it, and wasn't sure of a better
> > way. I'll try with 0.
> 
> It should be 0 (if it works reliably) or we should add some kind of
> synchronization mechanism. 100 is bound to be flaky on some bots and we do
> not want to land that.

I can run it a bunch locally and check for flakiness, but otherwise I'm not sure how to confirm reliability except to let the bots test it.

[Chris Dumez]

(In reply to katherine_cheney from comment #17)
> (In reply to Chris Dumez from comment #16)
> > (In reply to katherine_cheney from comment #15)
> > > (In reply to Chris Dumez from comment #14)
> > > > Comment on attachment 390966 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=390966&action=review
> > > > 
> > > > > LayoutTests/http/tests/websocket/web-socket-loads-captured-in-per-page-domains.html:29
> > > > > +                setTimeout(askForPrevalentResources, 100);
> > > > 
> > > > This is concerning. Why 100? Will it be flaky? Would 0 suffice?
> > > 
> > > It hasn't been flaky when running locally, I just wanted to make sure the
> > > resource had been stored before asking for it, and wasn't sure of a better
> > > way. I'll try with 0.
> > 
> > It should be 0 (if it works reliably) or we should add some kind of
> > synchronization mechanism. 100 is bound to be flaky on some bots and we do
> > not want to land that.
> 
> I can run it a bunch locally and check for flakiness, but otherwise I'm not
> sure how to confirm reliability except to let the bots test it.

If it seems to work for you locally with 0 locally then it is ok to land with 0 and see on the bots. If 0 does not reliably work for you locally, then landing with 100 (or any number greater than 0) is not OK and you will need some kind of synchronization mechanism (or write the test differently to keep trying until it gets the result it expects).

[Kate Cheney]

(In reply to Chris Dumez from comment #18)
> (In reply to katherine_cheney from comment #17)
> > (In reply to Chris Dumez from comment #16)
> > > (In reply to katherine_cheney from comment #15)
> > > > (In reply to Chris Dumez from comment #14)
> > > > > Comment on attachment 390966 [details]
> > > > > Patch
> > > > > 
> > > > > View in context:
> > > > > https://bugs.webkit.org/attachment.cgi?id=390966&action=review
> > > > > 
> > > > > > LayoutTests/http/tests/websocket/web-socket-loads-captured-in-per-page-domains.html:29
> > > > > > +                setTimeout(askForPrevalentResources, 100);
> > > > > 
> > > > > This is concerning. Why 100? Will it be flaky? Would 0 suffice?
> > > > 
> > > > It hasn't been flaky when running locally, I just wanted to make sure the
> > > > resource had been stored before asking for it, and wasn't sure of a better
> > > > way. I'll try with 0.
> > > 
> > > It should be 0 (if it works reliably) or we should add some kind of
> > > synchronization mechanism. 100 is bound to be flaky on some bots and we do
> > > not want to land that.
> > 
> > I can run it a bunch locally and check for flakiness, but otherwise I'm not
> > sure how to confirm reliability except to let the bots test it.
> 
> If it seems to work for you locally with 0 locally then it is ok to land
> with 0 and see on the bots. If 0 does not reliably work for you locally,
> then landing with 100 (or any number greater than 0) is not OK and you will
> need some kind of synchronization mechanism (or write the test differently
> to keep trying until it gets the result it expects).

Update: it is flaky. I am going to add a call (something like ResourceLoadStatistics installStatisticsDidScanDataRecordsCallback) which waits for a signal to the TestRunner before proceeding with its callback function.

[Kate Cheney]

Created attachment 390996 [details]
Patch

[Kate Cheney]

(In reply to katherine_cheney from comment #20)
> Created attachment 390996 [details]
> Patch

Going to let the bots run on this before cq+, but with this change it runs with no flakiness locally.

[Kate Cheney]

Created attachment 391005 [details]
Patch

[Kate Cheney]

Created attachment 391009 [details]
Patch

[Kate Cheney]

Created attachment 391021 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 391021 [details]:

imported/w3c/web-platform-tests/IndexedDB/interleaved-cursors-large.html bug 201849
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 391021 [details]:

http/tests/cookies/document-cookie-during-iframe-parsing.html bug 207895 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 391021 [details]
Patch

Rejecting attachment 391021 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 391021, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=391021&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=207840&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 391021 from bug 207840.
Fetching: https://bugs.webkit.org/attachment.cgi?id=391021
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 8 diffs from patch file(s).
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/Modules/websockets/WebSocket.cpp
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/http/tests/websocket/web-socket-loads-captured-in-per-page-domains-expected.txt
patching file LayoutTests/http/tests/websocket/web-socket-loads-captured-in-per-page-domains.html
patching file LayoutTests/platform/gtk/TestExpectations
patching file LayoutTests/platform/mac-wk1/TestExpectations
Hunk #1 FAILED at 937.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/mac-wk1/TestExpectations.rej
patching file LayoutTests/platform/win/TestExpectations

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/13324482

[Kate Cheney]

Created attachment 391055 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 391055 [details]
Patch for landing

Clearing flags on attachment: 391055

Committed r256837: <https://trac.webkit.org/changeset/256837>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.