Bug 195266 – Web Inspector: Audit: provide a way to get the contents of resources

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

This would be very helpful for performing linting/style-checking kinds of audits.  Otherwise, you'd either have to use an inline <script> or re-fetch the contents of the script from the server to test it.

[Radar WebKit Bug Importer]

<rdar://problem/48550911>

[Devin Rousso]

Created attachment 363485 [details]
Patch

I'm missing some other cases (e.g. injected/anonymous/extension scripts), but I figured I'd put this up for now for early feedback.  I'd also like to make this work for non-web inspection, as it would be very useful there.

[EWS Watchlist]

Comment on attachment 363485 [details]
Patch

Attachment 363485 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11358845

New failing tests:
inspector/audit/run-resources.html

[EWS Watchlist]

Created attachment 363489 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 363485 [details]
Patch

Attachment 363485 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11358848

New failing tests:
inspector/audit/run-resources.html

[EWS Watchlist]

Created attachment 363492 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 363485 [details]
Patch

Attachment 363485 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11358909

New failing tests:
inspector/audit/run-resources.html

[EWS Watchlist]

Created attachment 363493 [details]
Archive of layout-test-results from ews112 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Devin Rousso]

Created attachment 363548 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 363548 [details]
Patch

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

> Source/WebCore/inspector/InspectorAuditResourcesObject.cpp:26
> +
> +

Nit: Extra

> Source/WebCore/inspector/InspectorAuditResourcesObject.cpp:54
> +    Vector<Resource> resources;
> +    if (auto* frame = document.frame()) {

Style: Early return?

> Source/WebCore/inspector/InspectorAuditResourcesObject.cpp:61
> +                if (!options.value().url.isNull() && !cachedResource->url().string().contains(options.value().url))
> +                    continue;
> +
> +                if (!options.value().mimeType.isNull() && !cachedResource->mimeType().contains(options.value().mimeType))
> +                    continue;

Should these queries by case-insensitive?

Maybe it make more sense to make the filters be an array. So that someone could do something like:

    WebInspectorAudit.Resources.getContents({
        mimeTypes: [
            "javascript",
            "json",
        ]
    });

To get a set of resources of varying mime types:

    text/javascript
    application/json
    application/vnd.api+json

> Source/WebCore/inspector/InspectorAuditResourcesObject.h:49
> +        String contents;

How about `content` instead of `contents`?

Shouldn't this include a base64Encoded property to signal to the client if they should base64 decode it or not instead of assuming off of the mime type?

> LayoutTests/inspector/audit/run-resources.html:7
> +<script src="resources/sample-resource.js"></script>
> +<link rel="stylesheet" href="resources/sample-resource.css">

How about an image resource?

• Someone could run a pngcrush optimizer and see if images could be reduced.
• Someone could check color schemes of images for adherence to some rules

[Devin Rousso]

Created attachment 363700 [details]
Patch

[EWS Watchlist]

Comment on attachment 363700 [details]
Patch

Attachment 363700 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11386222

New failing tests:
inspector/audit/run-resources.html

[EWS Watchlist]

Created attachment 363707 [details]
Archive of layout-test-results from ews100 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 363700 [details]
Patch

Attachment 363700 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11386260

New failing tests:
inspector/audit/run-resources.html

[EWS Watchlist]

Created attachment 363709 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Devin Rousso]

Created attachment 363713 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 363713 [details]
Patch

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

> Source/WebCore/inspector/InspectorAuditResourcesObject.cpp:97
> +        if (base64Encoded)
> +            resource.base64Encoded = base64Encoded;

I'd expect this to always be set.

> Source/WebCore/inspector/InspectorAuditResourcesObject.h:58
> +    ExceptionOr<Vector<Resource>> getContents(Document&, Optional<QueryOptions>);

One drawback of having this be one API instead of two is that if you want all the resources you get an enormous JSON result with the contents of all resources. In fact, building that JSON might blow up the memory of inspected page.

We could have this be two APIs:

    getResources(query?)
    getResourceContent(url|id)

This is also nicer for developers to get the complete list of URLs/MIMETypes and filter them by RegExp on their own.

What do you think about this approach?

> Source/WebCore/inspector/InspectorAuditResourcesObject.idl:40
> +    boolean? base64Encoded;

Why make this optional? It is either true or false, what would null/undefined mean?

> LayoutTests/inspector/audit/run-resources-expected.txt:54
> +-- Running test case: Audit.run.Resources.getContents.InvalidURL

There aren't Invalid, they are not not found. Could rename the tests.

[Joseph Pecoraro]

Comment on attachment 363713 [details]
Patch

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

>> Source/WebCore/inspector/InspectorAuditResourcesObject.h:58
>> +    ExceptionOr<Vector<Resource>> getContents(Document&, Optional<QueryOptions>);
> 
> One drawback of having this be one API instead of two is that if you want all the resources you get an enormous JSON result with the contents of all resources. In fact, building that JSON might blow up the memory of inspected page.
> 
> We could have this be two APIs:
> 
>     getResources(query?)
>     getResourceContent(url|id)
> 
> This is also nicer for developers to get the complete list of URLs/MIMETypes and filter them by RegExp on their own.
> 
> What do you think about this approach?

