Bug 147783 – Expand network cache tests to cover memory cache behavior

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

[Antti Koivisto]

Disk cache tests can used to also improve memory cache test coverage.

[Antti Koivisto]

Created attachment 258501 [details]
patch

[Alexey Proskuryakov]

Comment on attachment 258501 [details]
patch

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

Very nicely expanded testing! I have one general question, and some nits about the code.

What makes these tests reliable and repeatable? Could these resources get purged randomly? I just checked, and we have quite a few existing cache tests that are flaky:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=cache%2F

> Source/WebCore/loader/cache/CachedRawResource.cpp:144
> +        if (validationInProgress())
> +            response.setSource(ResourceResponse::Source::MemoryCacheAfterValidation);

This code looks quite strange. Do we really call client->responseReceived before we know that revalidation succeeded? Or is validationInProgress a misnomer, and validation is actually complete and successful when this code runs?

> Source/WebCore/loader/cache/CachedRawResource.cpp:184
> -        c->responseReceived(this, m_response);
> +        c->responseReceived(this, response);

This change is not mentioned in ChangeLog, what does it do?

> Source/WebCore/platform/network/ResourceResponseBase.h:110
> +    // This is primarily for testing support. It is not necessarily accurate in all scenarios.

Generally, we prefer adding such information to function name, to make sure that it's not overlooked (e.g. "deprecatedFoo", or "barForTesting"). Also, another engineer wouldn't know how much to trust the source even for testing.

Adding a comment is an improvement over having an unreliable function with no comment though, so I'm not asking for this to be necessarily improved further now.

[Antti Koivisto]

> What makes these tests reliable and repeatable? Could these resources get
> purged randomly? I just checked, and we have quite a few existing cache
> tests that are flaky:
> 
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=cache%2F

If I understand these correctly http/tests/cache/disk-cache/ tests (which this is about) look stable on Apple platforms newer than Mavericks. In any case thit shouldn't make these tests any more or less stable.

> > Source/WebCore/loader/cache/CachedRawResource.cpp:144
> > +        if (validationInProgress())
> > +            response.setSource(ResourceResponse::Source::MemoryCacheAfterValidation);
> 
> This code looks quite strange. Do we really call client->responseReceived
> before we know that revalidation succeeded? Or is validationInProgress a
> misnomer, and validation is actually complete and successful when this code
> runs?

This code runs when validation is just finishing. The whole thing is ugly but it is what it is. 

One of the reasons I'm adding test coverage is so that we can refactor memory cache to be better with more confidence in not breaking it.

> > Source/WebCore/loader/cache/CachedRawResource.cpp:184
> > -        c->responseReceived(this, m_response);
> > +        c->responseReceived(this, response);
> 
> This change is not mentioned in ChangeLog, what does it do?

Passes on the argument response object instead of the member since it can now be different. This was probably an oversight in the existing code since it didn't make any difference.

[Alexey Proskuryakov]

Comment on attachment 258501 [details]
patch

At least one of the tests was more flaky on Yosemite than on Mavericks, so it had to be marked flaky:

LayoutTests/platform/mac-wk2/TestExpectations:webkit.org/b/147075 [ Release Yosemite ] http/tests/cache/disk-cache/disk-cache-disable.html [ Pass Failure ]

There is really no difference between "current" and "not current" Apple OS here. If anything, Mavericks is more important, because the bots are generally more stable (not going into details now), so failures are more likely to indicate WebKit bugs, and need to be looked into more promptly.

[Antti Koivisto]

https://trac.webkit.org/r188211