User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.0.6) Gecko/2009011913 Firefox/3.0.6 Ubiquity/0.1.5 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.0.6) Gecko/2009011913 Firefox/3.0.6 Ubiquity/0.1.5 For some reason, when doing an XmlHttpRequest with method name "LINK" (defined in RFC 2068), the actual method name transferred is "Link" (last three letters lowercased). Reproducible: Always

Live HTTP Headers isn't enough for seeing this problem with http://greenbytes.de/test/xhrmethods.html

Steps to reproduce: 1) Load a page that contains an HTTP header called Link as a response header: http://www.hixie.ch/tests/adhoc/http/link/001.html 2) Open Live HTTP Headers 3) Navigate to http://greenbytes.de/test/xhrmethods.html 4) Press the button. 5) Inspect the Live HTTP headers log.

Further proof of this being related to previous occurance of HTTP headers: "CONTENT-LENGTH" gets lowercased, while "ONTENT-LENGTH" does not.

This is caused by the way we resolve HTTP atoms. nsHttp::ResolveAtom first look into hashtable if there is already such atom and if it is unknown it is created. The comparison is case insensitive and this is a problem when resolving methods since they are case sensitive (RFC 2616 section 5.1.1). "Link" is defined as atom at http://hg.mozilla.org/mozilla-central/annotate/3f5521e1c5a3/netwerk/protocol/http/src/nsHttpAtomList.h#l97 and this atom is resolved when somebody uses method "LINK". Hotfix for this bug can be removing header atom "Link" and adding method atom "LINK". This would cause sending header Link uppercase but this is OK since headers are case insensitive (RFC 2616 section 4.2). But I think that we should remove methods from current atom list and handle them separately. With current behavior some buggy or malicious JS code can prevent other JS applications from working properly. For example when somebody issues XMLHttpRequest with method undefined in nsHttpAtomList.h (e.g ACL, CHECKOUT, MKWORKSPACE etc.) with wrong case (e.g. acl) then this wrong command is stored into atom list and no other application can issue correct request. Also it is possible to have HTTP server that handles standard method "GET" and some user defined method "get" which is not possible to call in firefox. Separating methods into case sensitive list would solve this problem. Any opinions?

Yeah, it doesn't make sense to use case-insensitive atoms for HTTP methods. They should just use our normal atom table, no?

> They should just use our normal atom table, no? Yes, I think so. But changing this could break extensions that use lowercase method names. Can we really do it?

You mean use lowercase names for "get", "head", "post"? We can upcase some whitelist before atomizing if we need to... The XHR spec has something to say about this too, no?

You're right. XHR doc says: If stored method case-insensitively matches CONNECT, DELETE, GET, HEAD, OPTIONS POST, PUT, TRACE, or TRACK let stored method be the canonical uppercase form of the matched method name. The question is if we should implement it in nsXMLHttpRequest or in nsHttpChannel.

I'm happy with the latter, honestly.

Changes HTTP methods to case sensitive except methods CONNECT, DELETE, GET, HEAD, OPTIONS, POST, PUT, TRACE, TRACK

Comment on attachment 371678 [details] [diff] [review] patch v1 >+#define HTTP_METHOD_ATOM(_name, _value) nsIAtom* nsHttp::_name = nsnull; No need for the " = nsnull" part. >+_name = NS_NewPermanentAtom(_value); \ I'd think you want static atoms, not permanent atoms here. See how nsGkAtoms is set up, for example. >+++ b/netwerk/protocol/http/src/nsHttpChannel.cpp >@@ -1916,17 +1916,19 @@ nsHttpChannel::CheckCache() I assume this (and all places where HTTP code gets these method atoms from strings) is only called on the main thread, right? > nsHttpChannel::SetRequestMethod(const nsACString &method) >+ if (!nsHttp::IsValidToken(PromiseFlatCString(method))) You could probably do this after uppercasing, on a known-flat string, and avoid the extra PromiseFlatCString call, no? >+ // We've changed method names to case sensitive in bug 477578. Following >+ // methods are kept case insensitive to keep backward compatibility and >+ // to satisfy XMLHttpRequest specification which demands it. Is there a reason to not just atomize the uppecase method and then do atom compares? Also, I'd prefer if we put this information in the atom list. So instead of just having HTTP_METHOD_ATOM, have HTTP_METHOD_ATOM and HTTP_CASE_INSENSITIVE_METHOD_ATOM, define the latter to the former if it's not defined, then here define METHOD_ATOM to nothing and CASE_INSENSITIVE_METHOD_ATOM to do the compare. The rest looks fine.

