Bug 174415 – [GStreamer] Review WebKitWebSource after r219252.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Charlie Turner]

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.

[Charlie Turner]

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.

[Build Bot]

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

[Build Bot]

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

[Carlos Garcia Campos]

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.

[Charlie Turner]

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.

[Carlos Garcia Campos]

I don't know, I have no idea how threading works in GST, if you say it's useless, then let's remove it.

[Build Bot]

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

[Build Bot]

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

[Charlie Turner]

> 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.

[Build Bot]

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

[Build Bot]

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

[Michael Catanzaro]

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!

[Michael Catanzaro]

In particular, WebKitWebSrc is already a source of nightmare bugs... simplifying it as much as possible seems desirable.

[Charlie Turner]

Created attachment 315666 [details]
Patch

OK; agreed. Removes the locking code as well.

[Michael Catanzaro]

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!

[Build Bot]

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

[Build Bot]

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

[Charlie Turner]

> 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.

[Charlie Turner]

This will probably need a rebase before commit-queue, but just wondering if there's any review objections?

[Michael Catanzaro]

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.

[Michael Catanzaro]

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.

[Charlie Turner]

Created attachment 316461 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 316461 [details]
Patch

Clearing flags on attachment: 316461

Committed r219959: <http://trac.webkit.org/changeset/219959>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.