[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


Alexey Proskuryakov <ap at webkit.org> changed:

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




--- Comment #51 from Alexey Proskuryakov <ap at webkit.org>  2010-06-24 11:26:53 PST ---
(From update of attachment 59361)
+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?

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