Bug 111022 - Cache partitioning does not affect iframe MainResources
Summary: Cache partitioning does not affect iframe MainResources
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Vicki Pfau
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-02-27 16:01 PST by Vicki Pfau
Modified: 2013-03-04 16:25 PST (History)
5 users (show)

See Also:


Attachments
Patch (4.76 KB, patch)
2013-02-27 17:00 PST, Vicki Pfau
ddkilzer: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vicki Pfau 2013-02-27 16:01:59 PST
With the recently added cache partitioning patch, subresources are properly getting separated into the relevant partitions, but iframe MainResources are not currently being partitioned.
Comment 1 Vicki Pfau 2013-02-27 17:00:27 PST
Created attachment 190626 [details]
Patch
Comment 2 Adam Barth 2013-02-28 00:55:40 PST
Comment on attachment 190626 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190626&action=review

> Source/WebCore/loader/FrameLoader.cpp:1199
> +    if (m_frame->tree()->top() != m_frame && m_frame->tree()->top()->document())

How can m_frame->tree()->top()->document() be zero?

> Source/WebCore/loader/FrameLoader.cpp:1200
> +        request.setCachePartition(m_frame->tree()->top()->document()->securityOrigin()->cachePartition());

m_frame->tree()->top()->document()   <-- I'm not sure that's idiomatically the best way to find the topDocument.  Maybe it doesn't matter now that we don't have that disconnected frame stuff.
Comment 3 Vicki Pfau 2013-02-28 01:53:54 PST
(In reply to comment #2)
> (From update of attachment 190626 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190626&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:1199
> > +    if (m_frame->tree()->top() != m_frame && m_frame->tree()->top()->document())
> 
> How can m_frame->tree()->top()->document() be zero?

I've hit cases where a frame's document was 0, but I wasn't sure when exactly those were. Maybe it's only for subframes? That would render that clause useless. I just wanted to make sure I didn't accidentally run into null pointer derefs.

> > Source/WebCore/loader/FrameLoader.cpp:1200
> > +        request.setCachePartition(m_frame->tree()->top()->document()->securityOrigin()->cachePartition());
> 
> m_frame->tree()->top()->document()   <-- I'm not sure that's idiomatically the best way to find the topDocument.  Maybe it doesn't matter now that we don't have that disconnected frame stuff.

I was under the impression that Document::topDocument() did that, but it doesn't seem to. Did that change at some point? I'm all for a better idiom, but I didn't know of one.
Comment 4 Adam Barth 2013-02-28 01:56:48 PST
Frame::document() is only ever zero while we're initializing the frame.

The concept of a disconnected frame is gone from the codebase, which is probably why you don't see it in topDocument.
Comment 5 Alexey Proskuryakov 2013-03-02 10:23:30 PST
Comment on attachment 190626 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190626&action=review

> LayoutTests/http/tests/cache/partitioned-cache-iframe-expected.txt:1
> +CONSOLE MESSAGE: line 18: FAIL

Could you please make this test display produce more positive when it passes?
Comment 6 Alexey Proskuryakov 2013-03-02 10:23:45 PST
produce *something* more positive
Comment 7 Alexey Proskuryakov 2013-03-02 10:25:19 PST
Comment on attachment 190626 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190626&action=review

>> LayoutTests/http/tests/cache/partitioned-cache-iframe-expected.txt:1
>> +CONSOLE MESSAGE: line 18: FAIL
> 
> Could you please make this test display produce more positive when it passes?

Or rather, it's not clear to me why the test is expected to fail now - ChangeLog does not explain this.
Comment 8 Vicki Pfau 2013-03-02 21:55:17 PST
Perhaps I should change it to say "The cache is not partitioned" instead of "FAIL", as that's what's going on here. This test does not pass, per se, in any shipping ports at the moment, hence why it says "FAIL" right now.
Comment 9 David Kilzer (:ddkilzer) 2013-03-04 15:31:07 PST
Comment on attachment 190626 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190626&action=review

r=me

>>> Source/WebCore/loader/FrameLoader.cpp:1199
>>> +    if (m_frame->tree()->top() != m_frame && m_frame->tree()->top()->document())
>> 
>> How can m_frame->tree()->top()->document() be zero?
> 
> I've hit cases where a frame's document was 0, but I wasn't sure when exactly those were. Maybe it's only for subframes? That would render that clause useless. I just wanted to make sure I didn't accidentally run into null pointer derefs.

This code would read clearer if "m_frame->tree()->top() != m_frame" were extracted into a method called isSubFrame():

bool Frame::isSubFrame()
{
    return tree()->top() != self;
}

Not necessary for this patch, but please consider a follow-up patch.  (Or implement the opposite:  Frame::isTopFrame().)

--

Also, it would be nice not to call "m_frame->tree()->top()->document()" twice, so maybe you could do this:

    if (m_frame->isSubFrame()) {
        if (Document *topDocument = m_frame->tree()->top()->document())
            request.setCachePartition(topDocument->securityOrigin()->cachePartition());
    }
Comment 10 David Kilzer (:ddkilzer) 2013-03-04 15:31:46 PST
(In reply to comment #9)
> (From update of attachment 190626 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190626&action=review
> 
> r=me

Plus the changes that Alexey already mentioned (that you said you had already made locally).
Comment 11 Darin Adler 2013-03-04 15:32:50 PST
Comment on attachment 190626 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190626&action=review

>>>> Source/WebCore/loader/FrameLoader.cpp:1199
>>>> +    if (m_frame->tree()->top() != m_frame && m_frame->tree()->top()->document())
>>> 
>>> How can m_frame->tree()->top()->document() be zero?
>> 
>> I've hit cases where a frame's document was 0, but I wasn't sure when exactly those were. Maybe it's only for subframes? That would render that clause useless. I just wanted to make sure I didn't accidentally run into null pointer derefs.
> 
> This code would read clearer if "m_frame->tree()->top() != m_frame" were extracted into a method called isSubFrame():
> 
> bool Frame::isSubFrame()
> {
>     return tree()->top() != self;
> }
> 
> Not necessary for this patch, but please consider a follow-up patch.  (Or implement the opposite:  Frame::isTopFrame().)
> 
> --
> 
> Also, it would be nice not to call "m_frame->tree()->top()->document()" twice, so maybe you could do this:
> 
>     if (m_frame->isSubFrame()) {
>         if (Document *topDocument = m_frame->tree()->top()->document())
>             request.setCachePartition(topDocument->securityOrigin()->cachePartition());
>     }

If we added functions like that they should be in FrameTree, not on Frame itself.

    frame->tree()->isTop()

and/or

    frame->tree()->isSubframe()
Comment 12 Vicki Pfau 2013-03-04 15:39:13 PST
(In reply to comment #11)
> (From update of attachment 190626 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190626&action=review
> 
> >>>> Source/WebCore/loader/FrameLoader.cpp:1199
> >>>> +    if (m_frame->tree()->top() != m_frame && m_frame->tree()->top()->document())
> >>> 
> >>> How can m_frame->tree()->top()->document() be zero?
> >> 
> >> I've hit cases where a frame's document was 0, but I wasn't sure when exactly those were. Maybe it's only for subframes? That would render that clause useless. I just wanted to make sure I didn't accidentally run into null pointer derefs.
> > 
> > This code would read clearer if "m_frame->tree()->top() != m_frame" were extracted into a method called isSubFrame():
> > 
> > bool Frame::isSubFrame()
> > {
> >     return tree()->top() != self;
> > }
> > 
> > Not necessary for this patch, but please consider a follow-up patch.  (Or implement the opposite:  Frame::isTopFrame().)
> > 
> > --
> > 
> > Also, it would be nice not to call "m_frame->tree()->top()->document()" twice, so maybe you could do this:
> > 
> >     if (m_frame->isSubFrame()) {
> >         if (Document *topDocument = m_frame->tree()->top()->document())
> >             request.setCachePartition(topDocument->securityOrigin()->cachePartition());
> >     }
> 
> If we added functions like that they should be in FrameTree, not on Frame itself.
> 
>     frame->tree()->isTop()
> 
> and/or
> 
>     frame->tree()->isSubframe()

This idiom appears to also be used in MainResourceLoader::willSendRequest, so it should likely be done in a follow up patch (although it holds onto frame->tree()->top() and then uses it later in the function in much the same way I could be doing in this patch--it only saves one call to top()).
Comment 13 Vicki Pfau 2013-03-04 16:19:04 PST
<rdar://problem/13342678>
Comment 14 Vicki Pfau 2013-03-04 16:25:25 PST
Committed r144694: <http://trac.webkit.org/changeset/144694>