[Webkit-unassigned] [Bug 22443] XMLHttpRequest's PreflightResultCache should be thread-safe.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=22443


ap at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #25407|review?(ap at webkit.org)      |review-
               Flag|                            |




------- Comment #3 from ap at webkit.org  2008-11-24 00:09 PDT -------
(From update of attachment 25407)
> 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.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.