[Webkit-unassigned] [Bug 10313] xsl:import and document() don't work in stylesheets loaded via XMLHttpRequest

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=10313


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #84709|review?                     |review-
               Flag|                            |




--- Comment #81 from Adam Barth <abarth at webkit.org>  2011-05-05 01:31:05 PST ---
(From update of attachment 84709)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.