Bug 144542 – [SOUP] Network Cache: IOChannel operations are not sent to the right thread

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

We are ignoring the given WorkQueue and running the async operations in the current thread.

[Carlos Garcia Campos]

Created attachment 252264 [details]
Patch

[WebKit Commit Bot]

Attachment 252264 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannel.h:64:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannel.h:65:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannel.h:66:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:130:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:144:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:166:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:196:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:254:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:265:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 9 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 252264 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252264&action=review

Too bad all this code is boilerplate and repeated three times. Seems like instead you could have a helper function and that takes a single lambda to abstract away which of the three IOChannel functions is called.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:162
> +    GMainLoopSource::scheduleAndDeleteOnDestroy("[WebKit] IOChannel::read", std::function<void()>([channel, offset, size, completionHandler] {
> +        channel->read(offset, size, completionHandler);
> +    }), G_PRIORITY_DEFAULT, nullptr, g_main_context_default());

Why is that typecast to std::function needed? Should just work without it.

[Carlos Garcia Campos]

(In reply to comment #3)
> Comment on attachment 252264 [details]
> Patch

Thanks for the review.

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252264&action=review
> 
> Too bad all this code is boilerplate and repeated three times. Seems like
> instead you could have a helper function and that takes a single lambda to
> abstract away which of the three IOChannel functions is called.

Will do it.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:162
> > +    GMainLoopSource::scheduleAndDeleteOnDestroy("[WebKit] IOChannel::read", std::function<void()>([channel, offset, size, completionHandler] {
> > +        channel->read(offset, size, completionHandler);
> > +    }), G_PRIORITY_DEFAULT, nullptr, g_main_context_default());
> 
> Why is that typecast to std::function needed? Should just work without it.

There were some problemas with some compilers, see bug #131006, for example. I thought there were issues with GCC, but it seems it's MSVC, so it doesn't affect us here, I will remove the casts.

[Carlos Garcia Campos]

Comment on attachment 252264 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252264&action=review

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:157
> +    // Using nullptr as queue submits the result to the main context.
> +    if (g_main_context_get_thread_default() == g_main_context_default()) {
> +        read(offset, size, completionHandler);
> +        return;

I've also realized that this is not correct, g_main_context_get_thread_default() is usually NULL in the main thread, and then != g_main_context_default(). Also this particular case is very unlikely to happen, so I think it's safer to always schedule the task to the main context when the queue is nullptr. That also simplifies the code a bit.

[Carlos Garcia Campos]

Committed r183801: <http://trac.webkit.org/changeset/183801>