Bug 141788

Summary:

REGRESSION(r177738): Clearing the PageCache no longer clears the PageCache.

Product:

WebKit

Reporter:

Andreas Kling <kling>

Component:

History

Assignee:

Andreas Kling <kling>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

barraclough, cdumez, commit-queue, darin, kling

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

139921

Attachments:

Description	Flags
Patch	andersca: review+
Patch for landing	none
Patch	none

Andreas Kling

Reported 2015-02-18 18:54:06 PST

We got bit by the TemporaryChange trap!

Attachments
Patch (7.41 KB, patch) 2015-02-18 18:58 PST, Andreas Kling	andersca: review+	Details Formatted Diff Diff
Patch for landing (7.44 KB, patch) 2015-02-18 21:16 PST, Andreas Kling	no flags	Details Formatted Diff Diff
Patch (4.31 KB, patch) 2015-02-18 22:37 PST, Chris Dumez	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Andreas Kling

Comment 1 2015-02-18 18:58:45 PST

Created attachment 246874 [details] Patch

Chris Dumez

Comment 2 2015-02-18 19:08:47 PST

Comment on attachment 246874 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246874&action=review > LayoutTests/fast/history/page-cache-clearing.html:1 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> <!DOCTYPE html> ? > LayoutTests/fast/history/page-cache-clearing.html:29 > + window.testRunner.notifyDone(); This is redundant, I believe finishJSTest() already does that. > LayoutTests/fast/history/page-cache-clearing.html:38 > + window.testRunner.notifyDone(); This is redundant, I believe finishJSTest() already does that. > LayoutTests/fast/history/page-cache-clearing.html:47 > + window.testRunner.waitUntilDone(); window.jsTestIsAsync = true should already take care of this.

Chris Dumez

Comment 3 2015-02-18 19:11:07 PST

Comment on attachment 246874 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246874&action=review > LayoutTests/fast/history/page-cache-clearing.html:8 > +<div id="console"></div> This should not be needed. > LayoutTests/fast/history/page-cache-clearing.html:24 > + if (cachedPageCountBefore != 0 || cachedPageCountAfter == 0) Do you mean && ? > LayoutTests/fast/history/page-cache-clearing.html:36 > + debug("FAIL - Page did not enter the page cache."); testFailed("Page did not enter the page cache") ?

Chris Dumez

Comment 4 2015-02-18 19:15:01 PST

Comment on attachment 246874 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246874&action=review > LayoutTests/fast/history/page-cache-clearing.html:46 > + window.testRunner.overridePreference("WebKitUsesPageCachePreferenceKey", 1); This doesn't really need to be done in the onload handler. > LayoutTests/fast/history/page-cache-clearing.html:51 > + setTimeout(function() { Do we really need this setTimeout. Cannot we simply set href in the onload handler?

Andreas Kling

Comment 5 2015-02-18 21:16:52 PST

Created attachment 246877 [details] Patch for landing Fixed the passing condition in the test as pointed out by Chris.

Chris Dumez

Comment 6 2015-02-18 21:20:43 PST

Comment on attachment 246877 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=246877&action=review > LayoutTests/fast/history/page-cache-clearing.html:19 > + debug("Cached page count before clearing: " + cachedPageCountBefore); Isn't printing the pageCount here dangerous? How do we know there is always the same amount of pages in the page cache when running this test? > LayoutTests/fast/history/page-cache-clearing.html:29 > + window.testRunner.notifyDone(); still redundant. > LayoutTests/fast/history/page-cache-clearing.html:38 > + window.testRunner.notifyDone(); still redundant. > LayoutTests/fast/history/page-cache-clearing.html:47 > + window.testRunner.waitUntilDone(); still redundant.

Andreas Kling

Comment 7 2015-02-18 21:28:12 PST

(In reply to comment #6) > Comment on attachment 246877 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=246877&action=review > > > LayoutTests/fast/history/page-cache-clearing.html:19 > > + debug("Cached page count before clearing: " + cachedPageCountBefore); > > Isn't printing the pageCount here dangerous? How do we know there is always > the same amount of pages in the page cache when running this test? AFAIK the page cache is disabled between tests, as part of the "reset everything between tests so conditions are consistent" mechanism. Sent from my iThing.

Chris Dumez

Comment 8 2015-02-18 21:37:34 PST

This breaks Windows build due to missing symbol export according to EWS.

WebKit Commit Bot

Comment 9 2015-02-18 22:07:56 PST

Comment on attachment 246877 [details] Patch for landing Clearing flags on attachment: 246877 Committed r180337: <http://trac.webkit.org/changeset/180337>

WebKit Commit Bot

Comment 10 2015-02-18 22:08:00 PST

All reviewed patches have been landed. Closing bug.

Chris Dumez

Comment 11 2015-02-18 22:18:41 PST

Windows build fix landed in <http://trac.webkit.org/changeset/180338>.

Chris Dumez

Comment 12 2015-02-18 22:37:00 PST

Reopening to attach new patch.

Chris Dumez

Comment 13 2015-02-18 22:37:02 PST

Created attachment 246882 [details] Patch

Chris Dumez

Comment 14 2015-02-18 22:38:00 PST

Attaching a follow-up patch to clean up the layout test a bit.

Andreas Kling

Comment 15 2015-02-18 22:54:22 PST

Comment on attachment 246882 [details] Patch r=me Generally speaking I like tests to be a bit idiosyncratic since that increases our general code coverage. That said I don't mind these changes. And thanks for fixing the windows build!

WebKit Commit Bot

Comment 16 2015-02-18 23:37:22 PST

Comment on attachment 246882 [details] Patch Clearing flags on attachment: 246882 Committed r180339: <http://trac.webkit.org/changeset/180339>

WebKit Commit Bot

Comment 17 2015-02-18 23:37:28 PST

All reviewed patches have been landed. Closing bug.

Chris Dumez

Comment 18 2015-03-06 10:40:41 PST

Radar: rdar://problem/20072369

Chris Dumez

Comment 19 2015-03-06 10:47:27 PST

This is a regression from r177738, not r179347.

Note You need to log in before you can comment on or make changes to this bug.