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

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap 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 59361: New proposed patch
https://bugs.webkit.org/attachment.cgi?id=59361&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+void Document::setOriginDocument(PassRefPtr<Document> originDocument)
+{
+    // This function stores a reference to the document where this document
+    // originated from. This is used for frameless documents, so that we can

I really dislike it that both Document and DOMImplementation get an
m_originDocument reference. It doesn't seem that these should ever be
different, but it raises all sorts of unpleasant questions about when exactly
they are different in practice.

+    // find a frame when we are loading subresources. Also, because frameless
+    // documents don't get a SecurityOrigin and a cookieURL when constructed,
+    // we need to set these here to be able to load subresources.

Please add assertions that there is no SecurityOrigin and cookieURL indeed.

	     m_responseXML->setURL(m_url);
+	     m_responseXML->setOriginDocument(document());

This looks weird to me. The result of this code is that a script can decide
which security origin to use with the result of XMLHttpRequest -
req.responseXML will use one, DOMParser.parseFromString(req.responseText,
"application/xml") will use another. This sort of freedom looks highly
suspicious.

I'm not sure what exactly I'm asking for here - perhaps someone CC'ed on this
bug (Adam?) could weigh in.

     if (!parentSheet->finalURL().isNull())
-	 // use parent styleheet's URL as the base URL
-	 absHref = KURL(parentSheet->finalURL(), m_strHref).string();
-    
+	 // Use parent styleheet's URL as the base URL.
+	 absHref = KURL(parentSheet->finalURL(), m_strHref);
+    else if (originDocument)
+	 // When parent stylesheet doesn't have a final URL, use base URL from
the
+	 // document the root stylesheet originated from. This can happen when
the
+	 // stylesheet is created with DOMParser.parseFromString(), for
example.
+	 absHref = originDocument->completeURL(m_strHref);

Per WebKit coding style, these blocks should be in braces (comments do count).

+    else
+	 absHref = KURL(ParsedURLString, m_strHref);

This doesn't seem right - what guarantees that m_strHref is indeed parsed? When
do we take this code path?

It seems that this should be ASSERT_NOT_REACHED plus an early return instead.

I'm going to say r- for the above comments, but there is also something I don't
understand in how subresources are loaded in general. There is that old ugly
code in XSLStyleSheet::loadChildSheets() that iterates over stylesheets to find
imports and includes - it it obsoleted by your code? How do they interact?

I'll try to figure this out myself later, but an explanation from you would be
appreciated, too.

+    void finishLoadingSheetAsynchronously(DocLoader*, const KURL&);
+    void finishLoadingSheetSynchronously(DocLoader*, const KURL&);

A synchronous load can be started while an asynchronous one is in progress. But
we're using global variables for loading, see globalProcessor and
globalDocLoader. Is there a race condition here?