I'm going to r- while we discuss this aspect, I think this is a better API.

Also maybe we should make these asynchronous, but everyone enjoys the simplicity of synchronous APIs.

[Devin Rousso]

Comment on attachment 363713 [details]
Patch

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

>>> Source/WebCore/inspector/InspectorAuditResourcesObject.h:58
>>> +    ExceptionOr<Vector<Resource>> getContents(Document&, Optional<QueryOptions>);
>> 
>> One drawback of having this be one API instead of two is that if you want all the resources you get an enormous JSON result with the contents of all resources. In fact, building that JSON might blow up the memory of inspected page.
>> 
>> We could have this be two APIs:
>> 
>>     getResources(query?)
>>     getResourceContent(url|id)
>> 
>> This is also nicer for developers to get the complete list of URLs/MIMETypes and filter them by RegExp on their own.
>> 
>> What do you think about this approach?
> 
> I'm going to r- while we discuss this aspect, I think this is a better API.
> 
> Also maybe we should make these asynchronous, but everyone enjoys the simplicity of synchronous APIs.

I like that approach a bit more.  Getting the content by URL makes the most sense to me.

[Devin Rousso]

Comment on attachment 363713 [details]
Patch

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

>>>> Source/WebCore/inspector/InspectorAuditResourcesObject.h:58
>>>> +    ExceptionOr<Vector<Resource>> getContents(Document&, Optional<QueryOptions>);
>>> 
>>> One drawback of having this be one API instead of two is that if you want all the resources you get an enormous JSON result with the contents of all resources. In fact, building that JSON might blow up the memory of inspected page.
>>> 
>>> We could have this be two APIs:
>>> 
>>>     getResources(query?)
>>>     getResourceContent(url|id)
>>> 
>>> This is also nicer for developers to get the complete list of URLs/MIMETypes and filter them by RegExp on their own.
>>> 
>>> What do you think about this approach?
>> 
>> I'm going to r- while we discuss this aspect, I think this is a better API.
>> 
>> Also maybe we should make these asynchronous, but everyone enjoys the simplicity of synchronous APIs.
> 
> I like that approach a bit more.  Getting the content by URL makes the most sense to me.

Actually, that may not be enough.  I think we'd want to generate an id for each resource.

[Devin Rousso]

Created attachment 363864 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 363864 [details]
Patch

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

> Source/WebCore/inspector/InspectorAuditResourcesObject.h:71
> +    HashMap<String, CachedResource*> m_resources;

Even though this InspectorAuditResourcesObject gets recreated between audit runs it is still possible that someone could:

    1. Get a list of resources which has an (`id` => CachedResource A)
    2. Load enough resources such that (A) gets purged from the cache
    3. getResourceContent(`id`) => causing a use-after-free or worse

You will need to keep the CachedResource alive (which I think you can do by adding a CachedResourceClient) or remove CachedResources that are purged from this list.

> Source/WebCore/inspector/InspectorAuditResourcesObject.idl:31
> +    [CallWith=Document, MayThrowException] sequence<Resource> getResources(optional QueryOptions? options);

Should we drop the QueryObjects concept now? It is easier to filter the list script, and it is already weird that developers don't know that the filter is case-insensitive and partial.

[Devin Rousso]

Created attachment 363922 [details]
Patch

This made me cry T.T

[Joseph Pecoraro]

Comment on attachment 363922 [details]
Patch

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

> Source/WebCore/inspector/InspectorAuditResourcesObject.cpp:124
> +void InspectorAuditResourcesObject::willDestroyCachedResource(CachedResource& cachedResource)
> +{
> +    m_resources.removeIf([&] (const auto& entry) {
> +        return entry.value == &cachedResource;
> +    });
> +
> +    cachedResource.removeClient(clientForResource(cachedResource));
> +}

Can this happen if we are a client?

CachedResource.cpp will only call willDestroy if it canDelete which can only happen if it has no clients:

    bool canDelete() const { return !hasClients() && !m_loader && !m_preloadCount && !m_handleCount && !m_resourceToRevalidate && !m_proxyResource; }

If we have a client then we should be preventing deletion from happening.

> Source/WebCore/inspector/InspectorAuditResourcesObject.h:57
> +    class InspectorAuditCachedResourceClient : public CachedResourceClient { };
> +    class InspectorAuditCachedFontClient : public CachedFontClient { };
> +    class InspectorAuditCachedImageClient : public CachedImageClient { };
> +    class InspectorAuditCachedRawResourceClient : public CachedRawResourceClient { };
> +    class InspectorAuditCachedSVGDocumentClient : public CachedSVGDocumentClient { };
> +    class InspectorAuditCachedStyleSheetClient : public CachedStyleSheetClient { };

These deserve a comment.

Can these be private?

Why can we not just use an empty CachedResourceClient subclass? Do we need the specifics because of assertions?

[EWS Watchlist]

Comment on attachment 363922 [details]
Patch

Attachment 363922 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11418736

New failing tests:
inspector/audit/teardown.html
inspector/audit/run.html

