Bug 120710

Summary: Cached Page and Frame don't need to be ref-counted.
Product: WebKit Reporter: Andreas Kling <kling>
Component: Page LoadingAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ggaren, japhet, kling
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 120758    
Bug Blocks:    
Attachments:
Description Flags
Patch
andersca: review+
Patch v2 none

Andreas Kling
Reported 2013-09-04 17:29:14 PDT
CachedPage and CachedFrame are single-owner objects.
Attachments
Patch (7.13 KB, patch)
2013-09-04 17:36 PDT, Andreas Kling 		andersca: review+
DetailsFormatted DiffDiff
Patch v2 (10.79 KB, patch)
2013-09-05 10:52 PDT, Andreas Kling 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Andreas Kling
Comment 1 2013-09-04 17:36:41 PDT
Created attachment 210530 [details] Patch
Anders Carlsson
Comment 2 2013-09-04 17:47:14 PDT
Comment on attachment 210530 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=210530&action=review > Source/WebCore/loader/FrameLoader.cpp:1762 > + cachedPage = 0; Nullptr!
Andreas Kling
Comment 3 2013-09-04 18:40:50 PDT
Committed r155086: <http://trac.webkit.org/changeset/155086>
Geoffrey Garen
Comment 4 2013-09-04 22:33:34 PDT
Comment on attachment 210530 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=210530&action=review > Source/WebCore/loader/FrameLoader.cpp:1747 > + // Clear out 'cachedPage' right away since it now points to a deleted object. > + cachedPage = 0; This kind of stinks. This function used to be guaranteed-memory-safe by the compiler. Now it's unsafe, with a comment. And the comment doesn't explain how we know our pointer wasn't deleted sooner. If the memory model is that history().provisionalItem() owns the cached page, perhaps PageCache::get should return PassRefPtr<HistoryItem>, and we just shouldn't put cachedPage into a local variable.
Andreas Kling
Comment 5 2013-09-04 22:38:43 PDT
(In reply to comment #4) > (From update of attachment 210530 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=210530&action=review > > > Source/WebCore/loader/FrameLoader.cpp:1747 > > + // Clear out 'cachedPage' right away since it now points to a deleted object. > > + cachedPage = 0; > > This kind of stinks. This function used to be guaranteed-memory-safe by the compiler. Now it's unsafe, with a comment. And the comment doesn't explain how we know our pointer wasn't deleted sooner. > > If the memory model is that history().provisionalItem() owns the cached page, perhaps PageCache::get should return PassRefPtr<HistoryItem>, and we just shouldn't put cachedPage into a local variable. This is not my intended end state, I'm iterating towards having the function rip out an object from the page cache, taking ownership, and eventually grounding it into the new frame.
Geoffrey Garen
Comment 6 2013-09-04 23:30:53 PDT
OK, I will stay tuned for future episodes of the akling show. :)
Andreas Kling
Comment 7 2013-09-05 10:18:12 PDT
Reopening since I reverted this in r155120 for breaking Qt stuffs.
Andreas Kling
Comment 8 2013-09-05 10:52:00 PDT
Created attachment 210637 [details] Patch v2 Go all the way and add PageCache::take() which lets FrameLoader take ownership of the CachedPage during commitProvisionalLoad().
WebKit Commit Bot
Comment 9 2013-09-05 14:25:46 PDT
Comment on attachment 210637 [details] Patch v2 Clearing flags on attachment: 210637 Committed r155150: <http://trac.webkit.org/changeset/155150>
WebKit Commit Bot
Comment 10 2013-09-05 14:25:48 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.