197087 - xml-rpc should use XMLHttpRequest (and other updates)

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Samuel Sieb]

I'm converting xml-rpc to use XMLHttpRequest.  This should fix all the security
bugs, fix a couple of other bugs, and make it faster.

Comment 1

[Samuel Sieb]

Attachment: convert to XMLHttpRequest v1

Big patch.  I haven't had a chance to check xml-rpc fault handling.  I will
also attach the whole modified file for people to test with.

Comment 2

[Samuel Sieb]

Attachment: nsXmlRpcClient.js

Put this file in the components directory under the mozilla bin directory and
restart mozilla.

Comment 3

[Samuel Sieb]

cc'ing a few of people I expect would be interested in this.  Please test it out
and tell me if it solves the problems you've been having.

Comment 4

[Mike Lee]

"Error! (0) parseArray is not defined", so is array supported yet? The example
program seem to work now.

Comment 5

[Samuel Sieb]

Attachment: nsXmlRpcClient.js take 2

Sorry, I'm not sure why that one worked at all...  Try this one.

Comment 6

[Samuel Sieb]

Attachment: nsXmlRpcClient.js take 3

Ok, I think it's solid now.  It seems to work on structs, arrays, and faults.

Comment 7

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

I think you should be able to create XMLHttpRequest object simply like this:

new XMLHttpRequest()

Also, please attach a diff as well.

Comment 8

[C. Daniel Mojoli B.]

The new nsXmlRpcClient.js (take 3) breaks my application. It is returning
nsISupportsCString instead of nsISupportsString, so my onResults' QIs throw
exceptions and fail.

Now, I have been using my bug 157797 patch
http://bugzilla.mozilla.org/attachment.cgi?id=100049&action=view for a long time
because it doesn't trigger the aforementioned bug. I wasn't tracking any new
developments in nsXmlRpcClient.js outside that particular bug's scope. When did
nsISupportsCStrings start being sent instead of nsISupportsString?

Comment 9

[Samuel Sieb]

The CString change was for all of mozilla: bug 157624.

Comment 10

[Samuel Sieb]

Attachment: conversion diff v2

Ok, I think this is ready for reviewing.

Comment 11

[Daniel Fernandez]

I'm experiencing something I'm not sure if it's normal, when I do a request (via
xmlrpc) to a url that requires authentication, if failed it shows up the default
authentication window used by mozilla. In a custom made xul app this is not good
because I have my own authentication window, I hadn't experienced this in
previous versions of the library (the ones without XMLHttpRequest), maybe is
related with another thing and there is a way to avoid it that I'm not aware of,
could someone clarify?

Thanks

Comment 12

[Samuel Sieb]

Adding darin to the CC list.  Darin, any idea how to not get the authentication
request window?  I had assumed that since there was the option of passing
authentication information to XMLHttpRequest that it would just fail if it was
incorrect.

Comment 13

[Samuel Sieb]

Attachment: nsXmlRpcClient.js containing the workaround for bug 201823

Comment 14

[C. Daniel Mojoli B.]

I have finally tested attachment 121022 [details] as part of a migration of our Mozilla
application from 1.2.1 to 1.4. Our 60,000 line client exclusively uses Xml-Rpc.

Up to Mozilla 1.2.1, I used a modified copy of the old nsXmlRpcClient.js. You
can read all about it in bug 157797.

When first moving to Moz 1.4rc1, I decided not to use my variant and went for
the plain vanilla nsXmlRpcClient.js as found in 1.4. This didn't work apparently
due to some stream closed problem.

I recalled this bug and tried attachment 121022 [details]. Unfortunately, it exhibits bug
157797 behavior. To avoid that behavior the trivial "patch" at the end of this
comment needs to be applied.

As stated in bug 157797 there seems to be a deeply rooted problem somewhere in
XPConnect, possibly related to handling of JS Contexts.

-----------------------

@@ -309,7 +311,7 @@
                 obj=obj.QueryInterface(Components.interfaces['nsISupports' +
                     sType]);
                 writer.startElement('i4');
-                writer.write(obj.toString());
+                writer.write(obj.data.toString());
                 writer.endElement('i4');
                 break;

