Bug 130728

Summary: Web Replay: add page-level setting to bypass the MemoryCache
Product: WebKit Reporter: Brian Burg <bburg>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED LATER    
Severity: Normal CC: andersca, ap, buildbot, commit-queue, darin, japhet, joepeck, kling, mhock, psolanki, rniwa, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 130938    
Bug Blocks: 129391, 130672, 131318, 131376    
Attachments:
Description Flags
the patch
none
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
none
the patch.2
none
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
none
the patch.3
none
the patch.4 - fix rollout root cause
none
rebased patch, improved isEphemeral consistency.
none
rebased, added setting client callsite none

Description Brian Burg 2014-03-25 10:36:36 PDT
When replaying a specific Page we don't want to store its cached resources in the MemoryCache. This patch is for adding a new Setting to Page (independent of replay) that controls this behavior, and writing tests for the setting.
Comment 1 Brian Burg 2014-03-25 13:08:47 PDT
Created attachment 227785 [details]
the patch
Comment 2 Martin Hock 2014-03-25 13:31:47 PDT
Comment on attachment 227785 [details]
the patch

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

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:502
> +    ASSERT(sessionID() != SessionID::bypassCacheSessionID());

Are there more places that we can add this ASSERT?  It may make sense in many of the places where we check .isValid() on a SessionID.

> Source/WebCore/page/SessionID.h:46
> +    static SessionID bypassCacheSessionID() { return SessionID(3); }

You also need to update APISession.cpp's generateID() function:
    static uint64_t uniqueSessionID = WebCore::SessionID::bypassCacheSessionID().sessionID();

I don't think you need to add API::session::bypassCacheSession().
Comment 3 Build Bot 2014-03-25 14:18:07 PDT
Comment on attachment 227785 [details]
the patch

Attachment 227785 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6099854723907584

New failing tests:
http/tests/cache/bypass-memory-cache-after-reload.html
Comment 4 Build Bot 2014-03-25 14:18:11 PDT
Created attachment 227795 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 5 Build Bot 2014-03-25 14:34:12 PDT
Comment on attachment 227785 [details]
the patch

Attachment 227785 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6132809672425472

