Bug 39382 – Factor PageCache code out of FrameLoader into a PageCacheController

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nate Chapin]

Most of the logic related to PageCache can get pulled out into a separate class.  There will still need to be some references to CachedPages and CachedFrames in FrameLoader, but it can cut a couple hundred lines out of the megaclass.

[Nate Chapin]

Created attachment 56533 [details]
patch

[Darin Adler]

Comment on attachment 56533 [details]
patch

What's the relationship between the PageCache and the PageCacheController?

[Adam Barth]

Comment on attachment 56533 [details]
patch

WebCore/loader/PageCacheController.cpp:229
 +  bool PageCacheController::canCachePageContainingThisFrame(FrameLoader* frameLoader)
Probably should pass the Frame* here.

WebCore/loader/PageCacheController.cpp:328
 +  void PageCacheController::remove(HistoryItem* item)
Why does this method exist?

Maybe all or some of these methods should move onto the pageCache() object?

[WebKit Review Bot]

Attachment 56533 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2269337

[Nate Chapin]

(In reply to comment #3)
> (From update of attachment 56533 [details])
> WebCore/loader/PageCacheController.cpp:229
>  +  bool PageCacheController::canCachePageContainingThisFrame(FrameLoader* frameLoader)
> Probably should pass the Frame* here.

Yeah, I think you're right, will do.

> 
> WebCore/loader/PageCacheController.cpp:328
>  +  void PageCacheController::remove(HistoryItem* item)
> Why does this method exist?
> 
> Maybe all or some of these methods should move onto the pageCache() object?

My initial thought had been to try to full wrap pageCache() with PageCacheController, but that isn't necessary (and very well may not be desirable).

I think removing PageCacheController's get() and remove() functions makes a lot of sense (the logic of when to remove a stale page from the page cache probably belongs in PageCache).  The more I look at it though, yeah, I think there are some merits to moving all this logic into PageCache.  I'm least certain about moving canCachePage() and its logging code into PageCache, but it doesn't make sense to have PageCacheController just for that, and better to have it in PageCache than FrameLoader :)

I'll go ahead and try moving all this to PageCacheController unless I hear objections.

[Nate Chapin]

Created attachment 56640 [details]
Merged PageCacheController into PageCache

[WebKit Review Bot]

Attachment 56640 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2306381

[Nate Chapin]

Created attachment 56643 [details]
Hopefully with all the necessary #includes this time

[Adam Barth]

Comment on attachment 56643 [details]
Hopefully with all the necessary #includes this time

This looks good, although it's a bit too complicated for me to say 100% that it's correct.  Some minor comments below.

WebCore/history/PageCache.cpp:53
 +  #ifndef NDEBUG
I think we want a space after this line.

WebCore/history/PageCache.cpp:220
 +  #endif 
Similarly, I think we want a space before this line.

WebCore/history/PageCache.cpp:354
 +      if (RefPtr<CachedPage> cachedPage = item->m_cachedPage.get()) {
Why is this a RefPtr?  If we need this reference, then we'll get in trouble on line 359 below where we return a raw pointer.  Oh, I see.  In the case where we return the raw pointer, we don't remove the CachedPage, but in the case where we do, we'll end up with the last reference and delete the CachedPage when we return.  Boo.  I'm hoping to see these as minus lines later in the patch.

WebCore/loader/FrameLoader.cpp:2157
 +      Page* page = m_frame->page();
I'm not sure it's worth storing this in a temporary, but it probably doesn't matter.

WebCore/loader/FrameLoader.cpp:2181
 +          cachedPage->restore(m_frame->page());
We have a |page| temporary we could use here.

WebCore/loader/FrameLoader.cpp: 
 +              return;
I see.  So the trickiness with RefPtr is new...  Might be worth adding a comment explaining what's going one.  PageCache isn't very well tested by DumpRenderTree, so we need to be careful when changing this code.  Have you looked in manual-tests to see if there are any good page cache tests you can run manually?

[Nate Chapin]

> I see.  So the trickiness with RefPtr is new...  Might be worth adding a comment explaining what's going one.  PageCache isn't very well tested by DumpRenderTree, so we need to be careful when changing this code.  Have you looked in manual-tests to see if there are any good page cache tests you can run manually?

Re: RefPtrs, I don't think there's any reason why it's necessary, I just got a bit careless in what code samples I modeled my patch after.  I removed them.


I ran the two manual tests I could find that related to page cache and they both pass, as do the layout tests that cover page cache.  I used the layout tests for verification of this change, because there were 8 or so tests that broke in spectacular ways with early versions of this cleanup. :-)

Will submit shortly.

[Anders Carlsson]

Reverted r60256 for reason:

Causes fast/dom/prototype-inheritance-2.html to start crashing.

Committed r60263: <http://trac.webkit.org/changeset/60263>

[Adam Barth]

Here's a crash caused by this patch (null deref).

Program received signal:  “EXC_BAD_ACCESS”.

#0	0x100e749f0 in WTF::OwnPtr<WebCore::DocLoader>::get at OwnPtr.h:54
#1	0x100ef5808 in WebCore::Document::docLoader at Document.h:484
#2	0x100f93336 in WebCore::DocumentLoader::isLoadingInAPISense at DocumentLoader.cpp:394
#3	0x1010e5d10 in WebCore::FrameLoader::subframeIsLoading at FrameLoader.cpp:2596
#4	0x100f93396 in WebCore::DocumentLoader::isLoadingInAPISense at DocumentLoader.cpp:400
#5	0x101590bd0 in WebCore::logCanCacheFrameDecision at PageCache.cpp:141
#6	0x101590de2 in WebCore::logCanCacheFrameDecision at PageCache.cpp:166
#7	0x101590fe9 in WebCore::logCanCachePageDecision at PageCache.cpp:187
#8	0x1015912b6 in WebCore::PageCache::canCache at PageCache.cpp:286
#9	0x1010ed5c4 in WebCore::FrameLoader::commitProvisionalLoad at FrameLoader.cpp:2165
#10	0x100f93809 in WebCore::DocumentLoader::commitIfReady at DocumentLoader.cpp:258
#11	0x100f938d8 in WebCore::DocumentLoader::finishedLoading at DocumentLoader.cpp:265
#12	0x1010ea395 in WebCore::FrameLoader::init at FrameLoader.cpp:252
#13	0x100837947 in WebCore::Frame::init at Frame.h:369
#14	0x100834b2b in +[WebFrame(WebInternal) _createFrameWithPage:frameName:frameView:ownerElement:] at WebFrame.mm:270
#15	0x1008301c6 in +[WebFrame(WebInternal) _createSubframeWithOwnerElement:frameName:frameView:] at WebFrame.mm:284
#16	0x10083ad2c in WebFrameLoaderClient::createFrame at WebFrameLoaderClient.mm:1352
#17	0x1010f1da0 in WebCore::FrameLoader::loadSubframe at FrameLoader.cpp:429
#18	0x1010f20aa in WebCore::FrameLoader::requestFrame at FrameLoader.cpp:400
#19	0x101199133 in WebCore::HTMLFrameElementBase::openURL at HTMLFrameElementBase.cpp:114
#20	0x10119970b in WebCore::HTMLFrameElementBase::setNameAndOpenURL at HTMLFrameElementBase.cpp:173
#21	0x101199723 in WebCore::HTMLFrameElementBase::setNameAndOpenURLCallback at HTMLFrameElementBase.cpp:178
#22	0x100e3c4a8 in WebCore::ContainerNode::dispatchPostAttachCallbacks at ContainerNode.cpp:611
#23	0x100e3c5f0 in WebCore::ContainerNode::resumePostAttachCallbacks at ContainerNode.cpp:583
#24	0x10107ec4f in WebCore::Element::attach at Element.cpp:842
#25	0x101198c74 in WebCore::HTMLFrameElementBase::attach at HTMLFrameElementBase.cpp:219
#26	0x101198433 in WebCore::HTMLFrameElement::attach at HTMLFrameElement.cpp:73
#27	0x1011c641b in WebCore::HTMLParser::insertNode at HTMLParser.cpp:421
#28	0x1011cbd1c in WebCore::HTMLParser::insertNodeAfterLimitDepth at HTMLParser.cpp:247
#29	0x1011c8cc7 in WebCore::HTMLParser::parseToken at HTMLParser.cpp:319

[WebKit Review Bot]

http://trac.webkit.org/changeset/60256 might have broken Leopard Intel Debug (Tests)

[Nate Chapin]

Created attachment 57582 [details]
One option for a crash fix

So the crash in DocumentLoader::isLoadingInAPISense is triggered by trying to deref m_frame->document() when it is null.  The trivial fix for this is attached (null checking before using m_frame->document()). 

I make no claims that this is the right solution, but it's one option.  On the one hand, the return value of Frame::document() is not guaranteed to be valid and it gets null checked a bunch of other places.  On the other hand, I have yet to figure out what changed that we are hitting null cases now (I'm guessing I inadvertently changed some timing/ordering).