@@ -325,7 +327,7 @@
                 obj=obj.QueryInterface(Components.interfaces['nsISupports' +
                     sType]);
                 writer.startElement('string');
-                writer.write(obj.toString());
+                writer.write(obj.data.toString());
                 writer.endElement('string');
                 break;

@@ -334,7 +336,7 @@
                 obj=obj.QueryInterface(Components.interfaces['nsISupports' +
                     sType]);
                 writer.startElement('double');
-                writer.write(obj.toString());
+                writer.write(obj.data.toString());
                 writer.endElement('double');
                 break;

Comment 15

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

I was reading bug 180049, and wondered if nsIHttpAuthenticator would be the
answer that is blocking us from fixing this bug. If the custom XUL app
(described in comment #11) provides its own implementation of
nsIHttpAuthenticator, will that be called in lieu of the default authentication
window?

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

if you want an own password prompt, you should implement nsIAuthPrompt as well
as nsIInterfaceRequestor (set up to return an nsIAuthPrompt when asked for it),
and set that interfacerequestor as notificationCallbacks on the nsIChannel that
you're using.

Comment 17

[C. Daniel Mojoli B.]

Attachment: Workaround to line 158: parent has no properties problem

This patch is against attachment 121822 [details] [diff] [review].

Under semi-certain conditions (2-5% of the time, 10-20% in some of our code
sections) we are having problems with attachment 121822 [details] [diff] [review] (which we use heavily),
where we get the following error in nsXmlRpcClient.js.

X----------------------------------------------
Error: parent has no properties
Source File: file:///.../mozilla/components/nsXmlRpcClient.js	Line: 158
X----------------------------------------------

If you read the code there is no (reasonable) way that error could happen
without a problem with subjacent layers. We've spotted the error in Moz1.4 and
1.5 with attachment 121822 [details] [diff] [review] in multiple machines.

We suspect of a race condition along the lines of XPConnect, XPCom, and
handling of JSContexts. We have many observations, but most result too nebulous
to comment here.

After a lot of observation we produced a patch that has worked for us. We
cannot claim that this workaround is correct for the problem is yet to be
pinpointed (we have a good idea though), but so far it seems to be holding.
Please notice how the patch essentially changes nothing, but side-effects play
nicely.

Comment 18

[basic]

Samuel, what's the status of this bug? I'd like to help.

Comment 19

[Alejandro Borges]

Oh man. I still see  problems in mozilla with xmlrpc. In fact, ive never gotten
it to work properly (a really simple call of three parameters hangs the
component in the parse loop until mozilla asks me if i wanna stop it).

So I use the modifications from the mozblog crowd and they work fine. I think
theirs is also cleaner in a way because I dont really understand the parser of
the one included in mozilla or why should the component rewrite the parser if we
already have xmlhttprequest.

So, id like to see if the owner of this would find it usefull to rethink the
implementation towards the one depicted here: 

http://mozblog.mozdev.org/nsXmlRpcClient.js

I didnt make it, i just use it and most of the working articles ive found about
xmlrpc and mozilla mention one should overwrite the original with this one.

Its obviously based on the original, I think even Samuel worked on it and still,
its not what comes with my mozilla ff or suite. Why?

Comment 20

[mike]

I agree with comment #19. The file linked there was very helpful in fixing a
serious problem i had gettin xml-rpc to perform the most basic of tasks. I am
also putting a vote on this bug.

Comment 21

[seth]

I've looked at the version they use for mozblog (mozblog.mozdev.org), and the
tests I ran it through all worked fine. Can't say the same for what's shipping
right now. Even the ultra-simple example in the source
<http://lxr.mozilla.org/seamonkey/source/extensions/xml-rpc/test/xml-rpc.xul>
fails with the current version of nsXmlRpcClient.js, but it works fine with the
mozblog version.

(Actually, that's not exactly true, because it's trying to open a connection to
betty.userland.com, which doesn't work anymore. Change that to www.xmlrpc.com,
*then* it works fine.)

Comment 22

[Fabien]

Attachment: nsXmlRpcClient.js take 3 fix for authentication dialog issue

Comment 23

[Fabien]

Hello all, i've just uploaded a patch...and i'm looking for some comments on it. :-)

What this patch does is:
  * Fix the unwanting popup authentification dialog window. This fix uses the
