[webkit-reviews] review denied: [Bug 10313] xsl:import and document() don't work in stylesheets loaded via XMLHttpRequest : [Attachment 84709] New proposed patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Andreas Wictor
<andreas.wictor at xcerion.com>'s request for review:
Bug 10313: xsl:import and document() don't work in stylesheets loaded via
XMLHttpRequest
https://bugs.webkit.org/show_bug.cgi?id=10313

Attachment 84709: New proposed patch
https://bugs.webkit.org/attachment.cgi?id=84709&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=84709&action=review

Ok.  So, I got half-way though the review, and my conclusion is that this is
too much complexity for such a corner use case.  I would like to support this
use case, but the virtuous way to do that is to disentangle the loader from the
Frame.	That's not going to be easy, but this patch will end up looking like
the wrong direction if/when we make that happen.  I'm sympathetic to the fact
that you've spent a lot of time and effort working on this patch, but this is
complex stuff that's basically going to be untested because so few people will
use this feature.  That means its going to have bugs, and likely bad security
bugs at that.

> Source/WebCore/bindings/v8/custom/V8XSLTProcessorCustom.cpp:93
> +    ScriptExecutionContext* scriptContext = getScriptExecutionContext();

getScriptExecutionContext => scriptExecutionContext

> Source/WebCore/dom/Document.cpp:4100
> -    processor->createDocumentFromSource(newSource, resultEncoding,
resultMIMEType, this, frame());
> +    processor->createDocumentFromSource(newSource, resultEncoding,
resultMIMEType, this, frame(), 0);

There's no origin document in this case?

> Source/WebCore/dom/Document.h:521
> +    void setOriginDocument(PassRefPtr<Document>);

This should probably have a scary warning about how calling it could be
disaster.  There's a similar warning for setSecurityOrigin and this on is even
more powerful!

> Source/WebCore/dom/Document.h:1180
> +    RefPtr<Document> m_originDocument;

Does this create a reference cycle?

> Source/WebCore/loader/DocumentThreadableLoader.cpp:361
> +	   String referrer = m_document->url().strippedForUseAsReferrer();

Is this the correct referrer?  It seems like we do something more complicated
when we compute frame->loader()->outgoingReferrer().

> Source/WebCore/loader/DocumentThreadableLoader.cpp:363
> +	   identifier = frame->loader()->loadResourceSynchronously(
> +	       request, storedCredentials, referrer, error, response, data);

No need to break this line.

> Source/WebCore/loader/FrameLoader.cpp:2763
> +unsigned long FrameLoader::loadResourceSynchronously(const ResourceRequest&
request, StoredCredentials storedCredentials, const String& outgoingReferrer,
ResourceError& error, ResourceResponse& response, Vector<char>& data)

Boo to adding another one of these methods.  We have too many of these
FrameLoader methods with infinite arguments.

> Source/WebCore/xml/DOMParser.cpp:37
> +	  
doc->setOriginDocument(static_cast<Document*>(scriptExecutionContext));

Why can't we pass the origin during document construction?  That seems like a
safer time than after-the-fact.

> Source/WebCore/xml/DOMParser.idl:22
> -	   Document parseFromString(in DOMString str, in DOMString
contentType);
> +	   [CallWith=ScriptExecutionContext] Document parseFromString(in
DOMString str, in DOMString contentType);

So this means the originDocument will be based on the lexical scope in which
this function was called.  I guess that makes sense.  It's slightly odd how
that interacts with third-party cookie blocking, especially if the constructed
document is handed off from one page to another.

> Source/WebCore/xml/XMLHttpRequest.cpp:253
> -	       m_responseXML->setSecurityOrigin(document()->securityOrigin());
> +	       m_responseXML->setOriginDocument(document());

This part of the change looks elegant.

> Source/WebCore/xml/XSLImportRule.cpp:100
> +    RefPtr<Document> parentDocument = parentSheet->ownerDocument();

So there's an originDocument and a parentDocument, but they're not the same. 
Interesting.

> Source/WebCore/xml/XSLImportRule.cpp:112
> +	       parentURL = originDocument->url();

Wait, why do we care about the originDocument's URL?  That doesn't make any
sense to me.  The origin document is just a bread-crumb trail back to the
loader machinery.

> Source/WebCore/xml/XSLImportRule.cpp:116
> +	   // Create a fake document used for the duration of this load.
> +	   parentDocument = Document::create(0, parentURL);

Woah, yet another document.  This is starting to look pretty hacky.

> Source/WebCore/xml/XSLImportRule.cpp:119
> +	   if (originDocument)
> +	       parentDocument->setOriginDocument(originDocument);

Ok, you've lost me.  Why are we calling setOrigin document on this fake
document?  That seems like an extra level of indirection.