Bug 207840

<rdar://problem/59511589>

<rdar://problem/59511746>

Created attachment 390928 [details]
Patch

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
..

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.

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

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.

(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.

Created attachment 390948 [details]
Patch

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.

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" ?

Created attachment 390966 [details]
Patch

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?

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?

(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.

(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.

(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.

(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).

(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.

Created attachment 390996 [details]
Patch

(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.

Created attachment 391005 [details]
Patch

Created attachment 391009 [details]
Patch

Created attachment 391021 [details]
Patch

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.

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: cdumez@apple.com)
The commit-queue is continuing to process your patch.

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 commit-queue@webkit.org...
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

Created attachment 391055 [details]
Patch for landing

Comment on attachment 391055 [details]
Patch for landing

Clearing flags on attachment: 391055

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

All reviewed patches have been landed.  Closing bug.