Summary: | Update CachedResourceLoader lifetime to allow for main resources | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nate Chapin <japhet> | ||||||||||||||
Component: | Page Loading | Assignee: | 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
Nate Chapin
2012-05-17 16:41:30 PDT
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. |