Bug 86787 – Update CachedResourceLoader lifetime to allow for main resources

• Component: Page Loading
• Status: RESOLVED FIXED

[Nate Chapin]

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.

[Nate Chapin]

Created attachment 142588 [details]
RefCounted<CachedResourceLoader>

[Adam Barth]

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]

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]

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 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]

(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]

Created attachment 144139 [details]
Reposting previous patch to see if EWS failures were flakes

[Build Bot]

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]

Created attachment 144149 [details]
Add missing symbols

[Nate Chapin]

Created attachment 144899 [details]
Fix gtk

[Nate Chapin]

Created attachment 167410 [details]
Merge to trunk

[Adam Barth]

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 on attachment 167410 [details]
Merge to trunk

This patch appears correct to me.

[WebKit Review Bot]

Comment on attachment 167410 [details]
Merge to trunk

Clearing flags on attachment: 167410

Committed r130817: <http://trac.webkit.org/changeset/130817>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.