New failing tests:
http/tests/cache/bypass-memory-cache-after-reload.html
Comment 6 Build Bot 2014-03-25 14:34:17 PDT
Created attachment 227797 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 7 Brian Burg 2014-03-25 15:34:48 PDT
(In reply to comment #2)
> (From update of attachment 227785 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227785&action=review
> 
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:502
> > +    ASSERT(sessionID() != SessionID::bypassCacheSessionID());
> 
> Are there more places that we can add this ASSERT?  It may make sense in many of the places where we check .isValid() on a SessionID.

My intent is to have the asserts in any place where we assume !MemoryCache::disabled().

> > Source/WebCore/page/SessionID.h:46
> > +    static SessionID bypassCacheSessionID() { return SessionID(3); }
> 
> You also need to update APISession.cpp's generateID() function:
>     static uint64_t uniqueSessionID = WebCore::SessionID::bypassCacheSessionID().sessionID();

Will do, thanks for the heads-up.

> I don't think you need to add API::session::bypassCacheSession().

Rgr
Comment 8 Brian Burg 2014-03-25 15:47:57 PDT
(In reply to comment #5)
> (From update of attachment 227785 [details])
> Attachment 227785 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.appspot.com/results/6132809672425472
> 
> New failing tests:
> http/tests/cache/bypass-memory-cache-after-reload.html

I need to rebaseline this test, as I had a conflicting fix for 130632 underneath. Not sure about that one crash, though.
Comment 9 Brian Burg 2014-03-25 15:56:22 PDT
Created attachment 227812 [details]
the patch.2
Comment 10 Build Bot 2014-03-25 17:27:59 PDT
Comment on attachment 227812 [details]
the patch.2

Attachment 227812 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6616075701583872

New failing tests:
http/tests/cache/bypass-memory-cache-after-reload.html
Comment 11 Build Bot 2014-03-25 17:28:02 PDT
Created attachment 227817 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 12 Brian Burg 2014-03-25 18:04:18 PDT
(In reply to comment #11)
> Created an attachment (id=227817) [details]
> Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
> 
> The attached test failures were seen while running run-webkit-tests on the mac-ews.
> Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

On mac-mountainlion, it seems that it is not logging loader callbacks for the favicon, but these callbacks showed up in my WK2 baseline when running the test by itself. I am guessing that it is being requested before the test page can tell the page to bypass the memory cache.

Alternatively, it seems that DRT always disables icon database while WKTR doesn't disable the icon database, but this would cause other tests that dump resource loader callbacks to look wrong as well.
Comment 13 Build Bot 2014-03-25 23:08:22 PDT
Comment on attachment 227812 [details]
the patch.2

Attachment 227812 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6515420727083008

New failing tests:
http/tests/cache/bypass-memory-cache-after-reload.html
Comment 14 Build Bot 2014-03-25 23:08:28 PDT
Created attachment 227828 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 15 Build Bot 2014-03-26 00:09:36 PDT
Comment on attachment 227812 [details]
the patch.2

Attachment 227812 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5975871567429632

New failing tests:
http/tests/cache/bypass-memory-cache-after-reload.html
Comment 16 Build Bot 2014-03-26 00:09:42 PDT
Created attachment 227830 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 17 Brian Burg 2014-03-27 16:29:02 PDT
Created attachment 228004 [details]
the patch.3
Comment 18 Brian Burg 2014-03-27 16:34:07 PDT
I could not find a way to test this setting by dumping resource loader callbacks, because WK and WK2 differ in their timing of starting the IconLoader, which affects whether they hit the memory cache before the test can disable it.

For WK1, this happens when the main resource has finished loading, so the icon load request will hit the memory cache before the test script is able to disable the memory cache for requests that come from its page. So, for WK1 there is never a network request for the icon.

For WK2, the icon request always misses the cache, possibly because it is send to NetworkProcess on the next runloop or something like that.

In any case, this setting will be used every time we capture or replay an execution. So the code will be covered by those test cases for ports that build with ENABLE_WEB_REPLAY.

If desired I could wrap this setting with ENABLE_WEB_REPLAY.
Comment 19 Brian Burg 2014-03-27 17:07:02 PDT
Oops, should have removed the InternalSettings entry since there's no test using it now.
Comment 20 Timothy Hatcher 2014-03-27 21:06:12 PDT
We can check in different results for WK1 and WK2.
Comment 21 Brian Burg 2014-03-28 15:46:51 PDT
Committed r166434: <http://trac.webkit.org/changeset/166434>
Comment 22 Alexey Proskuryakov 2014-03-29 22:44:09 PDT
This change caused crashes and failures on various bots:

http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK2%20(Tests)/r166457%20(16884)/http/tests/cache/bypass-memory-cache-after-reload-crash-log.txt

http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r166457%20(3665)/http/tests/cache/bypass-memory-cache-after-reload-pretty-diff.html

Another regressed test is http/tests/cache/cached-main-resource.html, but there is no crash log captured on bots.
Comment 23 Alexey Proskuryakov 2014-03-29 22:50:44 PDT
Looks like http/tests/cache/content-type-ignored-during-revalidation.html too:

http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fcache%2Fcontent-type-ignored-during-revalidation.html
Comment 24 WebKit Commit Bot 2014-03-29 22:51:52 PDT
Re-opened since this is blocked by bug 130938
Comment 25 Alexey Proskuryakov 2014-03-29 23:02:13 PDT
Rolled out in <https://trac.webkit.org/r166459>.
Comment 26 Brian Burg 2014-03-30 09:55:37 PDT
(In reply to comment #22)
> This change caused crashes and failures on various bots:
> 
> http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK2%20(Tests)/r166457%20(16884)/http/tests/cache/bypass-memory-cache-after-reload-crash-log.txt
> 
> http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r166457%20(3665)/http/tests/cache/bypass-memory-cache-after-reload-pretty-diff.html
> 
> Another regressed test is http/tests/cache/cached-main-resource.html, but there is no crash log captured on bots.

Will investigate. Things did look fine on EWS bots, so our testing strategy here (dumping resource loader callbacks in the presence of NetworkProcess) may be too flaky for the test to be useful. I will fix the crash, though.
Comment 27 Brian Burg 2014-03-30 22:39:24 PDT
It seems I forgot to add a special case which returns the default networking context if the SessionID is our special value. So in some cases (surprisingly not many, though) we would try to dereference a null session elsewhere, usually when scheduling resource loaders or pulling cookies out of the session.
Comment 28 Brian Burg 2014-03-31 16:32:00 PDT
Created attachment 228218 [details]
the patch.4 - fix rollout root cause
Comment 29 Brian Burg 2014-04-06 00:03:44 PDT
Created attachment 228695 [details]
rebased patch, improved isEphemeral consistency.
Comment 30 Timothy Hatcher 2014-04-06 09:29:11 PDT
Comment on attachment 228695 [details]
rebased patch, improved isEphemeral consistency.

Seems fine to me. I'll let others take a look.
Comment 31 Martin Hock 2014-04-08 10:21:50 PDT
Comment on attachment 228695 [details]
rebased patch, improved isEphemeral consistency.

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

> Source/WebCore/page/Page.cpp:1086
> -    // Don't allow changing the legacy private browsing state if we have set a session ID.
> -    ASSERT(m_sessionID == SessionID::defaultSessionID() || m_sessionID == SessionID::legacyPrivateSessionID());
> +    // Don't allow changing the legacy private browsing state if we have set an ephemeral session ID.
> +    ASSERT(m_sessionID.isEphemeral() || m_sessionID == SessionID::legacyPrivateSessionID());

This code won't work as we need to allow enabling private browsing when we have a default session ID. I think the original code here was correct, unless it doesn't play well with bypassCacheSessionID. The idea behind this code is that we don't enable legacy private browsing unless we already had a legacy-compatible ID, which is either the default session ID or the legacy private session ID. I'm not sure if you need to allow enabling legacy private browsing in the presence of bypassCacheSessionID - the problem is that toggling private browsing on and off would then eradicate the bypassCacheSessionID. If not, leave the code as-is; otherwise, you could either spell out all the cases or do
ASSERT(!m_sessionID.isEphemeral() || m_sessionID == SessionID::legacyPrivateSessionID());
But then you'll need to figure out how to deal with bypassCacheSessionID getting replaced by defaultSessionID when legacy private browsing is disabled.

Even for today's code, a better comment would be:
// Don't allow changing the legacy private browsing state if we have set a non-legacy session ID.
Comment 32 Martin Hock 2014-04-08 10:47:12 PDT
(In reply to comment #31)
> But then you'll need to figure out how to deal with bypassCacheSessionID getting replaced by defaultSessionID when legacy private browsing is disabled.

The key to whether a value will be overwritten is in the logic found in WebPage::updatePreferences:

    if (store.getBoolValueForKey(WebPreferencesKey::privateBrowsingEnabledKey()) && !usesEphemeralSession())
        setSessionID(SessionID::legacyPrivateSessionID());
    else if (!store.getBoolValueForKey(WebPreferencesKey::privateBrowsingEnabledKey()) && sessionID() == SessionID::legacyPrivateSessionID())
        setSessionID(SessionID::defaultSessionID());

My best guess is that you want the value to remain unchanged when toggling private browsing. If so, leave this code as-is and restore the original Page.cpp code as with my first suggestion above (maybe update the comment though).
Comment 33 Brian Burg 2014-04-08 11:37:30 PDT
(In reply to comment #32)
> (In reply to comment #31)
> > But then you'll need to figure out how to deal with bypassCacheSessionID getting replaced by defaultSessionID when legacy private browsing is disabled.
> 
> The key to whether a value will be overwritten is in the logic found in WebPage::updatePreferences:
> ...
> My best guess is that you want the value to remain unchanged when toggling private browsing. If so, leave this code as-is and restore the original Page.cpp code as with my first suggestion above (maybe update the comment though).

I think I should just revert the change to the assertion.

I don't want to support the use case where the user can duck in and out of private browsing during capture or replay. If they enter during capturing, we should just cancel the current recording. If they enter during replay, it shouldn't have any effect on the replayed execution because it essentially is a private browsing session- nothing should be saved during replay anyway.

I opened a separate bug for replay/private browsing integration. <https://bugs.webkit.org/show_bug.cgi?id=131376>
Comment 34 Darin Adler 2014-04-12 13:20:37 PDT
Comment on attachment 228695 [details]
rebased patch, improved isEphemeral consistency.

I don’t really understand why this is being treated as a “setting”. I am mixed up about what settings are vs. functions on Page itself. I can’t really judge that question until I see what code would be calling setUsesMemoryCache.
Comment 35 Brian Burg 2014-04-13 12:41:36 PDT
Created attachment 229241 [details]
rebased, added setting client callsite
Comment 36 Brian Burg 2014-04-13 12:42:10 PDT
(In reply to comment #34)
> (From update of attachment 228695 [details])
> I don’t really understand why this is being treated as a “setting”. I am mixed up about what settings are vs. functions on Page itself. I can’t really judge that question until I see what code would be calling setUsesMemoryCache.

I updated the patch to contain the callsite of setUsesMemoryCache. It seems to have been forgotten when I split this patch 4 ways. Sorry!

I am not quite clear what the criteria is for putting boolean flags, etc in page settings vs on Page itself and adding getters and setters. Things like deviceScaleFactor are on Page but textSizeScaleFactor is a Setting. I made this flag a Setting because Setting::usesPageCache is already there.

For this case, the "setting" affects which SessionID the page assigns to its resources (triggering different MemoryCache behavior on those resources only).

The setting gets set and unset at the beginning and end of a capture or playback session. This tends to coincide with main frame navigations.
Comment 37 Brian Burg 2015-08-05 10:34:16 PDT
Comment on attachment 229241 [details]
rebased, added setting client callsite

Clearing r? as this patch is bitrotted.
Comment 38 Brian Burg 2017-07-10 13:59:31 PDT
Closing web replay-related bugs until we resume working on the feature again.