Bug 238495 – IPC::Connection::connection(UniqueID) makes thread-safe IPC::Connection harder to implement

• Component: WebKit2
• Status: NEW

[Kimmo Kinnunen]

IPC::Connection::connection(UniqueID) makes thread-safe IPC::Connection harder

[Kimmo Kinnunen]

Created attachment 456007 [details]
Patch

[Chris Dumez]

Comment on attachment 456007 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkBroadcastChannelRegistry.cpp:63
> +    auto& connectionIdentifiersForName = channelsForOrigin.ensure(name, [] { return Vector<IPC::Connection*> { }; }).iterator->value; // NOLINT

Curious, what is this NOLINT about? First time I see it. I am not aware that it is a thing in WebKit?

> Source/WebKit/NetworkProcess/NetworkBroadcastChannelRegistry.h:56
> +    using NameToConnectionIdentifiersMap = HashMap<String, Vector<IPC::Connection*>>;

We don't store raw pointers in containers in WebKit anymore because of UAF risks. We may want to use a WeakPtr<>.

[Kimmo Kinnunen]

(In reply to Chris Dumez from comment #2)
> Comment on attachment 456007 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=456007&action=review
> 
> > Source/WebKit/NetworkProcess/NetworkBroadcastChannelRegistry.cpp:63
> > +    auto& connectionIdentifiersForName = channelsForOrigin.ensure(name, [] { return Vector<IPC::Connection*> { }; }).iterator->value; // NOLINT
> 
> Curious, what is this NOLINT about? First time I see it. I am not aware that
> it is a thing in WebKit?

git grep NOLINT Source/WebCore Source/WebKit Source/WTF Source/JavaScriptCore | wc -l 
     104

It's a workaround for case where where webkit style check regular expressions fail.
It is implemented in the webkit style checker.

> > Source/WebKit/NetworkProcess/NetworkBroadcastChannelRegistry.h:56
> > +    using NameToConnectionIdentifiersMap = HashMap<String, Vector<IPC::Connection*>>;
> 
> We don't store raw pointers in containers in WebKit anymore because of UAF
> risks. We may want to use a WeakPtr<>.

WeakPtrs of cross-thread objects are an oxymoron.

[Kimmo Kinnunen]

>> We don't store raw pointers in containers in WebKit anymore because of UAF
>> risks. We may want to use a WeakPtr<>.

> WeakPtrs of cross-thread objects are an oxymoron.

And to elaborate: currently we don't have useful abstraction for holding cross-thread object without a Ref in a thread-safe manner. It is generally a sign of a bug if WeakPtrs are being used for ThreadSafeRefCounted objects.

However, if we don't think that's an issue for today, maybe these objects are only used in main thread, and IPC::Connections are created and destroyed in main thread, so it should work. I can try this if there's no better options and RefPtrs nor raw pointers are appropriate..

[Radar WebKit Bug Importer]

<rdar://problem/91283581>