> I assume this (and all places where HTTP code gets these method atoms from > strings) is only called on the main thread, right? Yes, I think so. This happens in nsHttpChannel::SetRequestMethod() and in nsHttpChannel::CheckCache() which is called only from nsHttpChannel::Connect().

Comment on attachment 371762 [details] [diff] [review] patch v2 Looks good.

Unfortunately, this patch doesn't apply anymore (because files moved, and a few things seem to be refactored).

Pushed to try server http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=766bc8757176. Does this need a sr?

works for me (tested: http://www.mnot.net/javascript/xmlhttprequest/)

This code is broken, it refcounts atoms off the main thread. See the assertions in the log at https://tbpl.mozilla.org/php/getParsedLog.php?id=6499610&tree=Firefox&full=1.

Ugh. I didn't realize we create request heads on the socket thread! That's pretty seriously broken. Kyle, did you back this out already, or does that still need to be done?

I have it queued up, but haven't pushed yet.

The stacks look like: ###!!! ASSERTION: wrong thread: 'NS_IsMainThread()', file /builds/slave/m-cen-osx64-dbg/build/xpcom/ds/nsAtomTable.cpp, line 306 PermanentAtomImpl::AddRef [xpcom/ds/nsAtomTable.cpp:307] nsCOMPtr<nsIAtom>::nsCOMPtr [nsCOMPtr.h:568] nsHttpRequestHead::nsHttpRequestHead [netwerk/protocol/http/nsHttpRequestHead.h:55] nsHttpConnection::SetupProxyConnect [netwerk/protocol/http/nsHttpConnection.cpp:842] nsHttpConnection::Activate [netwerk/protocol/http/nsHttpConnection.cpp:183] nsHttpConnectionMgr::DispatchTransaction [netwerk/protocol/http/nsHttpConnectionMgr.cpp:866] nsHttpConnectionMgr::nsHalfOpenSocket::OnOutputStreamReady [netwerk/protocol/http/nsHttpConnectionMgr.cpp:1577] nsSocketOutputStream::OnSocketReady [netwerk/base/src/nsSocketTransport2.cpp:514] nsSocketTransport::OnSocketReady [netwerk/base/src/nsSocketTransport2.cpp:1534] nsSocketTransportService::DoPollIteration [netwerk/base/src/nsSocketTransportService2.cpp:736] nsSocketTransportService::Run [netwerk/base/src/nsSocketTransportService2.cpp:634] nsThread::ProcessNextEvent [xpcom/threads/nsThread.cpp:631] NS_ProcessNextEvent_P [obj-firefox/xpcom/build/nsThreadUtils.cpp:245] nsThread::ThreadFunc [xpcom/threads/nsThread.cpp:271] _pt_root [nsprpub/pr/src/pthreads/ptthread.c:190] libSystem.B.dylib + 0x3a536

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/d300ffff0be7

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #23) > Backed out: > https://hg.mozilla.org/integration/mozilla-inbound/rev/d300ffff0be7 Merge of backout: https://hg.mozilla.org/mozilla-central/rev/d300ffff0be7

(In reply to Michal Novotny (:michal) from comment #12) > Yes, I think so. This happens in nsHttpChannel::SetRequestMethod() and in > nsHttpChannel::CheckCache() which is called only from > nsHttpChannel::Connect(). CheckCache() will be done off the main thread real soon now.

Any updates on that one? I have to rewrite quite a bunch of frameworks and libraries, that do not know of case-sensitivity for http-verbs ... they all just uppercased them. Even if that's against the docs, it would've worked ;)

I wasn't aware of this bug - thanks simonsimcity imo, rather than having two atom tables, methods should just not be atoms. In most cases we'll just be able to strcmp method, GET and set a bool and be done with it rather than worrying about locked hashtables and whatnot. I'll work on a patch... I expect it will be large but pretty mechanical.

Comment on attachment 8393111 [details] [diff] [review] http methods should be case sensitive boris, r? just for the content/base/src/nsXMLHttpRequest.cpp bit In HTTP the method name is case sensitive, but xhr requires a few particular methods to be upcased and the rest to be left case sensitive... so the patch accommodates (and tests) each different case. note that xhr is also supposed to throw an error on CONNECT, (along with trace and track which it was doing) and that I plugged that omission.

Comment on attachment 8393111 [details] [diff] [review] http methods should be case sensitive r=me on the DOM bits, but mind making the return code there for the trace/connect/track case be NS_ERROR_DOM_SECURITY_ERR, since the spec wants a SecurityError? And maybe file a followup on doing the other checking the spec calls for here (e.g. making sure the method matches the production at http://tools.ietf.org/html/rfc2616#section-5.1.1

(In reply to Boris Zbarsky [:bz] from comment #32) > And maybe file a followup on doing the other checking the spec calls for > here (e.g. making sure the method matches the production at > http://tools.ietf.org/html/rfc2616#section-5.1.1 Nowadays: http://greenbytes.de/tech/webdav/draft-ietf-httpbis-p1-messaging-26.html#rfc.section.3.1.1 (in RFC Editor queue). But no, the ABNF production did not change.

> Nowadays Probably need a bug on the XHR spec then to get it updated; I linked to the thing it links to.

(In reply to Boris Zbarsky [:bz] from comment #34) > Probably need a bug on the XHR spec then to get it updated; I linked to the > thing it links to. -> <https://www.w3.org/Bugs/Public/show_bug.cgi?id=25097>

(In reply to Boris Zbarsky [:bz] from comment #32) > > r=me on the DOM bits, but mind making the return code there for the > trace/connect/track case be NS_ERROR_DOM_SECURITY_ERR, since the spec wants > a SecurityError? > ok > And maybe file a followup on doing the other checking the spec calls for > here (e.g. making sure the method matches the production at > http://tools.ietf.org/html/rfc2616#section-5.1.1 the channel already enforces that

Comment on attachment 8393111 [details] [diff] [review] http methods should be case sensitive Review of attachment 8393111 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, just one comment on the mochitest that may or may not be an issue. ::: netwerk/protocol/http/nsHttpChannel.cpp @@ +630,5 @@ > + method == nsHttpRequestHead::kMethod_Head || > + method == nsHttpRequestHead::kMethod_Options) { > + return true; > + } > + nit: whitespace ::: netwerk/protocol/http/nsHttpRequestHead.h @@ +62,5 @@ > return mHeaders.SetHeader(h, nsDependentCString(v), merge); > return NS_OK; > } > > + nit: whitespace ::: netwerk/test/mochitests/test_xhr_method_case.html @@ +38,5 @@ > +] > + > +function doIter(index) > +{ > + SimpleTest.waitForExplicitFinish(); So here comes my complete lack of experience with mochitests: this seems like a case of unbalanced calls to waitForExplicitFinish. Perhaps mochitest doesn't care, in which case it's possibly fine, but it just looks weird to me. If this call in each iteration of the loop isn't required, remove it, to prevent future mochitest-inexperienced people like me from being confused :)

nick, > > ::: netwerk/test/mochitests/test_xhr_method_case.html > @@ +38,5 @@ > > +] > > + > > +function doIter(index) > > +{ > > + SimpleTest.waitForExplicitFinish(); > that wasn't supposed to be there - just a bug you caught. Thanks!

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/20d8f9639b9c

Thanks for all the work you put in here, :mcmanus and all others involved! Haven't thought it would've been fixed that fast (even so I now have to wait for Firefox 31 :) )

I reproduced the issue on Firefox 30 using scenario from comment 3. Verified as fixed on Win 7 64bit, using Firefox 31 Beta 2: - User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 - BuildID: 20140616143923