Bug 8099 – REGRESSION: XMLHttpRequest lowercase post requests broken

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Jonas Munk]

Think this is a easy to fix regression – from the latest request method fixes :-)

When using req.open("post",url,true) (with lowercase "post" instead of uppercase "POST" the request method is sent as lowercase "post".

Of course there is no such thing as a lowercase http POST request and at least PHP doesn't understand it.

Safari 2.0.3 (417.9.2) translates req.open("post",url,true) into a uppercase POST.

I noticed this because Prototype and Scriptaculous sometimes calls open() with lowercase "post".

I know you SHOULD use uppercase when calling open() but it seems not everyone does it :-( and Prototype gets used by many people – so I think its a good idea to translate post into POST

Note: haven't checked GET, PUT etc.

[Alexey Proskuryakov]

Created attachment 7447 [details]
proposed fix

Firefox does uppercase the method.

[Darin Adler]

Comment on attachment 7447 [details]
proposed fix

Whose job should it be to do this uppercasing? Should this be in TransferJob so we don't have to do it two different places in XMLHttpRequest?

Can m_method be changed into a String instead of a DeprecatedString so that we can do comparisons on it using equalIgnoringCase instead of constantly calling lower() on it?

Anyway, r=me.

[Alexey Proskuryakov]

Created attachment 7458 [details]
revised fix

> Whose job should it be to do this uppercasing? Should this be in TransferJob so
> we don't have to do it two different places in XMLHttpRequest?

  "Get" or "put" are valid methods, different from "GET" and "PUT", so TransferJob probably shouldn't change their case. However, I have now found that Firefox has more complex logic - it uppercases methods it knows about, and leaves others untouched. This is a bit weird, but since we are matching their quirk here, we should probably honor this further complication, too. This new patch uses a list of headers from Firefox (nsHttpAtomList.h).

> Can m_method be changed into a String instead of a DeprecatedString so that we
> can do comparisons on it using equalIgnoringCase instead of constantly calling
> lower() on it?

  This new patch does comparisons via ==; since there are many other uses of DeprecatedString in this class, I think that they should rather be all changed is a separate cleanup patch.

[Darin Adler]

Comment on attachment 7458 [details]
revised fix

The number of different methods listed in XMLHttpRequest::open is great enought that we might even want to use a HashSet. If we use CaseInsensitiveHash, we won't even need to call upper! You can call find, then get the value out of the hash, something like this:

    HashSet<String, CaseInsensitiveHash>::const_iterator it = knownMethods.find(method);
    if (it != knownMethods.end())
        m_method = *it;

The only obstacle is that currently we only have CaseInsensitiveHash defined for StringImpl*, not String or RefPtr<StringImpl*>. But since this is only a single global set, we could just ref the StringImpl* instances and then use a HashSet<StringImpl*, CaseInsensitiveHash>.

I suggest the comment:

+    // Methods names are case-sensitive, but Firefox uppercases methods it knows

be changed to something more like:

    // Method names are case sensitive. But since Firefox uppercases method names it knows, we'll do the same.

The code is good as-is. r=me

[Alexey Proskuryakov]

Perhaps it would also be possible to use AtomicStrings in a way similar to HTMLNames - but since the HTTP request itself is so slow, I am not sure if optimizing these comparisons is necessary.

Landed with a changed comment.

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.