Bug 188270 – Web Inspector: Global search sometimes returns duplicate results for a resource

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

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?

[Radar WebKit Bug Importer]

<rdar://problem/42867498>

[Joseph Pecoraro]

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?

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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

[Joseph Pecoraro]

Created attachment 346432 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Filed a follow-up for a backend fix. (I'm not investigating that right now)
https://bugs.webkit.org/show_bug.cgi?id=188287

[EWS Watchlist]

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

[EWS Watchlist]

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

[Blaze Burg]

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.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

https://trac.webkit.org/changeset/234637/webkit