Bug 144259 – Network Cache: Disk cache getting filled by YouTube video data

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Antti Koivisto]

MSE media loads via the cache and tends to eventually fill it.

[Antti Koivisto]

rdar://problem/20127334

[Antti Koivisto]

Created attachment 251736 [details]
patch

[WebKit Commit Bot]

Attachment 251736 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/ResourceRequestBase.h:150:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

Created attachment 251738 [details]
patch

[WebKit Commit Bot]

Attachment 251738 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/ResourceRequestBase.h:150:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

Created attachment 251739 [details]
patch

[WebKit Commit Bot]

Attachment 251739 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/ResourceRequestBase.h:150:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

Created attachment 251743 [details]
patch

[WebKit Commit Bot]

Attachment 251743 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/ResourceRequestBase.h:150:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 251743 [details]
patch

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

> Source/WebCore/platform/network/ResourceRequestBase.h:231
> +        Requester m_requester { Requester::Unspecified };

The data members just above are going out of their way to use bitfields, and here for something that takes only 2 bits we are not using one.

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:307
> +    bool isLikelyStreamingMedia = isXHR && (response.mimeType().startsWith("video/") || response.mimeType().startsWith("audio/"));

Is MIME type guaranteed to be lower case? I think it would be best to do this comparison in a case folding way.

[Antti Koivisto]

> The data members just above are going out of their way to use bitfields, and
> here for something that takes only 2 bits we are not using one.

Those bitfields are an over-optimization. I was planning to get rid of them separately.

[Geoffrey Garen]

Comment on attachment 251743 [details]
patch

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

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:309
> +    // Media loaded via XHR is likely being used for MSE streaming (YouTube and Netflix for example).
> +    // Streaming media fills the cache quickly and is unlikely to be reused.
> +    bool isXHR = originalRequest.requester() == WebCore::ResourceRequest::Requester::XHR;
> +    bool isLikelyStreamingMedia = isXHR && (response.mimeType().startsWith("video/") || response.mimeType().startsWith("audio/"));
> +    if (isLikelyStreamingMedia)
> +        return StoreDecision::NoDueToStreamingMedia;

We don't think that people rewind, fast-forward, and/or revisit videos? I tend to do all of those things. :(

[Alexey Proskuryakov]

We just removed "requestor" from ResourceRequest due to it being a layering violation, and deprecated isMainResourceRequest, so it's unfortunate that it's being added back. ResourceRequest is a network level concept that shouldn't know about "XMLHttpRequest" on "main resource" in my opinion.

Should we have an abstraction for networking, or just use web concepts all the way through?

[Antti Koivisto]

In practice we need this sort of information through the stack. We can pass it via side channels. This is bug-prone and cumbersome. We could add a new high level request type that knows about the web and wraps a low level type that doesn't. This is adds yet another layer to the already too layered networking code.

In the end we are making a browser engine not a platonic networking platform. "Web concepts all the way through" is a decent description of WebKit architecture.

[Antti Koivisto]

> We don't think that people rewind, fast-forward, and/or revisit videos? I
> tend to do all of those things. :(

At least the current YouTube player never seems to generate the same URL twice so cache entires are never hit. For rewind and fast-forward have 5min 1024p, 314MB memory cache on MSE level (bigger than the entire disk cache).

We can't currently move data from the memory cache to MSE without copying so there is no mapped file benefit either.

We should revisit this at some point. I think we want a separate more transient media cache section on disk and we should allow copy-free use of mapped buffers through the stack.

[Antti Koivisto]

> We can't currently move data from the memory cache to MSE without copying so
> there is no mapped file benefit either.

from the disk cache I mean

[Antti Koivisto]

https://trac.webkit.org/r183467

(In reply to comment #17)
> https://trac.webkit.org/r183467

Looks like this broke the USE(CFNETWORK) iOS configuration:

Source/WebCore/platform/network/ios/ResourceRequestIOS.mm:42:7: error: member initializer 'm_mainResourceRequest' does not name a non-static data member or base class

[Andy Estes]

Mavericks Debug build fix: http://trac.webkit.org/changeset/183478

[Darin Adler]

Regarding Alexey and Antti’s discussion about “web concepts” at the lower level: I think what we want for the layering here is that the lower level exposes the concepts it uses to make the cache work, and the higher level adopts/implements those. There is no reason, in my opinion, that the lower level concepts need to be “non-web”, but we do want them to be as simple as possible, not simply plumbing through the entire high level. And obviously, “nothing at all” is the simplest, but may not be sufficient.

[Alexey Proskuryakov]

It's not really a choice about platonic vs. pragmatical, it's simple layered software design.

If you want networking to implement a policy that is easy for other engineers to understand, if should have a descriptive name (not "whatever XHR needs"), and the policies should be as centralized as possible, not scattered across multiple frameworks and processes.

In other words, policy decisions are made before a ResourceRequest is passed to NetworkProcess. It should be especially straightforward in this case, where you just want to inhibit caching.

[Antti Koivisto]

If you look at the patch you will see that the requster is used for number of non-cache related things. Those used to be handled by a side-channel and by (already) having the requester type in ResourceRequest (on iOS).

I want to keep cache decision-making in one place. I also don't want to be coy and use some other name when I really mean XHR. I would be surprised if this confused anyone. Generally less abstract code is more understandable.

[Antti Koivisto]

Also "which layer knows about which concept" kind of layering is very soft when not reflected in code dependencies. I don't think there is any "simple layered software design" hard rules here.

[Alexey Proskuryakov]

This caused bug 144533.

For the record, I still think that this is poor design. Even your own words demonstrate that - you say that one needs to look at the patch to find out what "XMLHttpRequest" means to the low level networking code. I do not think that any human can fully understand the relationship between explicit behaviors passed from upper levels and implicit ones triggered by ResourceRequest::Requester::XHR.