Global search sometimes returns duplicate results for a resource Steps to Reproduce: 1. Inspect <https://www.cio.com> 2. Search for "touchMoved:" 3. May require a reload + research to trigger this issue => Should find 2 results, instead finds 4 (the 2 results are each duplicated) Notes: I'm seeing two, slightly different frontend requests to search the same resource: [Log] request: {"id":795,"method":"Page.searchInResource","params":{"frameId":"0.4","url":"https://www.cio.com/www.idge.ans/js/select2-3.5.0/select2.js","query":"touchMoved:","caseSensitive":false,"isRegex":false}} (Main.js, line 941) [Log] request: {"id":796,"method":"Page.searchInResource","params":{"frameId":"0.4","url":"https://www.cio.com/www.idge.ans/js/select2-3.5.0/select2.js","query":"touchMoved:","caseSensitive":false,"isRegex":false,"requestId":"0.972"}} (Main.js, line 941) Why did this duplication happen?
<rdar://problem/42867498>
Hmm, the backend points to this resource twice in the result of `Page.searchInResources`: [Log] request: { "id": 596, "method": "Page.searchInResources", "params": { "text": "touchMoved:", "caseSensitive": false, "isRegex": false } } [Log] response: { "result": { "result": [{ "url": "https://www.cio.com/www.idge.ans/js/select2-3.5.0/select2.js", "frameId": "0.2", "matchesCount": 2 }, { "url": "https://www.cio.com/www.idge.ans/js/select2-3.5.0/select2.js", "frameId": "0.2", "matchesCount": 2, "requestId": "0.857" }] }, "id": 596 } Looks to be the same resource, why is it getting duplicated?
Page.searchInResources does two searches: (1) PageAgent search cached resources in frame tree (2) NetworkAgent searchOtherRequests to search non-cached resources So it would seem (2) may be searching in a resource that has a CachedResource that could otherwise be skipped.
(In reply to Joseph Pecoraro from comment #3) > Page.searchInResources does two searches: > > (1) PageAgent search cached resources in frame tree > (2) NetworkAgent searchOtherRequests to search non-cached resources > > So it would seem (2) may be searching in a resource that has a > CachedResource that could otherwise be skipped. Hmm, unfortunately that suggested change doesn't work for some Worker/XHR resources, which this test catches: LayoutTests/inspector/page/searchInResources.html Worst case the frontend can avoid duplicates. That might be an easy fix for now, I'm not sure off-hand what the backend can do to address this.
Created attachment 346432 [details] [PATCH] Proposed Fix
Filed a follow-up for a backend fix. (I'm not investigating that right now) https://bugs.webkit.org/show_bug.cgi?id=188287
Comment on attachment 346432 [details] [PATCH] Proposed Fix Attachment 346432 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8746870 New failing tests: http/tests/security/video-poster-cross-origin-crash2.html
Created attachment 346462 [details] Archive of layout-test-results from ews200 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 346432 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=346432&action=review r=me > Source/WebInspectorUI/UserInterface/Views/SearchSidebarPanel.js:175 > + for (let i = 0; i < result.length; ++i) { Alternatively, use reduce() > Source/WebInspectorUI/UserInterface/Views/SearchSidebarPanel.js:180 > + // FIXME: Backend sometimes searches files twice. Nit: add link to backend bug > Source/WebInspectorUI/UserInterface/Views/SearchSidebarPanel.js:182 > + let key = searchResult.frameId + ":" + searchResult.url; Nit: could use destructuring of searchResult above and template literal. Or not, doesn't matter. > Source/WebInspectorUI/UserInterface/Views/SearchSidebarPanel.js:187 > // COMPATIBILITY (iOS 9): Page.searchInResources did not have the optional requestId parameter. Not a blocker, but... it seems problematic that the View should be doing this sort of backend-specific deduplication logic at all. It may make sense to introduce a SearchManager or some other model/controller object to make requests and keep track of cached search results apart from the sidebar view class. That would also make this testable. If there is any reason to wait on the searchInResource backend command to finish, we could iterate preventDuplicates and issue all the commands at once and use Promise.all to wait the results.
Comment on attachment 346432 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=346432&action=review >> Source/WebInspectorUI/UserInterface/Views/SearchSidebarPanel.js:180 >> + // FIXME: Backend sometimes searches files twice. > > Nit: add link to backend bug Will do. >> Source/WebInspectorUI/UserInterface/Views/SearchSidebarPanel.js:187 >> // COMPATIBILITY (iOS 9): Page.searchInResources did not have the optional requestId parameter. > > Not a blocker, but... it seems problematic that the View should be doing this sort of backend-specific deduplication logic at all. It may make sense to introduce a SearchManager or some other model/controller object to make requests and keep track of cached search results apart from the sidebar view class. That would also make this testable. > > If there is any reason to wait on the searchInResource backend command to finish, we could iterate preventDuplicates and issue all the commands at once and use Promise.all to wait the results. I think we want to issue each of the backend requests individually such that we populate the UI incrementally with each result. It is effectively chunking the work for us.
https://trac.webkit.org/changeset/234637/webkit