Bug 8099 - REGRESSION: XMLHttpRequest lowercase post requests broken
: REGRESSION: XMLHttpRequest lowercase post requests broken
Status: RESOLVED FIXED
: WebKit
XML DOM
: 420+
: Macintosh Mac OS X 10.4
: P1 Normal
Assigned To:
:
: Regression
:
:
  Show dependency treegraph
 
Reported: 2006-03-31 01:28 PST by
Modified: 2006-04-03 09:36 PST (History)


Attachments
proposed fix (4.02 KB, patch)
2006-04-01 14:13 PST, Alexey Proskuryakov
darin: review+
Review Patch | Details | Formatted Diff | Diff
revised fix (5.97 KB, patch)
2006-04-02 06:14 PST, Alexey Proskuryakov
darin: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2006-03-31 01:28:21 PST
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.
------- Comment #1 From 2006-04-01 14:13:36 PST -------
Created an attachment (id=7447) [details]
proposed fix

Firefox does uppercase the method.
------- Comment #2 From 2006-04-01 15:23:06 PST -------
(From update of attachment 7447 [details])
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.
------- Comment #3 From 2006-04-02 06:14:49 PST -------
Created an attachment (id=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.
------- Comment #4 From 2006-04-02 16:41:55 PST -------
(From update of attachment 7458 [details])
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
------- Comment #5 From 2006-04-03 09:36:55 PST -------
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.