203192 - Improvements to XSLT loading

Status: VERIFIED FIXED

(Core :: XSLT, defect, P1)

Description

[Peter Van der Beken [:peterv]]

I'll be attaching a patch that resolves a number of bugs in XSLT loading, both
loading of stylesheets and of additional source documents. The most important
change is adding back the same-origin checks for stylesheet loads.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: Patch for mozilla/content

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: Patch to move buster Makefile.in, jar.mn etc into buster dir

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: Patch for mozilla/extensions/transformiix

Comment 4

[Axel Hecht]

Comment on attachment 121534 [details] [diff] [review]
Patch to move buster Makefile.in, jar.mn etc into buster dir

transformiix/resources/buster/Makefile.in can loose the 
MOZ_CHROME_FILE_FORMAT=flat
with that, r=[e-mail]

Comment 5

[Boris Zbarsky [:bzbarsky]]

Btw, does XSLT do content-policy checks for the sheet loads?

Comment 6

[Axel Hecht]

Comment on attachment 121535 [details] [diff] [review]
Patch for mozilla/extensions/transformiix

not sure about the empty lines before return rv;s

+    if (contentType.IsEmpty()) {
+	 // Time to sniff!

IMHO, we should just sniff for file:// and chrome://. That'd be consistent. I
vote for sniffing as soon as the content type is not a xml content type.
(Remember, VERSION ended up as text/plain.)
At least this should trigger for the unkown mime type, too.

+    else if (NS_FAILED(aStatusCode) &&
+	      aStatusCode != NS_ERROR_XSLT_WRONG_MIME_TYPE) {
+	 mCompiler->cancel(aStatusCode);
which code path results in OnStopRequest being called with our error code?

should txMozillaXSLTProcessor::SetSourceContentModel return mTransformResult on
error?

// XXX We might want to call SetDefaultStylesheets here
IIRC, jst just removed that from the tree.

the error texts have room for improvement, but I need to see them in action
before making suggestions.

these are the comments from glancing on the patches.
I'm not sure that I like the fact that we have yet another place to generate an
error doc. Not sure if it's worth the hassle to share that somehow.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> +    channel->GetContentType(contentType);
> +    if (contentType.IsEmpty()) {

This test will never be true.  The channel will return a type of
"application/x-unknown-content-type" or a real type gotten by some means.

Comment 8

[Axel Hecht]

about content-policy, first thing I heard.
after some pointers in #mozilla, this is about 
http://lxr.mozilla.org/seamonkey/source/content/base/public/nsIContentPolicy.idl
Not sure what the ctxt and window arguments are supposed to be, though.
Especially the ctxt. Needing the window sounds like we should trigger this from
content. It'd be interesting to know how granular the content policy might get,
can things succeed for same-origin-policy but fail for content-policy? Precisely,
would a policy allow for a stylesheet load of the base, but not allow the 
stylesheet load for an import/include adhering to same-origin-policy?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Not sure what the ctxt and window arguments are supposed to be, though.

That's in the process of being clarified... Note that at the moment there _are_
no content policies for stylesheets; that may change, however.

You could just pass nsnull for the context for now; then when the API is fixed,
we will look at all callers and do something reasonable.

The content policy can get arbitrarily granular, since embeddors can drop in
whatever content policy modules they want.  In particular, it could decide to
block all loads of urls with a 'c' in the filename... ;)

Comment 10

[Heikki Toivonen (remove -bugzilla when emailing directly)]

This should block 1.4b because we need the same-origin checks.

Comment 11

[Axel Hecht]

for what it's worth, here is what the CSSLoader does.
http://lxr.mozilla.org/seamonkey/source/content/html/style/src/nsCSSLoader.cpp#918

as this should be a blocking and we're not having a policy for stylesheets as of
now, I vote to put content-policy off into a different bug.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Yeah, sure.  As long as it happens...

Comment 13

[Axel Hecht]

filed bug 203211 for content-policy

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 121533 [details] [diff] [review]
Patch for mozilla/content

r=sicking

Comment 15

[Axel Hecht]

How about just using
NS_FORWARD_NSISTREAMLISTENER(mListener->)
in nsForceXMLListener.

NS_ERROR_XSLT_GET_NEW_HANDLER should actually be a success, IMHO. We should fix
that now, before making people localize that.

txStylesheetSink::OnDataAvailable should set the URL as sourceText, IMHO.
Same for network errors.

We should have a new bug to cancel loads and compiles on stylesheet compilation
errors.

Here a few suggestions on error strings.

+1  = Parsing a XSLT stylesheet failed.
+2  = Parsing a XPath expression failed.
+5  = XSLT transformation failed.
+6  = XSLT/XPath tried to call an unknown function.
+7  = XSLT Stylesheet (possibly) contains a recursion.
+8  = Attribute value illegal in XSLT 1.0.
+9  = A XPath expression was expected to return a NodeSet.
+10 = XSLT transformation was terminated by <xsl:message>.
+11 = A network error occured loading a XSLT stylesheet:
+12 = A XSLT stylesheet does not have an XML mimetype:

The ':' end is for those that have the URL as source info coming, but a '.'
would be fine for me, too. Could you put a warning to localizers into the 
properties file that those numbers are subject to change? We do need to
discuss stuff like NS_ERROR_XSLT_ALREADY_SET in more detail than we have time
right now.
Didn't test the patch, but those are my comments for this set of patches.

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 121534 [details] [diff] [review]
Patch to move buster Makefile.in, jar.mn etc into buster dir

sr=jst

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 121533 [details] [diff] [review]
Patch for mozilla/content

sr=jst

Comment 18

[Peter Van der Beken [:peterv]]

Attachment: Patch for mozilla/extensions/transformiix v1.1

Comment 19

[Peter Van der Beken [:peterv]]

Attachment: Patch for mozilla/extensions/transformiix v1.2

Comment 20

[Axel Hecht]

Comment on attachment 121533 [details] [diff] [review]
Patch for mozilla/content

content doesn't build on windows.
'LoadDocument': redefinition; different type modifiers

Reverted nsresult back to NS_IMETHOD. The other option would be to rename that
method.

Comment 21

[Peter Van der Beken [:peterv]]

Actually, the fix is to remove NS_IMETHODIMP from the implementation.

Comment 22

[Axel Hecht]

Comment on attachment 122018 [details] [diff] [review]
Patch for mozilla/extensions/transformiix v1.2

r=[e-mail] with the NS_IMETHODIMP fixed.
I personally prefer
"chrome://xslt/locale/xslt.properties"
(like p3p does) over 
"chrome://communicator/locale/layout/xslt.properties"
but either way is fine for me.

Comment 23

[Peter Van der Beken [:peterv]]

chrome://xslt/locale/xslt.properties needs a xslt/content too, so I'll keep it
in communicator for now.

Comment 24

[Pierre Chanial]

After 1.4, we'll need to move the files in communicator/layout elsewhere (bug
111339). But for now, I think adding one more file is not a problem.

Comment 25

[Johnny Stenback (:jst)]

Comment on attachment 122018 [details] [diff] [review]
Patch for mozilla/extensions/transformiix v1.2

+#define NS_ERROR_XSLT_GET_NEW_HANDLER			\
+    NS_ERROR_GENERATE_SUCCESS(NS_ERROR_MODULE_XSLT, 1)
+

Um, NS_ERROR_..., but it's a SUCCESS code? Seems wrong to me...

Other than that, sr=jst

Comment 26

[Peter Van der Beken [:peterv]]

Comment on attachment 121533 [details] [diff] [review]
Patch for mozilla/content

This adds a new method to load an XML document. Low-risk, only used for
documents loaded from XSLT stylesheets (for now).

Comment 27

[Peter Van der Beken [:peterv]]

Comment on attachment 121534 [details] [diff] [review]
Patch to move buster Makefile.in, jar.mn etc into buster dir

Move all the files of the buster XSLT test harness into the buster directory.
Not part of the regular build.

Comment 28

[Peter Van der Beken [:peterv]]

Comment on attachment 122018 [details] [diff] [review]
Patch for mozilla/extensions/transformiix v1.2

This is the main patch for this bug. Adds back security checks for stylesheet
loads and add error reporting for loading of XSLT stylesheets (important for
stylesheet authors). This code only runs for XSLT transformed pages, is
well-tested and apart from the security checks only runs when something goes
wrong with the loading of an XSLT stylesheet.

Comment 29

[Asa Dotzler [:asa]]

Comment on attachment 122018 [details] [diff] [review]
Patch for mozilla/extensions/transformiix v1.2

a=asa (on behalf of drivers) for checkin to 1.4b

Comment 30

[Peter Van der Beken [:peterv]]

Checked in.

Comment 31

[Axel Hecht]

mass verifying