Bug 22443 – XMLHttpRequest's PreflightResultCache should be thread-safe.

• Status: RESOLVED
• Resolution: FIXED
• Component: XML

[David Levin]

In order to make XMLHttpRequest available to Workers, it needs to stop using non-threadsafe globals to work.  PreflghtResultCache is one of those globals.

[David Levin]

Created attachment 25406 [details]
This makes PreflightResultCache thread-safe.

[David Levin]

Created attachment 25407 [details]
The make the XMLHttpRequest PreflightResultCache thread-safe.

[Alexey Proskuryakov]

Comment on attachment 25407 [details]
The make the XMLHttpRequest PreflightResultCache thread-safe.

> Index: WebCore/ChangeLog
> +	Made the PreflightResultCache thread-safe in preparation for usage of XMLHttpRequest by
> +	Workers on threads.

Please replace tabs with spaces here and elsewhere in ChangeLog.

> +         WARNING: NO TEST CASES ADDED OR CHANGED

This line is just a reminder for the author - you can either delete it, or add an explanation of why there are no tests (in this case, there is no observable change in behavior).

>          Since the event loop is not ever entered again in this case, the fix necessarily involves
> -        some shared data hackery.
> +        Some shared data hackery.

Why did you make this change?

> +static bool isMethodAllowed(const HashSet<String>& methods_allowed, const String& method);
> +static bool areHeadersAllowed(const HeadersSet& headers_allowed, const HTTPHeaderMap& requestHeaders);

I don't think that hiding the behavior in functions with such generic names helps readability. There are several concepts of valid/safe/allowed headers and requests for XMLHttpRequest, which can be confusing. We'll need to find more specific names, or just keep the logic at call sites (I'd prefer the former, but I don't have a suggestion for better names).

