Bug 82991 – Implement <iframe srcdoc>

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Implement <iframe srcdoc>

[Adam Barth]

Created attachment 135251 [details]
Patch

[Adam Barth]

Dearest reviewers, you can find the step-by-step creation of this patch at:

https://github.com/abarth/webkit/compare/master...srcdoc

[Eric Seidel (no email)]

Comment on attachment 135251 [details]
Patch

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

> Source/WebCore/ChangeLog:26
> +        a mechanim previously used for error messages and the like. Using

mechanim

> Source/WebCore/dom/Document.cpp:4835
> +    if (m_frame->loader()->shouldTreatURLAsSrcdocDocument(url())) {

Odd that the loader knows this.  I guess srcdoc docs are defined by having about:srcdoc, and the loader is the one who's supposed to know these mappings?

> Source/WebCore/dom/Document.cpp:4844
> +        // If we're supposed to inherit our security origin from our owner,
> +        // but we're also sandboxed, the only thing we inherit is the ability
> +        // to load local resources. This lets about:blank iframes in file://
> +        // URL documents load images and other resources from the file system.

I'm slightly confused where the "when we're not sandboxed" case happens?

> Source/WebCore/html/HTMLFrameElementBase.cpp:108
> +    if (attr->name() == srcdocAttr)
> +        setLocation("about:srcdoc");

So these two are equivalent?

> Source/WebCore/html/HTMLFrameElementBase.cpp:110
> +    else if (attr->name() == srcAttr && !fastHasAttribute(srcdocAttr))
>          setLocation(stripLeadingAndTrailingHTMLSpaces(attr->value()));

Intresting that we don't even parse the src when srcdoc is set.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2532
> +    if (!m_fragmentContext.fragment() && !m_document->isSrcdocDocument())

srcdoc can never be quirks?

> Source/WebCore/loader/FrameLoader.cpp:2567
> -    String referrer = SecurityPolicy::generateReferrerHeader(m_frame->document()->referrerPolicy(), request.url(), m_outgoingReferrer);
> +    String referrer = SecurityPolicy::generateReferrerHeader(m_frame->document()->referrerPolicy(), request.url(), outgoingReferrer());

You might do the outgoingReferrer() replacement first.  rs=me on such a patch were you to do so.

> Source/WebCore/loader/FrameLoader.cpp:2957
> +    return ownerElement->fastHasAttribute(srcdocAttr);

Interesting.  Setting src="about:srcdoc" is not enough! :)

> Source/WebCore/loader/FrameLoader.h:313
> +    SubstituteData defaultSubstituteDataForURL(const KURL&);

I assume SubstituteData is some sort of "smart pointer" of sorts?  I guess it holds a ref to the SharedBuffer.  Otherwise seems like this could be expensive to return it by value this way.

[Adam Barth]

