Bug 93554 – Don't put blob: urls in caches

• Status: NEW
• Component: New Bugs

[Scott Graham]

Don't put blob: urls in caches

[Scott Graham]

Created attachment 157337 [details]
Patch

[Scott Graham]

(Is including BlobURL.h ok here? Is there a better way to check for blob-ness?)

[Gyuyoung Kim]

Comment on attachment 157337 [details]
Patch

Attachment 157337 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13460370

[Build Bot]

Comment on attachment 157337 [details]
Patch

Attachment 157337 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13456700

[Scott Graham]

Evidently not OK to include.

[Build Bot]

Comment on attachment 157337 [details]
Patch

Attachment 157337 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13456745

[Scott Graham]

Created attachment 157460 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 157460 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Avoids space leaks when content generates many blob: urls. See
> +        http://crbug.com/114570 for downstream bug report and repro.

It is not good form to reference external systems in bug reports. Making everyone open an additional tab and scan through relevant and irrelevant comments takes time from multiple people. Please post all relevant information to Bugzilla.

> Source/WebCore/ChangeLog:12
> +        No new tests, as there's no way to track not-leaked-but-growing memory
> +        usage.

There has been work done recently on memory tracking for performance tests, perhaps that helps here.

> Source/WebCore/loader/DocumentLoader.h:230
>              if (protocolIs(url, "data"))
>                  return;
> +            // Don't include potentially large unique blob urls here.
> +            if (url.startsWith("blob:"))

Why are these checks different (protocolIs vs. startsWith)?

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:508
> +    if (!request.url().protocolIsData() && !request.url().string().startsWith("blob:"))

Ditto.

Not sure if this case is affected, but there is often a subtle difference introduced by using URL string for checks. Namely, that does the wrong thing for invalid URLs.

[Scott Graham]

Created attachment 157508 [details]
Test case that causes slow space leak

[Scott Graham]

Created attachment 157510 [details]
Patch

[Scott Graham]

(In reply to comment #8)
> (From update of attachment 157460 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157460&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Avoids space leaks when content generates many blob: urls. See
> > +        http://crbug.com/114570 for downstream bug report and repro.
> 
> It is not good form to reference external systems in bug reports. Making everyone open an additional tab and scan through relevant and irrelevant comments takes time from multiple people. Please post all relevant information to Bugzilla.
> 
> > Source/WebCore/ChangeLog:12
> > +        No new tests, as there's no way to track not-leaked-but-growing memory
> > +        usage.
> 
> There has been work done recently on memory tracking for performance tests, perhaps that helps here.
> 
> > Source/WebCore/loader/DocumentLoader.h:230
> >              if (protocolIs(url, "data"))
> >                  return;
> > +            // Don't include potentially large unique blob urls here.
> > +            if (url.startsWith("blob:"))
> 
> Why are these checks different (protocolIs vs. startsWith)?
> 
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:508
> > +    if (!request.url().protocolIsData() && !request.url().string().startsWith("blob:"))
> 
> Ditto.
> 
> Not sure if this case is affected, but there is often a subtle difference introduced by using URL string for checks. Namely, that does the wrong thing for invalid URLs.

Thanks for your review. I've uploaded the test case here, and fixed the issues you pointed out in the code.

So far, I've only found reportMemoryUsage which appears to track DOM node memory usage, but wouldn't catch this growth of internal caches. Do you happen to know if there's another way to track overall memory usage, currently?

[Scott Graham]

Created attachment 158860 [details]
Patch

[Build Bot]

Comment on attachment 158860 [details]
Patch

Attachment 158860 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13517434

[WebKit Review Bot]

Comment on attachment 158860 [details]
Patch

Attachment 158860 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13514487

New failing tests:
fast/files/blob-cache-leak.html

[WebKit Review Bot]

Created attachment 158872 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Build Bot]

Comment on attachment 158860 [details]
Patch

Attachment 158860 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13518292

[Scott Graham]

Created attachment 158876 [details]
tweak to avoid timeout

[WebKit Review Bot]

Comment on attachment 158876 [details]
tweak to avoid timeout

Attachment 158876 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13515516

New failing tests:
fast/files/blob-cache-leak.html

[WebKit Review Bot]

Created attachment 158883 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Build Bot]

Comment on attachment 158876 [details]
tweak to avoid timeout

Attachment 158876 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13510739

[Build Bot]

Comment on attachment 158876 [details]
tweak to avoid timeout

Attachment 158876 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13512620

[Andreas Kling]

Comment on attachment 158876 [details]
tweak to avoid timeout

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.