> +    static void appendEntry(String origin, KURL url, unsigned expiryDelta, 

This comes from existing code, but why aren't these const references?

I think that code would look better if PreflightResultCache methods were not static.

> +        cache.m_preflightHashMap.set(std::make_pair(origin, url), item);

Map elements are used from multiple threads, but String refcounting is not thread safe. You need to use deep copy: std::make_pair(origin.copy, KURL(url.string().copy())), String copy constructor doesn't achieve this effect. Now may be a good opportunity to add a deep copy method to KURL to avoid the ugly and slow conversion via String.

> +        return cache.canSkipPreflightImpl(origin, url, includeCredentials, method, requestHeaders);

You wouldn't need a separate Impl function if PreflightResultCache methods were not static. At call sites, they can be called e.g. as PreflightResultCache::instance().addEntry(...).

Also, you've put method bodies in class definition, which implicitly makes them inline. It doesn't look like these methods should be inline, so it would be better to move their bodies out.

> +          Locker<Mutex> lock(cache.m_mutex);

We have a MutexLocker typedef for Locker<Mutex>.

> +         PreflightResultCacheItem(unsigned expiryDelta, bool credentials, HashSet<String>* methods, HeadersSet* headers)
> +             : m_absoluteExpiryTime(currentTime() + expiryDelta)
> +             , m_credentials(credentials)
> +             , m_methods(methods)
> +             , m_headers(headers)

All Strings should be deep copied in PreflightResultCacheItem constructor, too.

> +         OwnPtr<HeadersSet > m_headers;

Unneeded extra space after HeadersSet.

> +     static bool doesCacheAllowRequest(const PreflightResultCacheItem& item, bool includeCredentials, const String& method, const HTTPHeaderMap& requestHeaders)

The name is misleading - this method doesn't check if the cache allows the request, it just checks a single cache item. Also, it doesn't sound like correct English grammar - you don't say "if does cache allow request".

r- for missing deep copy calls.

[David Levin]

Created attachment 25464 [details]
Make the PreflightResultCache thread-safe.

[David Levin]

(In reply to comment #4)
> Created an attachment (id=25464) [review]
> Make the PreflightResultCache thread-safe.
>

I've addressed all comments.

[David Levin]

(In reply to comment #5)
> (In reply to comment #4)
> > Created an attachment (id=25464) [review] [review]
> > Make the PreflightResultCache thread-safe.
> >
> 
> I've addressed all comments.
> 

(In reply to comment #5)
> (In reply to comment #4)
> > Created an attachment (id=25464) [review] [review]
> > Make the PreflightResultCache thread-safe.
> >
> 
> I've addressed all comments.
> 
Please don't review yet as I'm making a few last changes.

[David Levin]

Created attachment 25471 [details]
Make the PreflightResultCache thread-safe.

Ok, now it is ready for review.  All comments have been addressed.

[David Levin]

Created attachment 25473 [details]
Make the PreflightResultCache thread-safe.

Removed the change in KURl that added a deep copy since this was implemented in r38746.

[Alexey Proskuryakov]

Comment on attachment 25464 [details]
Make the PreflightResultCache thread-safe.

Marking obsolete patches as such.

[Alexey Proskuryakov]

Comment on attachment 25473 [details]
Make the PreflightResultCache thread-safe.

-    PassRefPtr<StringImpl> copy();
+    PassRefPtr<StringImpl> copy(unsigned pos = 0, unsigned len = UINT_MAX);

I'm not convinced this is a good change. Constructing a String from a substring makes a copy anyway, so it's jst confusing that we have another way to achieve the same result.

On the other hand, this could help if we ever decided to make Strings share buffers like UStrings do, but it would be a tough discipline to always use copy() just for this, even where it's not currently needed.

+    bool ParseResponse(const ResourceResponse& response);

Method names should start with lower case letter.

Also, we omit parameter names from declarations where they do not provide any additional information. This is an issue in many declarations below, too.

+    bool isCrossSiteMethodAllowedByItem(const String& method);

Making these PreflightResultCacheItem methods was a great idea, it really helps! But this reads as if the method parameter were an item. I suggest calling this "allowsCrossSiteMethod".

+    bool areCrossSiteHeadersAllowedByItem(const HTTPHeaderMap& requestHeaders);
+    bool isRequestAllowedByItem(bool includeCredentials, const String& method, const HTTPHeaderMap& requestHeaders);

Same concern - what about "allowsCrossSiteHeaders" and "allowsRequest"?

+bool PreflightResultCacheItem::parseAccessControlAllowList(const String& string, HashSet<String, HashType>* set)
+{
+    ASSERT(set);

Now that m_methods is not an OwnPtr, I think you could change the method to take a reference.

These are extremely minor nitpicks, and the patch is very close to r+, but we need one more quick round, at least for ParseResponse.

[Alexey Proskuryakov]

> +    PassRefPtr<StringImpl> copy(unsigned pos = 0, unsigned len = UINT_MAX);

Maciej suggested on IRC that we could add a substringCopy() method, as a call to copy() with arguments wouldn't be obvious in meaning. The function would be an inline wrapper around substring(), with a comment explaining its purpose.

[David Levin]

Created attachment 25484 [details]
Make the PreflightResultCache thread-safe.

Addressed all comments as suggested.

[David Levin]

Created attachment 25485 [details]
Make the PreflightResultCache thread-safe.

Updated the ChangeLog due to change in functions modified (substringCopy).

[Alexey Proskuryakov]

Comment on attachment 25485 [details]
Make the PreflightResultCache thread-safe.

> [phone]  David Levin  <set EMAIL_ADDRESS environment variable>

Please do. Also, please reference this bug in ChangeLog, so that anyone reading it could easily find the relevant discussions.

> +    bool parse(const ResourceResponse& response);

The argument name should be omitted, because it doesn't add anything that its type doesn't say (same elsewhere).

> +class PreflightResultCache : public Noncopyable {

We currently use private inheritance for Noncopyable, although it doesn't really matter.

r=me. I'll address my own comments while landing this patch.

[Alexey Proskuryakov]

Committed revision 38756.