[Chromium] Memory cache client calls should be disabled

[Alexander Pavlov (apavlov)]

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.

[Alexander Pavlov (apavlov)]

Created attachment 44994 [details]
patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44994 [details] without any errors.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Seems this was added in https://bugs.webkit.org/show_bug.cgi?id=23160

[Alexander Pavlov (apavlov)]

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

[David Levin]

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

[David Levin]

Comment on attachment 44994 [details]
patch

r- per comments in the bug.

[Alexander Pavlov (apavlov)]

Created attachment 45087 [details]
patch (ChangeLog message augmented)

[Alexander Pavlov (apavlov)]

Created attachment 45089 [details]
patch (refined ChangeLog message as suggested)

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45089 [details] without any errors.

[Eric Seidel (no email)]

Comment on attachment 45089 [details]
patch (refined ChangeLog message as suggested)

OK.

[Pavel Feldman]

Comment on attachment 45089 [details]
patch (refined ChangeLog message as suggested)

Alexander does not have a commit bit yet.

[WebKit Commit Bot]

Comment on attachment 45089 [details]
patch (refined ChangeLog message as suggested)

Clearing flags on attachment: 45089

Committed r52291: <http://trac.webkit.org/changeset/52291>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Fisher (:fishd, Google)]

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.

[Darin Fisher (:fishd, Google)]

I suspect IN_PROC_BROWSER_TEST_F(SSLUITest, TestCachedMixedContents) will start failing.  That's a test in chrome/browser/ssl/ssl_browser_tests.cc.

[Eric Seidel (no email)]

Started the rollout.

[Eric Seidel (no email)]

Committed r52293: <http://trac.webkit.org/changeset/52293>

[Alexander Pavlov (apavlov)]

The change described is not needed anymore.