r219252 removed WebKitWebSource (WKWS) from being auto-plugged inside the GStreamer pipeline and hence used outside the main thread. There's a significant amount of code in WKWS that is now dead which handled this scenario of running multi-threaded. In particular ResourceHandleStreamingClient isn't used anymore, and there's various code paths conditioned on whether we're in the main thread or not that can be reviewed and probably removed.
Created attachment 315450 [details] Patch Must confess the history of multi-threading WKWS is quite confusing, this seems like a minimally decent change which doesn't regress any tests or manual checks I have. All of the locking is redundant now as well, and it seems a little heavy-handed anyway. I would like to just remove it all as well, but perhaps someone with more context can comment about it.
Comment on attachment 315450 [details] Patch Attachment 315450 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4122852 New failing tests: imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html
Created attachment 315523 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Comment on attachment 315450 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315450&action=review > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:-129 > -}; You can remove StreamingClient too, it was there only to handle both clients the same way, but now, we only have one.
Created attachment 315656 [details] Patch Carlos, do you think we should leave the locking in? It's basically pointless at this stage, aside from as a guide for the future, and I suppose it might have people question how this module is used multi-threaded if they're not familiar with history.
I don't know, I have no idea how threading works in GST, if you say it's useless, then let's remove it.
Comment on attachment 315656 [details] Patch Attachment 315656 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4135328 New failing tests: storage/websql/database-lock-after-reload.html
Created attachment 315659 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
> I don't know, I have no idea how threading works in GST, if you say it's useless, then let's remove it. They're needed to some extent if we are spawned multiple times in the pipeline. Maybe we'll be moving in that direction in the future (netcode going through network process). In that case obviously they are needed. May as well leave them there I suppose, they're not causing any noticeable issues. It's more of a directional question than a technical one.
Comment on attachment 315656 [details] Patch Attachment 315656 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4135559 New failing tests: http/tests/cache/disk-cache/speculative-validation/cacheable-redirect.html
Created attachment 315662 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
No, if the code is not currently needed, then it should be removed. If the code needs to be threadsafe again in the future, it should be resurrected then. We don't keep useless code around in WebKit unless we have imminent plans to use it in the future!
In particular, WebKitWebSrc is already a source of nightmare bugs... simplifying it as much as possible seems desirable.
Created attachment 315666 [details] Patch OK; agreed. Removes the locking code as well.
Comment on attachment 315666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315666&action=review > Source/WebCore/ChangeLog:13 > + Also remove the locking code. It's not needed now, and what existed > + was heavy-handed and missed several cases it was supposed to protect Ah, so it never worked properly anyway. All the more reason to remove it!
Comment on attachment 315666 [details] Patch Attachment 315666 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4136177 New failing tests: http/tests/cache/disk-cache/speculative-validation/cacheable-redirect.html
Created attachment 315674 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
> Ah, so it never worked properly anyway. All the more reason to remove it! Well.. Define worked :) -- I was just mentioning that if this code were to be resurrected, there were some places where a lock should have been taken and it wasn't. In the cases I found it was more recent code, and as the locking stood before this patch, it's easy to forget you're in a critical region. So would need a very careful review of all these places.
This will probably need a rebase before commit-queue, but just wondering if there's any review objections?
You already got r+ from Carlos, so this is good to go. Just rebase the patch, copy his name into the reviewed by line in the ChangeLog, and reupload using 'webkit-patch --no-review --request-commit'. You could have alternatively grabbed the patch using 'webkit-patch apply-from-bug' so that it would come with Carlos's name already there in the reviewed by line, and you wouldn't have to do that manually.
What I mean is: try not to set r? after you already have r+, unless you've made really substantial changes and want another review.
Created attachment 316461 [details] Patch
Comment on attachment 316461 [details] Patch Clearing flags on attachment: 316461 Committed r219959: <http://trac.webkit.org/changeset/219959>
All reviewed patches have been landed. Closing bug.