(In reply to comment #3)
> (From update of attachment 135251 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135251&action=review
> 
> > Source/WebCore/dom/Document.cpp:4835
> > +    if (m_frame->loader()->shouldTreatURLAsSrcdocDocument(url())) {
> 
> Odd that the loader knows this.  I guess srcdoc docs are defined by having about:srcdoc, and the loader is the one who's supposed to know these mappings?

Originally I named this function shouldLoadURLasSrcdocDocument, but then I changed the name to the one you see above to be consistent with shouldTreatURLAsSameAsCurrent.  The idea is that loading about:srcdoc does different things depending on the context, in particular depending on the ownerElement's srcdoc attribute.

> > Source/WebCore/dom/Document.cpp:4844
> > +        // If we're supposed to inherit our security origin from our owner,
> > +        // but we're also sandboxed, the only thing we inherit is the ability
> > +        // to load local resources. This lets about:blank iframes in file://
> > +        // URL documents load images and other resources from the file system.
> 
> I'm slightly confused where the "when we're not sandboxed" case happens?

That happens when you have the following markup in your document:

<iframe src="about:blank">

In that case, we inherit a bunch of things from the owner (see a few lines further down), including the security origin.

> > Source/WebCore/html/HTMLFrameElementBase.cpp:108
> > +    if (attr->name() == srcdocAttr)
> > +        setLocation("about:srcdoc");
> 
> So these two are equivalent?

It's more like setting the srcdocAttr triggers a load for about:srcdoc, just like setting the srcAttr triggers a load for whatever URL is stored in the attribute.

> > Source/WebCore/html/HTMLFrameElementBase.cpp:110
> > +    else if (attr->name() == srcAttr && !fastHasAttribute(srcdocAttr))
> >          setLocation(stripLeadingAndTrailingHTMLSpaces(attr->value()));
> 
> Intresting that we don't even parse the src when srcdoc is set.

We do parse it, we just don't trigger a load.  It's still there in the DOM.  I can write a test that verifies that, if you like.

> > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2532
> > +    if (!m_fragmentContext.fragment() && !m_document->isSrcdocDocument())
> 
> srcdoc can never be quirks?

They can, if you supply a quirky doctype.  See fast/frames/srcdoc/srcdoc-can-be-in-qurks-mode.html

This line just makes the default be standards mode, e.g., if you don't supply a doctype at all.

> > Source/WebCore/loader/FrameLoader.cpp:2567
> > -    String referrer = SecurityPolicy::generateReferrerHeader(m_frame->document()->referrerPolicy(), request.url(), m_outgoingReferrer);
> > +    String referrer = SecurityPolicy::generateReferrerHeader(m_frame->document()->referrerPolicy(), request.url(), outgoingReferrer());
> 
> You might do the outgoingReferrer() replacement first.  rs=me on such a patch were you to do so.

Ok.

> > Source/WebCore/loader/FrameLoader.cpp:2957
> > +    return ownerElement->fastHasAttribute(srcdocAttr);
> 
> Interesting.  Setting src="about:srcdoc" is not enough! :)

Correct.

> > Source/WebCore/loader/FrameLoader.h:313
> > +    SubstituteData defaultSubstituteDataForURL(const KURL&);
> 
> I assume SubstituteData is some sort of "smart pointer" of sorts?  I guess it holds a ref to the SharedBuffer.  Otherwise seems like this could be expensive to return it by value this way.

Yeah, it just holds a RefPtr to the data:
http://trac.webkit.org/browser/trunk/Source/WebCore/loader/SubstituteData.h

I can change this to use another pattern if you're worried about churning the refcount, but it's not like we're memcpying the data each time.

[Adam Barth]

Created attachment 135253 [details]
patch (fixed typo in ChangeLog)

[Eric Seidel (no email)]

Comment on attachment 135253 [details]
patch (fixed typo in ChangeLog)

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

> Source/WebCore/dom/Document.cpp:4847
> +    if (isSandboxed(SandboxOrigin)) {
> +        // If we're supposed to inherit our security origin from our owner,
> +        // but we're also sandboxed, the only thing we inherit is the ability
> +        // to load local resources. This lets about:blank iframes in file://
> +        // URL documents load images and other resources from the file system.
> +        if (ownerFrame->document()->securityOrigin()->canLoadLocalResources())
> +            securityOrigin()->grantLoadLocalResources();
> +        return;

Should this be in initSecurityContext?

[Adam Barth]

(In reply to comment #6)
> (From update of attachment 135253 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135253&action=review
> 
> > Source/WebCore/dom/Document.cpp:4847
> > +    if (isSandboxed(SandboxOrigin)) {
> > +        // If we're supposed to inherit our security origin from our owner,
> > +        // but we're also sandboxed, the only thing we inherit is the ability
> > +        // to load local resources. This lets about:blank iframes in file://
> > +        // URL documents load images and other resources from the file system.
> > +        if (ownerFrame->document()->securityOrigin()->canLoadLocalResources())
> > +            securityOrigin()->grantLoadLocalResources();
> > +        return;
> 
> Should this be in initSecurityContext?

It's currently in initSecurityContext.  Do you think it should be elsewhere?

[WebKit Review Bot]

Comment on attachment 135253 [details]
patch (fixed typo in ChangeLog)

Clearing flags on attachment: 135253

Committed r112987: <http://trac.webkit.org/changeset/112987>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Philippe Normand]

(In reply to comment #9)
> All reviewed patches have been landed.  Closing bug.

Hits ASSERT on GTK and Mac Debug builds. About 17 failures :(
http://build.webkit.org/results/Lion%20Debug%20(WebKit2%20Tests)/r112992%20(5693)/results.html

Crash log example: http://build.webkit.org/results/Lion%20Debug%20(WebKit2%20Tests)/r112992%20(5693)/fast/frames/sandboxed-iframe-navigation-top-denied-crash-log.txt

[Eric Seidel (no email)]

I believe Adam is asleep.  Probably best to roll out for now.

[Philippe Normand]

(In reply to comment #11)
> I believe Adam is asleep.  Probably best to roll out for now.

Ok, rolled out in http://trac.webkit.org/changeset/113002

[Adam Barth]

Thanks.  I'll take a look at the asserts.

[Adam Barth]

*** Bug 43683 has been marked as a duplicate of this bug. ***

[Adam Barth]

Created attachment 135439 [details]
Patch (now with updated ASSERT)

[Adam Barth]

Comment on attachment 135439 [details]
Patch (now with updated ASSERT)

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

> Source/WebCore/page/SecurityOrigin.cpp:377
> +    ASSERT(isUnique() || SecurityPolicy::allowSubstituteDataAccessToLocal());

This is the only difference from the previous patch.  This ASSERT is here to protect us from granting privileges to some, but not all, documents in a given origin.  However, in this case, we're granting the privilege to a unique origin, so that's not a problem.  I've added isUnique() to the ASSERT to allow this case.

[Adam Barth]

Committed r113143: <http://trac.webkit.org/changeset/113143>