Bug 32618

Summary: [Chromium] Memory cache client calls should be disabled
Product: WebKit Reporter: Alexander Pavlov (apavlov) <apavlov>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED WONTFIX    
Severity: Normal CC: commit-queue, eric, fishd, jparent, levin, ojan, pfeldman, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
patch
levin: review-
patch (ChangeLog message augmented)
none
patch (refined ChangeLog message as suggested) none

Description Alexander Pavlov (apavlov) 2009-12-16 10:42:09 PST
A Chromium issue http://code.google.com/p/chromium/issues/detail?id=29076 is caused by Frame::loadedResourceFromMemoryCache() calling requestFromDelegate(). It creates a new identifier for the same resource on reload, unless page->areMemoryCacheClientCallsEnabled() == false.
It is always the case in Safari (which sets the flag to false from the outset), so we should just emulate the behaviour.
Comment 1 Alexander Pavlov (apavlov) 2009-12-16 10:52:30 PST
Created attachment 44994 [details]
patch
Comment 2 WebKit Review Bot 2009-12-16 10:54:37 PST
style-queue ran check-webkit-style on attachment 44994 [details] without any errors.
Comment 3 Eric Seidel (no email) 2009-12-16 13:12:23 PST
Comment on attachment 44994 [details]
patch

What is a memoryCacheClientCall?   Seems the ChangeLog might want to mention why we're disabling this.  In general the change seems fine.
Comment 4 Eric Seidel (no email) 2009-12-16 13:14:23 PST
Seems this was added in https://bugs.webkit.org/show_bug.cgi?id=23160
Comment 5 Alexander Pavlov (apavlov) 2009-12-16 16:36:39 PST
(In reply to comment #3)
> (From update of attachment 44994 [details])
> What is a memoryCacheClientCall?   Seems the ChangeLog might want to mention
> why we're disabling this.  In general the change seems fine.

Eric, does mentioning the reasons similar to those in my original description for this bug seem fine with you?

Basically, the active code path with memoryCacheClientCallsEnabled == true creates duplicate resource IDs for cached resources that are loaded in a way similar to "(new Image()).src = 'foo.png'" (I'm afraid to touch the solid ground of this behavior as it stems from the JavaScript VM handling DOM, but it seems Safari will exhibit the same bug as Chromium, without disabling the AforeMentionedCalls.)
Comment 6 David Levin 2009-12-17 10:36:10 PST
(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 44994 [details] [details])
> > What is a memoryCacheClientCall?   Seems the ChangeLog might want to mention
> > why we're disabling this.  In general the change seems fine.
> 
> Eric, does mentioning the reasons similar to those in my original description
> for this bug seem fine with you?

Yes or something more terse even like this:
"Disable memory cache client calls so that a new identifier isn't created for the same resource on reload."
Comment 7 David Levin 2009-12-17 10:36:30 PST
Comment on attachment 44994 [details]
patch

r- per comments in the bug.
Comment 8 Alexander Pavlov (apavlov) 2009-12-17 10:40:45 PST
Created attachment 45087 [details]
patch (ChangeLog message augmented)
Comment 9 Alexander Pavlov (apavlov) 2009-12-17 10:44:58 PST
Created attachment 45089 [details]
patch (refined ChangeLog message as suggested)
Comment 10 WebKit Review Bot 2009-12-17 10:46:12 PST
style-queue ran check-webkit-style on attachment 45089 [details] without any errors.
Comment 11 Eric Seidel (no email) 2009-12-17 13:01:01 PST
Comment on attachment 45089 [details]
patch (refined ChangeLog message as suggested)

OK.
Comment 12 Pavel Feldman 2009-12-17 14:05:56 PST
Comment on attachment 45089 [details]
patch (refined ChangeLog message as suggested)

Alexander does not have a commit bit yet.
Comment 13 WebKit Commit Bot 2009-12-17 14:14:46 PST
Comment on attachment 45089 [details]
patch (refined ChangeLog message as suggested)

Clearing flags on attachment: 45089

Committed r52291: <http://trac.webkit.org/changeset/52291>
Comment 14 WebKit Commit Bot 2009-12-17 14:14:55 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Darin Fisher (:fishd, Google) 2009-12-17 14:35:32 PST
Does this change suppress calls to FrameLoaderClient::dispatchDidLoadResourceFromMemoryCache?  If so, then it just broke updating the SSL lock indicator.  We use this very important signal to know when a resource is loaded from the memory cache in case that resource was originally fetched over broken SSL.
Comment 16 Darin Fisher (:fishd, Google) 2009-12-17 14:41:11 PST
I suspect IN_PROC_BROWSER_TEST_F(SSLUITest, TestCachedMixedContents) will start failing.  That's a test in chrome/browser/ssl/ssl_browser_tests.cc.
Comment 17 Eric Seidel (no email) 2009-12-17 15:09:38 PST
Started the rollout.
Comment 18 Eric Seidel (no email) 2009-12-17 15:22:47 PST
Committed r52293: <http://trac.webkit.org/changeset/52293>
Comment 19 Alexander Pavlov (apavlov) 2010-07-22 08:51:55 PDT
The change described is not needed anymore.