Bug 127794 - Add support for sessions to MemoryCache
Summary: Add support for sessions to MemoryCache
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Martin Hock
URL:
Keywords:
Depends on: 129646
Blocks:
  Show dependency treegraph
 
Reported: 2014-01-28 14:01 PST by Martin Hock
Modified: 2014-03-05 12:05 PST (History)
8 users (show)

See Also:


Attachments
patch (14.67 KB, patch)
2014-01-28 14:20 PST, Martin Hock
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (347.34 KB, application/zip)
2014-01-28 15:55 PST, Build Bot
no flags Details
patch (23.04 KB, patch)
2014-02-10 13:49 PST, Martin Hock
no flags Details | Formatted Diff | Diff
Remove extraneous change to WebCache.mm (21.67 KB, patch)
2014-02-10 16:26 PST, Martin Hock
no flags Details | Formatted Diff | Diff
Create another MemoryCache level; make WebCore sessionID-aware (77.26 KB, patch)
2014-02-18 12:50 PST, Martin Hock
no flags Details | Formatted Diff | Diff
refactor (92.67 KB, patch)
2014-02-20 10:31 PST, Martin Hock
no flags Details | Formatted Diff | Diff
split out SessionID changes (129141) (65.06 KB, patch)
2014-02-26 15:12 PST, Martin Hock
no flags Details | Formatted Diff | Diff
fix WebCache.mm (65.90 KB, patch)
2014-02-26 19:38 PST, Martin Hock
no flags Details | Formatted Diff | Diff
restore MemoryCache::resourceForURL(const URL&) (64.34 KB, patch)
2014-02-27 17:25 PST, Martin Hock
no flags Details | Formatted Diff | Diff
build fix (66.00 KB, patch)
2014-02-27 20:53 PST, Martin Hock
sam: review+
Details | Formatted Diff | Diff
rebase (66.02 KB, patch)
2014-03-03 14:14 PST, Martin Hock
no flags Details | Formatted Diff | Diff
support multithreading in removeRequestFromSessionCaches (67.14 KB, patch)
2014-03-04 13:11 PST, Martin Hock
no flags Details | Formatted Diff | Diff
misplaced & (67.14 KB, patch)
2014-03-04 13:15 PST, Martin Hock
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Hock 2014-01-28 14:01:44 PST
Currently, the memory cache ignores sessions.  Different sessions should not see each other's cache.
Comment 1 Martin Hock 2014-01-28 14:20:10 PST
Created attachment 222488 [details]
patch
Comment 2 Vicki Pfau 2014-01-28 14:30:23 PST
Comment on attachment 222488 [details]
patch

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

> Source/WebCore/platform/network/cf/ResourceRequest.h:171
> +        const String& setPartitionSession() const { return m_partitionSession = (m_cachePartition.isNull() ? emptyString() : m_cachePartition) + '|' + String::number(m_sessionID); }

Given that this takes no arguments and returns a value, it probably shouldn't be called set*. I'm a bit confused as to why you have it both do an assignment and return the value in one operation, however.
Comment 3 Build Bot 2014-01-28 15:55:15 PST
Comment on attachment 222488 [details]
patch

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