[EWS Watchlist]

Created attachment 363936 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 363922 [details]
Patch

Attachment 363922 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11419748

New failing tests:
inspector/audit/basic.html

[EWS Watchlist]

Created attachment 363952 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 363922 [details]
Patch

Attachment 363922 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11421181

New failing tests:
inspector/audit/run-resources.html
inspector/audit/basic.html

[EWS Watchlist]

Created attachment 363968 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Devin Rousso]

Comment on attachment 363922 [details]
Patch

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

>> Source/WebCore/inspector/InspectorAuditResourcesObject.cpp:124
>> +}
> 
> Can this happen if we are a client?
> 
> CachedResource.cpp will only call willDestroy if it canDelete which can only happen if it has no clients:
> 
>     bool canDelete() const { return !hasClients() && !m_loader && !m_preloadCount && !m_handleCount && !m_resourceToRevalidate && !m_proxyResource; }
> 
> If we have a client then we should be preventing deletion from happening.

Good point!  I missed that.

>> Source/WebCore/inspector/InspectorAuditResourcesObject.h:57
>> +    class InspectorAuditCachedStyleSheetClient : public CachedStyleSheetClient { };
> 
> These deserve a comment.
> 
> Can these be private?
> 
> Why can we not just use an empty CachedResourceClient subclass? Do we need the specifics because of assertions?

I will try private.  They need to subclass because `CachedResourceClient` has a protected constructor.  It's necessary to have these be separate because there are assertions/casts.

[Devin Rousso]

Created attachment 364037 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 364037 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 364037 [details]
Patch

Rejecting attachment 364037 [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', 364037, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

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

Parsed 25 diffs from patch file(s).
patching file Source/JavaScriptCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebInspectorUI/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/JavaScriptCore/inspector/InjectedScriptBase.cpp
patching file Source/JavaScriptCore/inspector/protocol/Audit.json
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file Source/JavaScriptCore/inspector/protocol/Audit.json.rej
patching file Source/WebCore/CMakeLists.txt
Hunk #1 succeeded at 902 (offset 1 line).
patching file Source/WebCore/DerivedSources-input.xcfilelist
Hunk #1 succeeded at 872 (offset 11 lines).
patching file Source/WebCore/DerivedSources-output.xcfilelist
Hunk #1 succeeded at 867 (offset 14 lines).
patching file Source/WebCore/DerivedSources.make
Hunk #1 succeeded at 852 (offset 1 line).
patching file Source/WebCore/Sources.txt
Hunk #1 succeeded at 1276 (offset 5 lines).
Hunk #2 succeeded at 2916 (offset 11 lines).
patching file Source/WebCore/WebCore.xcodeproj/project.pbxproj
Hunk #1 succeeded at 2608 (offset 7 lines).
Hunk #2 succeeded at 10468 (offset 20 lines).
Hunk #3 succeeded at 17067 (offset 22 lines).
Hunk #4 succeeded at 17093 (offset 22 lines).
Hunk #5 succeeded at 29781 (offset 37 lines).
Hunk #6 succeeded at 30217 (offset 37 lines).
patching file Source/WebCore/inspector/InspectorAuditResourcesObject.cpp
patching file Source/WebCore/inspector/InspectorAuditResourcesObject.h
patching file Source/WebCore/inspector/InspectorAuditResourcesObject.idl
patching file Source/WebCore/inspector/agents/InspectorPageAgent.cpp
patching file Source/WebCore/inspector/agents/InspectorPageAgent.h
patching file Source/WebCore/inspector/agents/page/PageAuditAgent.cpp
patching file Source/WebInspectorUI/UserInterface/Models/AuditTestBase.js
Hunk #1 FAILED at 185.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebInspectorUI/UserInterface/Models/AuditTestBase.js.rej
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/inspector/audit/resources/sample-resource.css
patching file LayoutTests/inspector/audit/resources/sample-resource.js
patching file LayoutTests/inspector/audit/run-resources-expected.txt
patching file LayoutTests/inspector/audit/run-resources.html
patching file LayoutTests/inspector/model/auditTestCase-expected.txt
Hunk #1 FAILED at 27.
Hunk #2 FAILED at 36.
2 out of 2 hunks FAILED -- saving rejects to file LayoutTests/inspector/model/auditTestCase-expected.txt.rej
patching file LayoutTests/inspector/model/auditTestGroup-expected.txt
Hunk #1 FAILED at 41.
Hunk #2 FAILED at 57.
Hunk #3 FAILED at 74.
Hunk #4 FAILED at 95.
4 out of 4 hunks FAILED -- saving rejects to file LayoutTests/inspector/model/auditTestGroup-expected.txt.rej

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

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

[Devin Rousso]

Created attachment 364650 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 364650 [details]
Patch

Rejecting attachment 364650 [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-02', 'validate-changelog', '--check-oops', '--non-interactive', 364650, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

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

[Devin Rousso]

Created attachment 364651 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 364651 [details]
Patch

Clearing flags on attachment: 364651

Committed r242941: <https://trac.webkit.org/changeset/242941>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.