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.
Created attachment 142588 [details] RefCounted<CachedResourceLoader>
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.
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.
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.
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
(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.
Created attachment 144139 [details] Reposting previous patch to see if EWS failures were flakes
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
Created attachment 144149 [details] Add missing symbols
Created attachment 144899 [details] Fix gtk
Created attachment 167410 [details] Merge to trunk
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?
Comment on attachment 167410 [details] Merge to trunk This patch appears correct to me.
Comment on attachment 167410 [details] Merge to trunk Clearing flags on attachment: 167410 Committed r130817: <http://trac.webkit.org/changeset/130817>
All reviewed patches have been landed. Closing bug.