Bug 111022 – Cache partitioning does not affect iframe MainResources

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Vicki Pfau]

With the recently added cache partitioning patch, subresources are properly getting separated into the relevant partitions, but iframe MainResources are not currently being partitioned.

[Vicki Pfau]

Created attachment 190626 [details]
Patch

[Adam Barth]

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.

[Vicki Pfau]

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

[Adam Barth]

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.

[Alexey Proskuryakov]

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?

[Alexey Proskuryakov]

produce *something* more positive

[Alexey Proskuryakov]

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.

[Vicki Pfau]

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.

[David Kilzer (:ddkilzer)]

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());
    }

[David Kilzer (:ddkilzer)]

(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).

[Darin Adler]

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

[Vicki Pfau]

(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()).

[Vicki Pfau]

<rdar://problem/13342678>

[Vicki Pfau]

Committed r144694: <http://trac.webkit.org/changeset/144694>