[webkit-reviews] review denied: [Bug 22443] XMLHttpRequest's PreflightResultCache should be thread-safe. : [Attachment 25407] The make the XMLHttpRequest PreflightResultCache thread-safe.

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied David Levin
<levin at chromium.org>'s request for review:
Bug 22443: XMLHttpRequest's PreflightResultCache should be thread-safe.
https://bugs.webkit.org/show_bug.cgi?id=22443

Attachment 25407: The make the XMLHttpRequest PreflightResultCache thread-safe.
https://bugs.webkit.org/attachment.cgi?id=25407&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
> 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.