[webkit-reviews] review denied: [Bug 195266] Web Inspector: Audit: provide a way to get the contents of resources : [Attachment 363864] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 195266: Web Inspector: Audit: provide a way to get the contents of
resources
https://bugs.webkit.org/show_bug.cgi?id=195266

Attachment 363864: Patch

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




--- Comment #22 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 363864
  --> https://bugs.webkit.org/attachment.cgi?id=363864
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.