Web socket and service worker loads should be captured for logging per-page prevalent domains
<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?
(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.