We're going to use a lightweight LRU cache for this, keyed off of referrer, http method, and target uri.

Marking P1 as we should really fix this for next beta.

This patch implements an LRU Cache and hooks it up to the first GET request sent for an access-controlled non-GET request.

Jonas - beta freeze is comin up!^

Comment on attachment 296267 [details] [diff] [review] Patch, v1 >Index: content/base/src/nsXMLHttpRequest.cpp >=================================================================== >RCS file: /cvsroot/mozilla/content/base/src/nsXMLHttpRequest.cpp,v >retrieving revision 1.211 >diff -u -8 -p -r1.211 nsXMLHttpRequest.cpp >--- content/base/src/nsXMLHttpRequest.cpp 12 Dec 2007 08:33:32 -0000 1.211 >+++ content/base/src/nsXMLHttpRequest.cpp 10 Jan 2008 02:36:06 -0000 >@@ -236,31 +236,34 @@ nsMultipartProxyListener::OnDataAvailabl > class nsACProxyListener : public nsIStreamListener, > public nsIInterfaceRequestor, > public nsIChannelEventSink > { > public: > nsACProxyListener(nsIChannel* aOuterChannel, > nsIStreamListener* aOuterListener, > nsISupports* aOuterContext, >+ nsIPrincipal* aReferrerPrincipal, > const nsACString& aRequestMethod) > : mOuterChannel(aOuterChannel), mOuterListener(aOuterListener), >- mOuterContext(aOuterContext), mRequestMethod(aRequestMethod) >+ mOuterContext(aOuterContext), mReferrerPrincipal(aReferrerPrincipal), >+ mRequestMethod(aRequestMethod) > { } > > NS_DECL_ISUPPORTS > NS_DECL_NSISTREAMLISTENER > NS_DECL_NSIREQUESTOBSERVER > NS_DECL_NSIINTERFACEREQUESTOR > NS_DECL_NSICHANNELEVENTSINK > > private: > nsCOMPtr<nsIChannel> mOuterChannel; > nsCOMPtr<nsIStreamListener> mOuterListener; > nsCOMPtr<nsISupports> mOuterContext; >+ nsCOMPtr<nsIPrincipal> mReferrerPrincipal; > nsCString mRequestMethod; > }; > > NS_IMPL_ISUPPORTS4(nsACProxyListener, nsIStreamListener, nsIRequestObserver, > nsIInterfaceRequestor, nsIChannelEventSink) > > NS_IMETHODIMP > nsACProxyListener::OnStartRequest(nsIRequest *aRequest, nsISupports *aContext) >@@ -285,25 +288,47 @@ nsACProxyListener::OnStartRequest(nsIReq > if (mRequestMethod.Equals(tok.nextToken(), > nsCaseInsensitiveCStringComparator())) { > rv = NS_OK; > break; > } > } > } > >+ if (nsXMLHttpRequest::sAccessControlCache && NS_SUCCEEDED(rv)) { >+ // Everything worked, check to see if there is an expiration time set on >+ // this access control list. If so go ahead and cache it. >+ nsCAutoString expirationString; >+ http->GetResponseHeader(NS_LITERAL_CSTRING("Method-Check-Expires"), >+ expirationString); >+ if (!expirationString.IsEmpty()) { >+ PRTime expirationTime, now = PR_Now(); >+ PRStatus status = PR_ParseTimeString(expirationString.get(), PR_TRUE, >+ &expirationTime); >+ // Only cache if we received a proper expiration header and we're still >+ // within the expiration time. >+ if (status == PR_SUCCESS && LL_UCMP(expirationTime, >=, now)) { The LL_ macros are considered obsolete these days, just do |expirationTime >= now|. Same in a few other places. You can also replace LL_ZERO in a few places with simply 0. >+nsAccessControlLRUCache::GetEntry(const nsACString& aMethod, >+ nsIURI* aURI, >+ nsIPrincipal* aPrincipal, >+ PRTime* _retval) >+{ >+ if (!IsInitialized()) >+ return PR_FALSE; >+ >+ nsCString key; >+ PRBool validKey = GetCacheKey(aMethod, aURI, aPrincipal, key); >+ NS_ENSURE_TRUE(validKey, PR_FALSE); >+ >+ CacheEntry* entry; >+ if (GetEntryInternal(key, &entry)) { >+ >+ // Let this be optional like the other hashtable implementations. >+ if (_retval) >+ *_retval = entry->value; Why? I'm generally against this design pattern. Usually it's easier to just use a dummy variable at the receiving end if you really don't care about the result. >+nsAccessControlLRUCache::PutEntry(const nsACString& aMethod, >+ nsIURI* aURI, >+ nsIPrincipal* aPrincipal, >+ PRTime aValue) >+{ >+ if (!IsInitialized()) { >+ PRBool success = mTable.Init(); >+ NS_ENSURE_TRUE(success, PR_FALSE); >+ } >+ >+ nsCString key; >+ PRBool validKey = GetCacheKey(aMethod, aURI, aPrincipal, key); >+ NS_ENSURE_TRUE(validKey, PR_FALSE); >+ >+ CacheEntry* entry; >+ if (GetEntryInternal(key, &entry)) { >+ // Entry already existed, just update the expiration time and bail. The list >+ // is updated as a result of the call to GetEntryInternal. I'd say "The LRU list" to make it clearer what list you're talking about. >+nsAccessControlLRUCache::GetCacheKey(const nsACString& aMethod, >+ nsIURI* aURI, >+ nsIPrincipal* aPrincipal, >+ nsACString& _retval) >+{ >+ NS_ENSURE_FALSE(aMethod.IsEmpty(), PR_FALSE); >+ NS_ENSURE_TRUE(aURI, PR_FALSE); >+ NS_ENSURE_TRUE(aPrincipal, PR_FALSE); Don't have release-code checks here. Having an assertion seems like a good idea, but more than that just encourages people to actually pass in bad values here. >+ _retval.Assign(aMethod); >+ _retval.Append(' '); >+ >+ nsCAutoString spec; >+ nsresult rv = aURI->GetSpec(spec); >+ NS_ENSURE_SUCCESS(rv, PR_FALSE); >+ >+ _retval.Append(spec); >+ _retval.Append(' '); >+ >+ nsCOMPtr<nsIURI> domain; >+ aPrincipal->GetDomain(getter_AddRefs(domain)); >+ NS_ENSURE_TRUE(domain, PR_FALSE); You don't want GetDomain here, that can be set by content code (though document.domain). You need to get the URI and get the domain out of that. However note that the URI can be empty if the principal is the system principal. Ideally the system principal should never get here, but I think that can still happen. In that case just use the empty string as domain. Also, put the domain before the spec, just in case the spec can contain spaces. That way we know that the first two spaces are always the field separators. >+ PRTime expiration; >+ if (!sAccessControlCache || >+ !sAccessControlCache->GetEntry(method, uri, mPrincipal, &expiration)) { >+ expiration = LL_ZERO; >+ } >+ else if (LL_UCMP(expiration, >=, PR_Now())) { >+ // Expired, remove it from the cache. >+ sAccessControlCache->RemoveEntry(method, uri, mPrincipal); Actually, no need to call RemoveEntry here (which would let you kill the whole RemoveEntry function). In the common case the data in the entry will just be overwritten when the GET request is done since we're likely to get a new expiration time if we got one for the first request. And if we don't we'll just continue to ignore it. I would just write it as PRTime expiration = 0 if (sAccessControlCache) { sAccessControlCache->GetEntry(method, uri, mPrincipal, &expiration)); } if (expiration < PR_Now()) { NS_NewChannel ... } And make GetEntry return void. >Index: content/base/src/nsXMLHttpRequest.h >+ nsAccessControlLRUCache(PRUint32 aMaxEntries = 100) >+ : mMaxEntryCount(aMaxEntries) No need for this member IMHO, just use the define right in the code. It's not likely that this class will be used elsewhere. >+private: >+ PRBool GetEntryInternal(const nsACString& aKey, CacheEntry** _retval); >+ >+ PR_STATIC_CALLBACK(PLDHashOperator) >+ RemoveExpiredEntries(const nsACString& aKey, nsAutoPtr<CacheEntry>& aValue, >+ void* aUserData); >+ >+ static PRBool GetCacheKey(const nsACString& aMethod, nsIURI* aURI, >+ nsIPrincipal* aPrincipal, nsACString& _retval); >+ >+ nsClassHashtable<nsCStringHashKey, CacheEntry> mTable; >+ PRCList mList; This won't work will it? The CacheEntry objects will be inline allocated by the hash table which means that they can move around when objects are inserted into the hash. That will make the pointers in mList either dangle or point to the wrong entries. >+ static void InitACCache() >+ { >+ sAccessControlCache = >+ new nsAccessControlLRUCache(ACCESS_CONTROL_CACHE_SIZE); >+ NS_WARN_IF_FALSE(sAccessControlCache, >+ "Failed to allocate access control cache!"); >+ } So the initialization is very inconsistent currently. You allocate the sAccessControlCache on startup, but that only half-way constructs the needed objects since you also need to initialize the hash. I suggest you don't do anything on startup, but then call InitACCache if needed when it's time to stick something into the cache (i.e. in OnStartRequest). And make InitACCache also initialize the hash so that you don't have to sprinkle IsInitialized checks everywhere. And make InitACCache return some fail code if initialization failed. You currently actually crash in nsXMLHttpRequest::OpenRequest if sAccessControlCache is null. looks good otherwise, but i'd like to see another patch. And actually, since you're already here. Could you change the GET request to be an OPTIONS request instead. This is per recent spec changes. That way you don't have to set the cache flags on the channel either.

(In reply to comment #4) > The LL_ macros are considered obsolete these days Ok, they're gone now. > Why? I'm generally against this design pattern. Ok. > I'd say "The LRU list" to make it clearer what list you're talking about. Updated. > Don't have release-code checks here. Changed to assertions. > You don't want GetDomain here, that can be set by content code (though > document.domain). Blegh, I still have a bunch to learn about this... Fixed. > However note that the URI can be empty if the principal is the system > principal. Ideally the system principal should never get here, but I think that > can still happen. In that case just use the empty string as domain. Also, put > the domain before the spec, just in case the spec can contain spaces. That way > we know that the first two spaces are always the field separators. Done. > I would just write it as > ... > And make GetEntry return void. Fixed. > No need for this member IMHO, just use the define right in the code. It's not > likely that this class will be used elsewhere. Done. > >+ nsClassHashtable<nsCStringHashKey, CacheEntry> mTable; > This won't work will it? The CacheEntry objects will be inline allocated by the > hash table which means that they can move around when objects are inserted into > the hash. That will make the pointers in mList either dangle or point to the > wrong entries. No, nsClassHashtables uses nsAutoPtrs. I manually allocate CacheEntry objects in PutEntry. > So the initialization is very inconsistent currently. Fixed, thanks. That was a mess. > And actually, since you're already here. Could you change the GET request to be > an OPTIONS request instead. Hm, did I do that correctly?

Comment on attachment 299797 [details] [diff] [review] Patch, v2 So there's been a new development in the spec. Forgot to mention this last night. The new syntax for the expires header is: Method-Check-Max-Age: 1*DIGIT giving how long until the requires expires in seconds. Mind updating to that? >+ if (NS_SUCCEEDED(rv) && nsXMLHttpRequest::EnsureACCache()) { >+ // Everything worked, check to see if there is an expiration time set >+ // this access control list. If so go ahead and cache it. >+ nsCAutoString expirationString; >+ http->GetResponseHeader(NS_LITERAL_CSTRING("Method-Check-Expires"), >+ expirationString); >+ if (!expirationString.IsEmpty()) { >+ PRTime expirationTime; Move the |&& nsXMLHttpRequest::EnsureACCache()| to the inner |if|. >+nsAccessControlLRUCache::GetEntry(const nsACString& aMethod, >+ nsIURI* aURI, >+ nsIPrincipal* aPrincipal, >+ PRTime* _retval) >+{ >+ NS_ASSERTION(_retval, "Null out param!"); Always set out param to a default value so that it gets set even under error conditions. Otherwise you'll run risk of using uninitialized values in edge cases. And IMO you don't need to even assert for out params. We'll notice since we'll crash right away :) It just adds clutter to the code and there's really no reason it should ever be null as it's generally just the address of a stack variable. >+ // Check to see if this initial GET request has already been cached in our s/GET/OPTIONS/ >+ // special Access Control Cache. >+ PRTime expiration = 0; >+ if (sAccessControlCache) >+ sAccessControlCache->GetEntry(method, uri, mPrincipal, &expiration); Nit: I prefer {} even around one-line if's. It makes for more consistent look and makes future patches simpler. It also avoids the risk that someone forgets to add the {} when adding another line (which has happened). The general mozilla rule on matters such as this is, follow the style of the file. >+ if (expiration < PR_Now()) { Actually, technically this should be <= right? Chances are low that it'll matter :) but you might as well. r=me with that fixed.

Fixed.

(In reply to comment #0) > We're going to use a lightweight LRU cache for this, keyed off of referrer, > http method, and target uri. The specification talks about a list of methods. So that Access-Control:allow <*> method PUT POST DELETE creates a cache entry that is used for any of those methods. Did you implement this per the specification or is this a different model?

It should be possible to test this, excepting PUT/DELETE, now that the body of requests is exposed to the server. PUT/DELETE can be supported once bug 468443 is fixed.

This bug is already being tested by the new cross-site XHR tests.