CachedPage and CachedFrame are single-owner objects.
Created attachment 210530 [details] Patch
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!
Committed r155086: <http://trac.webkit.org/changeset/155086>
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.
(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.
OK, I will stay tuned for future episodes of the akling show. :)
Reopening since I reverted this in r155120 for breaking Qt stuffs.
Created attachment 210637 [details] Patch v2 Go all the way and add PageCache::take() which lets FrameLoader take ownership of the CachedPage during commitProvisionalLoad().
Comment on attachment 210637 [details] Patch v2 Clearing flags on attachment: 210637 Committed r155150: <http://trac.webkit.org/changeset/155150>
All reviewed patches have been landed. Closing bug.