Bug 39382 - Factor PageCache code out of FrameLoader into a PageCacheController
Summary: Factor PageCache code out of FrameLoader into a PageCacheController
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-19 15:34 PDT by Nate Chapin
Modified: 2010-06-09 13:51 PDT (History)
5 users (show)

See Also:


Attachments
patch (39.42 KB, patch)
2010-05-19 16:49 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Merged PageCacheController into PageCache (31.36 KB, patch)
2010-05-20 15:17 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Hopefully with all the necessary #includes this time (31.41 KB, patch)
2010-05-20 16:03 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
One option for a crash fix (30.62 KB, patch)
2010-06-01 12:46 PDT, Nate Chapin
abarth: review-
Details | Formatted Diff | Diff
Only try to cache when the main frame is being committed (29.88 KB, patch)
2010-06-02 15:40 PDT, Nate Chapin
abarth: review-
Details | Formatted Diff | Diff
With ChangeLog (31.43 KB, patch)
2010-06-03 09:30 PDT, Nate Chapin
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2010-05-19 15:34:22 PDT
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.
Comment 1 Nate Chapin 2010-05-19 16:49:34 PDT
Created attachment 56533 [details]
patch
Comment 2 Darin Adler 2010-05-19 17:49:50 PDT
Comment on attachment 56533 [details]
patch

What's the relationship between the PageCache and the PageCacheController?
Comment 3 Adam Barth 2010-05-19 17:52:09 PDT
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?
Comment 4 WebKit Review Bot 2010-05-19 18:03:17 PDT
Attachment 56533 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2269337
Comment 5 Nate Chapin 2010-05-20 10:01:30 PDT
(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.
Comment 6 Nate Chapin 2010-05-20 15:17:09 PDT
Created attachment 56640 [details]
Merged PageCacheController into PageCache
Comment 7 WebKit Review Bot 2010-05-20 15:54:02 PDT
Attachment 56640 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2306381
Comment 8 Nate Chapin 2010-05-20 16:03:13 PDT
Created attachment 56643 [details]
Hopefully with all the necessary #includes this time
Comment 9 Adam Barth 2010-05-26 10:10:03 PDT
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?
Comment 10 Nate Chapin 2010-05-26 15:33:48 PDT
> 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.
Comment 11 Anders Carlsson 2010-05-26 18:03:32 PDT
Reverted r60256 for reason:

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

Committed r60263: <http://trac.webkit.org/changeset/60263>
Comment 12 Adam Barth 2010-05-26 18:21:45 PDT
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
Comment 13 WebKit Review Bot 2010-05-26 20:14:06 PDT
http://trac.webkit.org/changeset/60256 might have broken Leopard Intel Debug (Tests)
Comment 14 Nate Chapin 2010-06-01 12:46:40 PDT
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).
Comment 15 Adam Barth 2010-06-01 22:25:15 PDT
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.
Comment 16 Eric Seidel (no email) 2010-06-02 02:26:46 PDT
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.
Comment 17 Nate Chapin 2010-06-02 15:40:58 PDT
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?
Comment 18 Adam Barth 2010-06-03 00:26:09 PDT
Comment on attachment 57709 [details]
Only try to cache when the main frame is being committed

This patch is missing a ChangeLog.
Comment 19 Nate Chapin 2010-06-03 09:30:23 PDT
Created attachment 57779 [details]
With ChangeLog

Hmm....I don't remember reverting the ChangeLog, sorry about that.
Comment 20 Adam Barth 2010-06-03 10:23:13 PDT
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.
Comment 21 Nate Chapin 2010-06-09 13:51:57 PDT
(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