same workaround that the one in the actually shipping version of
nsXmlRpcClient.js. It sets the notificationCallback attribute of the channel to
point back to us, so that we can spoof the authentication dialog.
  * Fix network error reporting. Before, the onerror event of the XMLHttpRequest
object was not used. Doing an XML-RPC request to an unknown host for instance
was never reported back to the caller.

I'm impatiently waiting for comments of experienced Mozilla hackers :-), and i
hope, this will allow us to finally submit this new XML-RPC implementation :-)

Cheers,

Fabien

Comment 24

[Samuel Sieb]

Thank you for the help, I'll put the authentication fix in with the rest of the
patch and it should be reviewable now.  I have one question first though.  Do
you want to only provide the username and password, or do you want the option to
get a callback requesting it?

Comment 25

[Fabien]

I'm not sure I really understand the sense of your question, but i'll try to
answer anyway :-)
The essence of this patch is to be able to handle authentification failures
without popping default Mozilla prompt asking for username/password.
Here is how it behaves:
  - The programmer specifies the credentials in the JavaScript code.
  - If credentials are Ok, then the XML-RPC call succeeds.
  - If credentials are incorrect, then the listener onError method is called
with the "Server returned status 401" error message, which is the standard  HTTP
error code for user authentification failure.
  - The programmer can catch this error and do whatever he likes, maybe popup a
custom dialog to say that authentication has failed.

So for now it is the option to "only provide user name and password".

Basically it is how the currently shipping nsXmlRpcClient.js works.
To be honest with all, I wrote this fix so that we can finally try to submit
this new XML-RPC immplemantion, as it works *far better* than what is currently
shipping.

'hope it helps :-)

Cheers,

Fabien

Comment 26

[Fabien]

Hello guys, I hope the Xmas season was good and that now you're back in front of
your computers ;-)

Samuel, could you please put the whole stuff into the reviewing process?
I think a lot of people (including me :-)) would be a lot happier with this
brand new XML-RPC ;-)

Cheers,

Fabien

Comment 27

[Samuel Sieb]

Attachment: new version of nsXmlRpcClient.js

This one contains all the fixes except the parent problem which needs to be
looked into further.

Comment 28

[Calvin Hendryx-Parker]

In Deer Park this component is broken because of a change in the default
Content-Type to application/xml when the XML-RPC spec calls for text/xml.

To fix just add:

        this.xmlhttp.setRequestHeader('Content-Type','text/xml')
        
After opening the connection and before sending the request.

Comment 29

[Samuel Sieb]

*** Bug 287415 has been marked as a duplicate of this bug. ***

Comment 30

[Samuel Sieb]

Attachment: latest version of nsXmlRpcClient.js

Comment 31

[Samuel Sieb]

Attachment: diff for reviewing

Comment 32

[Samuel Sieb]

Attachment: corrected latest version of nsXmlRpcClient.js

of course, 30 seconds after I attached the previous version, I discovered a minor bug, a spelling mistake and a change that wasn't necessary...

Comment 33

[Samuel Sieb]

Attachment: idl file changes

Comment 34

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

I would rather that you asked someone else to review this patch. I don't know what the code is supposed to do and on top of that there seems to be many changes, cleanups and updates rolled into a single patch.

Comment 35

[lekma]

samuel,

i tried your last version in a xulrunner app, i get:

Error: XMLHttpRequest is not defined

Comment 36

[Samuel Sieb]

Attachment: minor updates

I'll upload a new full file after review.

Comment 37

[Doron Rosenberg (IBM)]

Comment on attachment 211818 [details] [diff] [review]
minor updates

>Index: nsXmlRpcClient.js
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xml-rpc/src/nsXmlRpcClient.js,v
>retrieving revision 1.37
>diff -u -r1.37 nsXmlRpcClient.js
>--- nsXmlRpcClient.js	12 Jan 2006 16:36:24 -0000	1.37
>+++ nsXmlRpcClient.js	14 Feb 2006 05:57:14 -0000
>+        this.xmlhttp = new XMLHttpRequest();
>+        if (this._useAuth) {
>+            this.xmlhttp.open('POST', this._serverUrl, true,
>+                              this._username, this._password);
>+        } else {
>+            this.xmlhttp.open('POST', this._serverUrl);
>         }