[Adam Barth]

Comment on attachment 57582 [details]
One option for a crash fix

I need to we need to understand this better before anding a random null check.  One thing I've done in the past is break my change down into a ton of tiny steps to see where the bug got introduced.  Once you see a tiny change the cased the bug, you might have a better idea what the issue is.

[Eric Seidel (no email)]

Comment on attachment 56643 [details]
Hopefully with all the necessary #includes this time

Cleared Adam Barth's review+ from obsolete attachment 56643 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Nate Chapin]

Created attachment 57709 [details]
Only try to cache when the main frame is being committed

I hadn't realized that I had changed the assumption of when we would try to cache.  If we maintain the interface I introduced in this patch (canCache(Page*) instead of canCache(Frame*)), there's no way for canCache() to confirm that this was called due to a commit in the main frame.  It makes sense (to me anyway) that trying to cache from something other than the main frame is wrong, so this version just adds a !m_frame->tree()->parent() check before calling canCache() in FrameLoader::commitProvisionalLoad().



Unrelated:  In my digging into isLoadingInAPISense(), I've noticed that it's recursive and checks all subframes.  Ergo, we're being unnecessarily O(n^2) when we call it on every subframe as well as the main frame.  Do we care?

[Adam Barth]

Comment on attachment 57709 [details]
Only try to cache when the main frame is being committed

This patch is missing a ChangeLog.

[Nate Chapin]

Created attachment 57779 [details]
With ChangeLog

Hmm....I don't remember reverting the ChangeLog, sorry about that.

[Adam Barth]

Comment on attachment 57779 [details]
With ChangeLog

Ok.  I understood this patch better last time.  I wish we had interdiffs.  Thanks for sticking with it.

[Nate Chapin]

(In reply to comment #20)
> (From update of attachment 57779 [details])
> Ok.  I understood this patch better last time.  I wish we had interdiffs.  Thanks for sticking with it.

Forgot to close this: http://trac.webkit.org/changeset/60688