WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
86787
Update CachedResourceLoader lifetime to allow for main resources
https://bugs.webkit.org/show_bug.cgi?id=86787
Summary
Update CachedResourceLoader lifetime to allow for main resources
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
Details
Formatted Diff
Diff
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-
Details
Formatted Diff
Diff
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-
Details
Formatted Diff
Diff
Add missing symbols
(20.30 KB, patch)
2012-05-25 14:38 PDT
,
Nate Chapin
no flags
Details
Formatted Diff
Diff
Fix gtk
(20.30 KB, patch)
2012-05-30 12:51 PDT
,
Nate Chapin
no flags
Details
Formatted Diff
Diff
Merge to trunk
(16.76 KB, patch)
2012-10-05 16:25 PDT
,
Nate Chapin
no flags
Details
Formatted Diff
Diff
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.
Top of Page
Format For Printing
XML
Clone This Bug