The useAuth case seems to be doing a sync request.

>+    _onload: function(e) {
>+        var result;
>+        var parent = e.target.parent;
>+        parent._inProgress = false;
>+        parent._responseStatus = e.target.status;
>+        parent._responseString = e.target.statusText;
>+        if (!e.target.responseXML) {

I have to admit, I have never seen that syntax (using e in an xmlHttpRequest onload). Usually I've seen having xmlhttp in a global scope and keying off it.  So I can't really say if using e.target will work or is a good idea...

Other than that, looks ok.

Comment 38

[James Ross]

(In reply to comment #37)
> The useAuth case seems to be doing a sync request.

The 3rd parameter is called "async" and "The default value is true.", see:

http://lxr.mozilla.org/mozilla/source/extensions/xmlextras/base/public/nsIXMLHttpRequest.idl#183

> 
> >+    _onload: function(e) {
> >+        var result;
> >+        var parent = e.target.parent;
> >+        parent._inProgress = false;
> >+        parent._responseStatus = e.target.status;
> >+        parent._responseString = e.target.statusText;
> >+        if (!e.target.responseXML) {
> 
> I have to admit, I have never seen that syntax (using e in an xmlHttpRequest
> onload). Usually I've seen having xmlhttp in a global scope and keying off it. 
> So I can't really say if using e.target will work or is a good idea...

I can confirm that onload and onerror (but NOT onreadystatechange) get normal DOM events passed to them like other handlers, and that the target (and currentTarget and originalTarget) is the XMLHttpRequest object itself. See:

http://lxr.mozilla.org/mozilla/source/extensions/xmlextras/base/src/nsXMLHttpRequest.cpp#778
http://lxr.mozilla.org/mozilla/source/extensions/xmlextras/base/src/nsXMLHttpRequest.cpp#1408

Comment 39

[Samuel Sieb]

Comment on attachment 211818 [details] [diff] [review]
minor updates

checked in on trunk

Comment 40

[Darin Fisher]

Is XML-RPC something that existing extensions use?  I don't recall any browser component using this.  How important is it that this patch be included with FF2?

Comment 41

[seth]

Darin, some of us (I won't claim "a lot," but definitely some) have been waiting for this to be fixed for a long time. I'm tired of having to hack around it.

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is this something that's only exposed to extensions and not Web content?  Do you have any idea how many extensions use this, and what proportion of those need the improvements?

We'd also like to see a patch of this size baked on the trunk before considering it for the branch.  It doesn't appear to have landed on the trunk yet.

Comment 43

[James Ross]

David Baron, comment 39 and http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/extensions/xml-rpc/src/nsXmlRpcClient.js&rev=1.38 disagree with you - it has been on trunk since February.

Comment 44

[Samuel Sieb]

You picked the week I'm away to ask these questions, although I see that they've all been answered by others.  Yes, it's been in trunk for quite a while and there have been no issues reported.  I know that mozblog has been using an older version of this patch for a long time (I think they include it in their package).  I should mark the dependent bugs fixed now too, but I'll wait until I get home.  I requested branch approval when I checked it in on trunk, but no one responded.

Comment 45

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #43)
> David Baron, comment 39 and
> http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/extensions/xml-rpc/src/nsXmlRpcClient.js&rev=1.38
> disagree with you - it has been on trunk since February.

We don't have time to read through 40 comments in a triage meeting; if it was fixed on the trunk it should have been marked RESOLVED-FIXED.

Comment 46

[Christian :Biesinger (don't email me, ping me on IRC)]

fwiw this patch doesn't apply on the 1.8 branch:

patching file nsXmlRpcClient.js
Hunk #7 FAILED at 158.
Hunk #9 FAILED at 337.
Hunk #11 FAILED at 825.

Comment 47

[Samuel Sieb]

Attachment: patch for branch

This is the patch to bring the trunk changes to the branch.

Comment 48

[Samuel Sieb]

checked in on branch

Comment 49

[Jake Briggs]

Attachment: nsXmlRpcClient.js

Put this file in the components directory under the mozilla bin directory and
restart mozilla.

This does not appear to work. Do you have to do something special once the file is in the directory?