New failing tests:
editing/unsupported-content/list-type-before.html
editing/unsupported-content/table-type-before.html
editing/unsupported-content/list-delete-003.html
editing/unsupported-content/list-delete-001.html
editing/unsupported-content/list-type-after.html
editing/unsupported-content/table-delete-002.html
editing/unsupported-content/table-type-after.html
Comment 4 Build Bot 2014-01-28 15:55:16 PST
Created attachment 222521 [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 5 Martin Hock 2014-02-10 13:49:04 PST
Created attachment 223739 [details]
patch
Comment 6 Martin Hock 2014-02-10 16:26:46 PST
Created attachment 223766 [details]
Remove extraneous change to WebCache.mm
Comment 7 Martin Hock 2014-02-18 12:50:58 PST
Created attachment 224535 [details]
Create another MemoryCache level; make WebCore sessionID-aware
Comment 8 Martin Hock 2014-02-20 10:31:35 PST
Created attachment 224770 [details]
refactor
Comment 9 Martin Hock 2014-02-26 15:12:28 PST
Created attachment 225304 [details]
split out SessionID changes (129141)
Comment 10 Martin Hock 2014-02-26 19:38:48 PST
Created attachment 225338 [details]
fix WebCache.mm
Comment 11 Martin Hock 2014-02-27 17:25:38 PST
Created attachment 225432 [details]
restore MemoryCache::resourceForURL(const URL&)
Comment 12 Martin Hock 2014-02-27 20:53:32 PST
Created attachment 225435 [details]
build fix
Comment 13 Martin Hock 2014-03-03 14:14:56 PST
Created attachment 225691 [details]
rebase
Comment 14 WebKit Commit Bot 2014-03-03 15:02:12 PST
Comment on attachment 225691 [details]
rebase

Clearing flags on attachment: 225691

Committed r165013: <http://trac.webkit.org/changeset/165013>
Comment 15 WebKit Commit Bot 2014-03-03 15:02:17 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Alexey Proskuryakov 2014-03-03 17:08:19 PST
Comment on attachment 225691 [details]
rebase

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

> Source/WebCore/loader/cache/MemoryCache.cpp:867
> +void MemoryCache::removeRequestFromSessionCaches(ScriptExecutionContext*, const ResourceRequest& request)
> +{
> +    for (auto& resources : memoryCache()->m_sessionResources) {
> +        if (CachedResource* resource = memoryCache()->resourceForRequestImpl(request, *resources.value))
> +        memoryCache()->remove(resource);
> +    }

Unlike all other functions around, this new function is not thread safe, which caused assertion failures on bots.

See <http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r165015%20(3510)/fast/files/workers/worker-apply-blob-url-to-xhr-crash-log.txt>.

> Source/WebCore/loader/cache/MemoryCache.cpp:897
> +    for (auto &resources : m_sessionResources) {

Misplaced &
Comment 17 WebKit Commit Bot 2014-03-03 17:12:08 PST
Re-opened since this is blocked by bug 129646
Comment 18 Martin Hock 2014-03-04 13:11:36 PST
Created attachment 225806 [details]
support multithreading in removeRequestFromSessionCaches
Comment 19 Martin Hock 2014-03-04 13:15:58 PST
Created attachment 225807 [details]
misplaced &
Comment 20 Martin Hock 2014-03-04 13:19:18 PST
(In reply to comment #16)
> (From update of attachment 225691 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225691&action=review
> 
> > Source/WebCore/loader/cache/MemoryCache.cpp:867
> > +void MemoryCache::removeRequestFromSessionCaches(ScriptExecutionContext*, const ResourceRequest& request)
> > +{
> > +    for (auto& resources : memoryCache()->m_sessionResources) {
> > +        if (CachedResource* resource = memoryCache()->resourceForRequestImpl(request, *resources.value))
> > +        memoryCache()->remove(resource);
> > +    }
> 
> Unlike all other functions around, this new function is not thread safe, which caused assertion failures on bots.
> 
> See <http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r165015%20(3510)/fast/files/workers/worker-apply-blob-url-to-xhr-crash-log.txt>.

This should be fixed - I've run all the tests and I don't think any of the minor flakiness I observe is due to my changes.

> > Source/WebCore/loader/cache/MemoryCache.cpp:897
> > +    for (auto &resources : m_sessionResources) {
> 
> Misplaced &

Fixed in second patch, thanks.
Comment 21 WebKit Commit Bot 2014-03-05 12:05:22 PST
Comment on attachment 225807 [details]
misplaced &

Clearing flags on attachment: 225807

Committed r165117: <http://trac.webkit.org/changeset/165117>
Comment 22 WebKit Commit Bot 2014-03-05 12:05:29 PST
All reviewed patches have been landed.  Closing bug.