Created attachment 227785 [details] the patch

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 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

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 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

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

(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

(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.

Created attachment 227812 [details] the patch.2

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

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

(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 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

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 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

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

Created attachment 228004 [details] the patch.3

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.

Oops, should have removed the InternalSettings entry since there's no test using it now.

We can check in different results for WK1 and WK2.

Committed r166434: <http://trac.webkit.org/changeset/166434>

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.

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

Re-opened since this is blocked by bug 130938

Rolled out in <https://trac.webkit.org/r166459>.

(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.

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.

Created attachment 228218 [details] the patch.4 - fix rollout root cause

Created attachment 228695 [details] rebased patch, improved isEphemeral consistency.

Comment on attachment 228695 [details] rebased patch, improved isEphemeral consistency. Seems fine to me. I'll let others take a look.

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.

(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).

(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 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.

Created attachment 229241 [details] rebased, added setting client callsite

(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 on attachment 229241 [details] rebased, added setting client callsite Clearing r? as this patch is bitrotted.

Closing web replay-related bugs until we resume working on the feature again.