Bug 82991

Created attachment 135251 [details] Patch

Dearest reviewers, you can find the step-by-step creation of this patch at: https://github.com/abarth/webkit/compare/master...srcdoc

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.

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

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

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?

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

Comment on attachment 135253 [details] patch (fixed typo in ChangeLog) Clearing flags on attachment: 135253 Committed r112987: <http://trac.webkit.org/changeset/112987>

All reviewed patches have been landed. Closing bug.

(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

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

(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

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

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

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

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.

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