Summary: | [SOUP] Network Cache: IOChannel operations are not sent to the right thread | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | cdumez, commit-queue, gustavo, koivisto, svillar | ||||
Priority: | P2 | Keywords: | Gtk, Soup | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | 144541 | ||||||
Bug Blocks: | 142821 | ||||||
Attachments: |
|
Description
Carlos Garcia Campos
2015-05-03 04:09:03 PDT
Created attachment 252264 [details]
Patch
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.
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. (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. 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. Committed r183801: <http://trac.webkit.org/changeset/183801> |