Bug 86787

Summary: Update CachedResourceLoader lifetime to allow for main resources
Product: WebKit Reporter: Nate Chapin <japhet>
Component: Page LoadingAssignee: Nate Chapin <japhet>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, gustavo, koivisto, philn, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 49246    
Attachments:
Description Flags
RefCounted<CachedResourceLoader>
none
Address abarth's comments, change and explain end of life code in Document.cpp
buildbot: commit-queue-
Reposting previous patch to see if EWS failures were flakes
buildbot: commit-queue-
Add missing symbols
none
Fix gtk
none
Merge to trunk none

Nate Chapin
Reported 2012-05-17 16:41:30 PDT
Currently, CachedResourceLoader is an OwnPtr of the Document. This doesn't work for main resources, since the Document will be created after the resource's data starts coming in. Possible ways to resolve this: 1. Refactor revalidation logic out of CachedResourceLoader and have MainResourceLoader talk to MemoryCache more directly. 2. Have the CachedResourceLoader OwnPtr'ed on DocumentLoader until the Document is created, then transfer the OwnPtr to the Document. 3. Make CachedResourceLoader RefCounted and have both DocumentLoader and Document hold RefPtrs to it. 4. Have DocumentLoader OwnPtr the CachedResourceLoader and have Document::cachedResourceLoader() return its loader()->cachedResourceLoader. Of those, (3) seems the most sane to me. (2) requires special handling in several cases (e.g., transferring between Documents during the creation of a SinkDocument), (4) requires adding a ton of null checks and seems unreliable to me, and (1) seems like it'll lead to a lot of duplicate code no matter what. Will post a patch for (3) soon, but I'm open other ideas if others feel differently.
Attachments
RefCounted<CachedResourceLoader> (14.88 KB, patch)
2012-05-17 16:50 PDT, Nate Chapin 		no flags
DetailsFormatted DiffDiff
Address abarth's comments, change and explain end of life code in Document.cpp (17.78 KB, patch)
2012-05-18 15:41 PDT, Nate Chapin 		buildbot: commit-queue-
DetailsFormatted DiffDiff
Reposting previous patch to see if EWS failures were flakes (17.78 KB, patch)
2012-05-25 13:48 PDT, Nate Chapin 		buildbot: commit-queue-
DetailsFormatted DiffDiff
Add missing symbols (20.30 KB, patch)
2012-05-25 14:38 PDT, Nate Chapin 		no flags
DetailsFormatted DiffDiff
Fix gtk (20.30 KB, patch)
2012-05-30 12:51 PDT, Nate Chapin 		no flags
DetailsFormatted DiffDiff
Merge to trunk (16.76 KB, patch)
2012-10-05 16:25 PDT, Nate Chapin 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Nate Chapin
Comment 1 2012-05-17 16:50:47 PDT
Created attachment 142588 [details] RefCounted<CachedResourceLoader>
Adam Barth
Comment 2 2012-05-17 18:02:38 PDT
Comment on attachment 142588 [details] RefCounted<CachedResourceLoader> View in context: https://bugs.webkit.org/attachment.cgi?id=142588&action=review > Source/WebCore/loader/cache/CachedResourceLoader.cpp:298 > - if (!document()->securityOrigin()->canDisplay(url)) { > + if (document() && !document()->securityOrigin()->canDisplay(url)) { Does this mean we can request any URL if we don't have a document yet? Should we add some asserts about |type| to make sure that loophole is only used for main resources? > Source/WebCore/loader/cache/CachedResourceLoader.h:106 > Document* document() const { return m_document; } Should we add a // Can be 0 comment here too? > Source/WebCore/loader/cache/CachedResourceLoader.h:125 > + CachedResourceLoader(DocumentLoader*); Please add the explicit keyword to one-argument constructors.
Adam Barth
Comment 3 2012-05-17 18:04:15 PDT
I'm going to leave this up here for a bit to give other folks a chance to comment. I wonder if it's worth writing a comment near some of these functions explaining the ownership model for the CachedResourceLoader (and why). I know we tend to avoid those sorts of comments in WebKit, but this case might be worth explaining since it's somewhat non-obvious.
Nate Chapin
Comment 4 2012-05-18 15:41:40 PDT
Created attachment 142800 [details] Address abarth's comments, change and explain end of life code in Document.cpp I wasn't clearing CachedResourceLoader::m_document at the end of the Document's life, which could theoretically lead to a stale pointer (I wasn't able to convince myself that the Document will always outlive the DocumentLoader, correct me if there's something I don't know). Turns out severing the Document<->CachedResourceLoader connection is more complicated than I thought, because there are actually cases where multiple Documents will end up with the same CachedResourceLoader. I think I've made this code sufficiently defensive (and commented to death), but I may be missing something.
Build Bot
Comment 5 2012-05-19 04:57:30 PDT
Comment on attachment 142800 [details] Address abarth's comments, change and explain end of life code in Document.cpp Attachment 142800 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12735109
Nate Chapin
Comment 6 2012-05-21 09:17:24 PDT
(In reply to comment #5) > (From update of attachment 142800 [details]) > Attachment 142800 [details] did not pass win-ews (win): > Output: http://queues.webkit.org/results/12735109 It looks like the red bots are because of redness on trunk to me.
Nate Chapin
Comment 7 2012-05-25 13:48:36 PDT
Created attachment 144139 [details] Reposting previous patch to see if EWS failures were flakes
Build Bot
Comment 8 2012-05-25 14:10:20 PDT
Comment on attachment 144139 [details] Reposting previous patch to see if EWS failures were flakes Attachment 144139 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12803599
Nate Chapin
Comment 9 2012-05-25 14:38:27 PDT
Created attachment 144149 [details] Add missing symbols
Nate Chapin
Comment 10 2012-05-30 12:51:44 PDT
Created attachment 144899 [details] Fix gtk
Nate Chapin
Comment 11 2012-10-05 16:25:30 PDT
Created attachment 167410 [details] Merge to trunk
Adam Barth
Comment 12 2012-10-09 14:34:06 PDT
Comment on attachment 167410 [details] Merge to trunk View in context: https://bugs.webkit.org/attachment.cgi?id=167410&action=review > Source/WebCore/dom/Document.cpp:535 > + if (!m_cachedResourceLoader) > + m_cachedResourceLoader = CachedResourceLoader::create(0); Woah, we do this even when we don't have a frame. I guess that's what we've always done. Interesting. > Source/WebCore/dom/Document.cpp:663 > + if (m_cachedResourceLoader->document() == this) > + m_cachedResourceLoader->setDocument(0); Do you want to make this a single operation on CachedResourceLoader?
Adam Barth
Comment 13 2012-10-09 14:34:55 PDT
Comment on attachment 167410 [details] Merge to trunk This patch appears correct to me.
WebKit Review Bot
Comment 14 2012-10-09 14:58:22 PDT
Comment on attachment 167410 [details] Merge to trunk Clearing flags on attachment: 167410 Committed r130817: <http://trac.webkit.org/changeset/130817>
WebKit Review Bot
Comment 15 2012-10-09 14:58:27 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.