Bug 183197 – Another crash in SecurityOrigin::shouldTreatAsPotentiallyTrustworthy

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Cédric Bellegarde]

Created attachment 334729 [details]
Backtrace

Backtrace as attachment. Happened while browsing webkitgtk.org!

[Cédric Bellegarde]

Version: 2.19.91

[Cédric Bellegarde]

WebKitGTK is crashing 3 or 4 times a day due to this one.

Not happening on another computer with 2.18.6.

So seems related to current beta.

[Michael Catanzaro]

I see you're building WebKit manually. Since it's happening a lot, any chance you could try to catching this one in a debug build? Having a backtrace without optimization might help here.

[Cédric Bellegarde]

What to I need to pass to cmake?

[Carlos Garcia Campos]

This might be fixed by r228972, which was merged in 2.19.92, could you try with the new release, please?

[Cédric Bellegarde]

Ok, build launched ;)

[Michael Catanzaro]

(In reply to Cédric Bellegarde from comment #4)
> What to I need to pass to cmake?

For future reference: -DCMAKE_BUILD_TYPE=Debug

[Cédric Bellegarde]

Does not happen with 2.19.92

[Cédric Bellegarde]

Just happened with 2.20.

Will build in debug mode and post backtrace here.

[Cédric Bellegarde]

Just happened on Fedora, building a package in Debug mode now:
https://copr.fedorainfracloud.org/coprs/gnumdk/gnumdk/build/728357/

[Cédric Bellegarde]

Seems Copr is unable to build webkitgtk in Debug mode, any idea?

[Michael Catanzaro]

(In reply to Cédric Bellegarde from comment #11)
> Seems Copr is unable to build webkitgtk in Debug mode, any idea?

It's a GCC crash, so I would report it to the GCC developers. Maybe try using Fedora 27 which has GCC 7 instead of GCC 8, or building it yourself.

I'm surprised we haven't gotten other reports of this, but I guess they'll start coming in soon enough....

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #5)
> This might be fixed by r228972, which was merged in 2.19.92, could you try
> with the new release, please?

I just independently rediscovered this... Cedric, an updated backtrace, even with optimization, would be useful. The issue in the backtrace you posted first should, in theory, be fixed by Chris in r228972, which was not present in 2.19.91. I assume a newer backtrace will be slightly different.

CC Chris since he worked on this last.

[Cédric Bellegarde]

Created attachment 335925 [details]
New backtrace

Fedora 28 with WebKitGTK 2.20

[Cédric Bellegarde]

Ok, previous backtrace was not useful. Finnaly get 2.20 symbols but gdb segfaults and fails to log to file, here what I get:

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x00007fa2081e1d14 in WTF::equalIgnoringASCIICaseCommon<WTF::StringImpl, WTF::StringImpl> (a=..., b=...)
    at /usr/src/debug/webkit2gtk3-2.20.0-2.fc28.x86_64/x86_64-redhat-linux-gnu/DerivedSources/ForwardingHeaders/wtf/text/StringCommon.h:360
360	bool equalIgnoringASCIICaseCommon(const StringClassA& a, const StringClassB& b)
[Current thread is 1 (Thread 0x7fa225211540 (LWP 11417))]
Missing separate debuginfos, use: dnf debuginfo-install python3-3.6.4-17.fc28.x86_64
(gdb) set logging file plop.txt
(gdb) set logging on
Copying output to plop.txt.
(gdb) bt full
#0  0x00007fa2081e1d14 in WTF::equalIgnoringASCIICaseCommon<WTF::StringImpl, WTF::StringImpl>(WTF::StringImpl const&, WTF::StringImpl const&) (a=..., b=...)
    at /usr/src/debug/webkit2gtk3-2.20.0-2.fc28.x86_64/x86_64-redhat-linux-gnu/DerivedSources/ForwardingHeaders/wtf/text/StringCommon.h:360
        length = <optimized out>
#1  0x00007fa209169044 in WTF::equalIgnoringASCIICase(WTF::StringImpl const&, WTF::StringImpl const&) (b=..., a=...)
    at /usr/src/debug/webkit2gtk3-2.20.0-2.fc28.x86_64/x86_64-redhat-linux-gnu/DerivedSources/ForwardingHeaders/wtf/text/StringImpl.h:1187
        entry = 0x7fa1fd5e00a8
        k = 0
        i = 5
        sizeMask = 7
        table = 0x7fa1fd5e0080
        h = 370
#2  0x00007fa209169044 in WTF::ASCIICaseInsensitiveHash::equal(WTF::StringImpl const&, WTF::StringImpl const&) (b=..., a=...)
    at /usr/src/debug/webkit2gtk3-2.20.0-2.fc28.x86_64/x86_64-redhat-linux-gnu/DerivedSources/ForwardingHeaders/wtf/text/StringHash.h:120
        entry = 0x7fa1fd5e00a8
        k = 0
        i = 5
        sizeMask = 7
        table = 0x7fa1fd5e0080
        h = 370
#3  0x00007fa209169044 in WTF::ASCIICaseInsensitiveHash::equal(WTF::StringImpl const*, WTF::StringImpl const*) (b=0x7f8c004bb0e0, a=0x1)
    at /usr/src/debug/webkit2gtk3-2.20.0-2.fc28.x86_64/x86_64-redhat-linux-gnu/DerivedSources/ForwardingHeaders/wtf/text/StringHash.h:126
        entry = 0x7fa1fd5e00a8
        k = 0
        i = 5
        sizeMask = 7
        table = 0x7fa1fd5e0080
        h = 370
#4  0x00007fa209169044 in WTF::ASCIICaseInsensitiveHash::equal(WTF::String const&, WTF::String const&) (b=..., a=...)
---Type <return> to continue, or q <return> to quit---
    at /usr/src/debug/webkit2gtk3-2.20.0-2.fc28.x86_64/x86_64-redhat-linux-gnu/DerivedSources/ForwardingHeaders/wtf/text/StringHash.h:149
        entry = 0x7fa1fd5e00a8
        k = 0
        i = 5
        sizeMask = 7
        table = 0x7fa1fd5e0080
        h = 370
#5  0x00007fa209169044 in WTF::IdentityHashTranslator<WTF::HashTraits<WTF::String>, WTF::ASCIICaseInsensitiveHash>::equal<WTF::String, WTF::String>(WTF::String const&, WTF::String const&) (b=..., a=...)
    at /usr/src/debug/webkit2gtk3-2.20.0-2.fc28.x86_64/x86_64-redhat-linux-gnu/DerivedSources/ForwardingHeaders/wtf/HashTable.h:284
        entry = 0x7fa1fd5e00a8
        k = 0
        i = 5
        sizeMask = 7
        table = 0x7fa1fd5e0080
        h = 370
#6  0x00007fa209169044 in WTF::HashTable<WTF::String, WTF::String, WTF::IdentityExtractor, WTF::ASCIICaseInsensitiveHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String> >::inlineLookup<WTF::IdentityHashTranslator<WTF::HashTraits<WTF::String>, WTF::ASCIICaseInsensitiveHash>, WTF::String>(WTF::String const&) (key=..., this=<optimized out>)
    at /usr/src/debug/webkit2gtk3-2.20.0-2.fc28.x86_64/x86_64-redhat-linux-gnu/DerivedSources/ForwardingHeaders/wtf/HashTable.h:642
        entry = 0x7fa1fd5e00a8
        k = 0
        i = 5
        sizeMask = 7
        table = 0x7fa1fd5e0080
        h = 370
#7  0x00007fa209169044 in WTF::HashTable<WTF::String, WTF::String, WTF::IdentityExtractor, WTF::ASCIICaseInsensitiveHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String> >::lookup<WTF::IdentityHashTranslator<WTF::HashTraits<WTF::String>, WTF::ASCIICaseInsensitiveHash>, WTF::String>(WTF::String const&) (this=<optimized out>, key=...)
    at /usr/src/debug/webkit2gtk3-2.20.0-2.fc28.x86_64/x86_64-redhat-linux-gnu/DerivedSources/ForwardingHeaders/wtf/HashTable.h:601
---Type <return> to continue, or q <return> to quit---
#8  0x00007fa2092f8392 in WTF::HashTable<WTF::String, WTF::String, WTF::IdentityExtractor, WTF::ASCIICaseInsensitiveHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String> >::contains<WTF::IdentityHashTranslator<WTF::HashTraits<WTF::String>, WTF::ASCIICaseInsensitiveHash>, WTF::String>(WTF::String const&) const (key=..., this=0x172)
    at /usr/src/debug/webkit2gtk3-2.20.0-2.fc28.x86_64/Source/WebCore/platform/SchemeRegistry.cpp:296
        locker = 
          {<WTF::AbstractLocker> = {<No data fields>}, m_lockable = 0x7fa20a5b3df0 <WebCore::localURLSchemesLock()::lock>}
#9  0x00007fa2092f8392 in WTF::HashTable<WTF::String, WTF::String, WTF::IdentityExtractor, WTF::ASCIICaseInsensitiveHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String> >::contains(WTF::String const&) const (key=..., this=0x172)
    at /usr/src/debug/webkit2gtk3-2.20.0-2.fc28.x86_64/x86_64-redhat-linux-gnu/DerivedSources/ForwardingHeaders/wtf/HashTable.h:397
        locker = 
          {<WTF::AbstractLocker> = {<No data fields>}, m_lockable = 0x7fa20a5b3df0 <WebCore::localURLSchemesLock()::lock>}
#10 0x00007fa2092f8392 in WTF::HashSet<WTF::String, WTF::ASCIICaseInsensitiveHash, WTF::HashTraits<WTF::String> >::contains(WTF::String const&) const (value=..., this=0x172)
    at /usr/src/debug/webkit2gtk3-2.20.0-2.fc28.x86_64/x86_64-redhat-linux-gnu/DerivedSources/ForwardingHeaders/wtf/HashSet.h:202
        locker = 
          {<WTF::AbstractLocker> = {<No data fields>}, m_lockable = 0x7fa20a5b3df0 <WebCore::localURLSchemesLock()::lock>}
#11 0x00007fa2092f8392 in WebCore::SchemeRegistry::shouldTreatURLSchemeAsLocal(WTF::String const&) (scheme=...)
    at /usr/src/debug/webkit2gtk3-2.20.0-2.fc28.x86_64/Source/WebCore/platform/SchemeRegistry.cpp:296
        locker = 
          {<WTF::AbstractLocker> = {<No data fields>}, m_lockable = 0x7fa20a5b3df0 <WebCore::localURLSchemesLock()::lock>}
#12 0x00007fa20928befd in WebCore::SecurityOrigin::isLocal() const (this=this@entry=0x7fa1fd5ddd90)
    at /usr/src/debug/webkit2gtk3-2.20.0-2.fc28.x86_64/Source/WebCore/page/SecurityOrigin.cpp:478
#13 0x00007fa20928c166 in WebCore::SecurityOrigin::SecurityOrigin(WebCore::URL const&) (this=0x7fa1fd5ddd90, url=...)
---Type <return> to continue, or q <return> to quit---
    at /usr/src/debug/webkit2gtk3-2.20.0-2.fc28.x86_64/Source/WebCore/page/SecurityOrigin.cpp:159
#14 0x00007fa20928c5e0 in WebCore::SecurityOrigin::create(WebCore::URL const&) (url=...)
    at /usr/src/debug/webkit2gtk3-2.20.0-2.fc28.x86_64/x86_64-redhat-linux-gnu/DerivedSources/ForwardingHeaders/wtf/ThreadSafeRefCounted.h:36
#15 0x00007fa2092918d5 in WebCore::SecurityOrigin::create(WTF::String const&, WTF::String const&, std::optional<unsigned short>) (protocol=..., host=..., port=Python Exception <class 'gdb.error'> There is no member or method named _M_payload.: 
...) at /usr/src/debug/webkit2gtk3-2.20.0-2.fc28.x86_64/Source/WebCore/platform/URL.h:58
        decodedHost = 
            {m_impl = {static isRefPtr = <optimized out>, m_ptr = 0x7f8c004ba910}}
        origin = {static isRef = <optimized out>, m_ptr = 0x0}
#16 0x00007fa209291cf6 in WebCore::SecurityOriginData::securityOrigin() const (this=0x7ffee2d04a78) at /usr/include/c++/8/new:169
#17 0x00007fa2082b3856 in WebKit::WebPageProxy::decidePolicyForNavigationAction(unsigned long, WebCore::SecurityOriginData const&, unsigned long, WebKit::NavigationActionData&&, WebKit::FrameInfoData const&, unsigned long, WebCore::ResourceRequest const&, WebCore::ResourceRequest&&, unsigned long, WebKit::UserData const&, bool&, unsigned long&, WebCore::PolicyAction&, WebKit::DownloadID&, std::optional<WebKit::WebsitePoliciesData>&) (this=0x7fa1b4ded900, frameID=<optimized out>, frameSecurityOrigin=..., navigationID=<optimized out>, navigationActionData=..., originatingFrameInfoData=..., originatingPageID=4, originalRequest=..., request=..., listenerID=15, userData=..., receivedPolicyAction=@0x7ffee2d04708: false, newNavigationID=@0x7ffee2d04700: 8, policyAction=@0x7ffee2d046f8: WebCore::PolicyAction::Use, downloadID=..., websitePolicies=Python Exception <class 'gdb.error'> There is no member or method named _M_payload.: 
...)
    at /usr/src/debug/webkit2gtk3-2.20.0-2.fc28.x86_64/Source/WebKit/UIProcess/WebPageProxy.cpp:3768
        protector = {m_pageClient = @0x55ad5d7771b0}
        transaction = 
          {m_webPageProxy = {static isRefPtr = <optimized out>, m_ptr = 0x7fa1b4ded900}, m_pageLoadState = 0x7fa1b4dedf48}
        fromAPI = false
        frame = 0x7fa1fd59a330
        listener = {static isRef = <optimized out>, m_ptr = 0x7fa1426c2938}
        originatingFrame = 0x7fa1fd59a330
#18 0x00007fa2084902f9 in IPC::callMemberFunctionImpl<WebKit::WebPageProxy, void (WebKit::---Type <return> to continue, or q <return> to quit---
WebPageProxy::*)(unsigned long, WebCore::SecurityOriginData const&, unsigned long, WebKit::NavigationActionData&&, WebKit::FrameInfoData const&, unsigned long, WebCore::ResourceRequest const&, WebCore::ResourceRequest&&, unsigned long, WebKit::UserData const&, bool&, unsigned long&, WebCore::PolicyAction&, WebKit::DownloadID&, std::optional<WebKit::WebsitePoliciesData>&), std::tuple<unsigned long, WebCore::SecurityOriginData, unsigned long, WebKit::NavigationActionData, WebKit::FrameInfoData, unsigned long, WebCore::ResourceRequest, WebCore::ResourceRequest, unsigned long, WebKit::UserData>, 0ul, 1ul, 2ul, 3ul, 4ul, 5ul, 6ul, 7ul, 8ul, 9ul, std::tuple<bool, unsigned long, WebCore::PolicyAction, WebKit::DownloadID, std::optional<WebKit::WebsitePoliciesData> >, 0ul, 1ul, 2ul, 3ul, 4ul>(WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)(unsigned long, WebCore::SecurityOriginData const&, unsigned long, WebKit::NavigationActionData&&, WebKit::FrameInfoData const&, unsigned long, WebCore::ResourceRequest const&, WebCore::ResourceRequest&&, unsigned long, WebKit::UserData const&, bool&, unsigned long&, WebCore::PolicyAction&, WebKit::DownloadID&, std::optional<WebKit::WebsitePoliciesData>&), std::tuple<unsigned long, WebCore::SecurityOriginData, unsigned long, WebKit::NavigationActionData, WebKit::FrameInfoData, unsigned long, WebCore::ResourceRequest, WebCore::ResourceRequest, unsigned long, WebKit::UserData>&&, std::tuple<bool, unsigned long, WebCore::PolicyAction, WebKit::DownloadID, std::optional<WebKit::WebsitePoliciesData> >&, std::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul, 4ul, 5ul, 6ul, 7ul, 8ul, 9ul>, std::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul, 4ul>) (replyArgs=std::tuple containing = {...}, args=..., function=<optimized out>, object=0x7fa1b4ded900)
    at /usr/src/debug/webkit2gtk3-2.20.0-2.fc28.x86_64/x86_64-redhat-linux-gnu/DerivedSources/ForwardingHeaders/wtf/text/StringImpl.h:1058
        arguments = 
../../gdb/dictionary.c:690: internal-error: void insert_symbol_hashed(dictionary*, symbol*): Assertion `SYMBOL_LANGUAGE (sym) == DICT_LANGUAGE (dict)->la_language' failed.
A problem internal to GDB has been detected,

[Charlie Turner]

(In reply to Cédric Bellegarde from comment #15)
> Ok, previous backtrace was not useful. Finnaly get 2.20 symbols but gdb
> segfaults and fails to log to file, here what I get:

Sigh, yet more GDB sadness. I've not seen that particular flavor of error, but can you try turning off the demangler?

set demangle-style none

Before running the bt commands.

[Cédric Bellegarde]

Created attachment 335927 [details]
Backtrace with demangle off

[Michael Catanzaro]

It might be helpful to use 'thread apply all bt full', because this is a thread-safety issue

[Chris Dumez]

The issue is this:
m_canLoadLocalResources = isLocal();

in the SecurityOrigin constructor. isLocal() queries a global static map but SecurityOrigin objects get constructed on various threads.

[Chris Dumez]

(In reply to Chris Dumez from comment #19)
> The issue is this:
> m_canLoadLocalResources = isLocal();
> 
> in the SecurityOrigin constructor. isLocal() queries a global static map but
> SecurityOrigin objects get constructed on various threads.

Although it looks like we have locks in SchemeRegistry::shouldTreatURLSchemeAsLocal()..

[Michael Catanzaro]

Yeah, there are locks there, so I assume the problem is a missing lock somewhere else. I see is SchemeRegistry::allBuiltinSchemes can be called without a lock. That's my only guess.

[Chris Dumez]

(In reply to Michael Catanzaro from comment #21)
> Yeah, there are locks there, so I assume the problem is a missing lock
> somewhere else. I see is SchemeRegistry::allBuiltinSchemes can be called
> without a lock. That's my only guess.

It does look like builtinLocalURLSchemes() can be called from multiple thread and we're not locking from SchemeRegistry::allBuiltinSchemes().

[Michael Catanzaro]

Looks like allBuiltinSchemes() needs to lock the secureSchemesLock() as well.

Also, this assert:

    ASSERT(localURLSchemesLock().isHeld());

should probably be added to builtinLocalURLSchemes(). Or we could compress builtinLocalURLSchemes() and localURLSchemes() together... I don't think we need both.

Lastly, it's rather confusing that only about half of this class is threadsafe. This will likely be the source of more bugs in the future if we don't change it. My inclination would be to add locking everywhere.

[Chris Dumez]

(In reply to Michael Catanzaro from comment #23)
> Looks like allBuiltinSchemes() needs to lock the secureSchemesLock() as well.
> 
> Also, this assert:
> 
>     ASSERT(localURLSchemesLock().isHeld());
> 
> should probably be added to builtinLocalURLSchemes(). Or we could compress
> builtinLocalURLSchemes() and localURLSchemes() together... I don't think we
> need both.
> 
> Lastly, it's rather confusing that only about half of this class is
> threadsafe. This will likely be the source of more bugs in the future if we
> don't change it. My inclination would be to add locking everywhere.

I think we need both maps because it is possible to register new local schemes. However, we do not want SchemeRegistry::allBuiltinSchemes() to start returning those newly registered ones.

[Michael Catanzaro]

(In reply to Chris Dumez from comment #24)
> I think we need both maps because it is possible to register new local
> schemes. However, we do not want SchemeRegistry::allBuiltinSchemes() to
> start returning those newly registered ones.

Hmmm... I have not tested, but looks like localURLSchemes() is not making a copy of builtinLocalURLSchemes(), so that might be broken currently?

If this line is a copy, it's not obvious:

    static NeverDestroyed<URLSchemesMap> localSchemes = builtinLocalURLSchemes();

[Chris Dumez]

(In reply to Michael Catanzaro from comment #25)
> (In reply to Chris Dumez from comment #24)
> > I think we need both maps because it is possible to register new local
> > schemes. However, we do not want SchemeRegistry::allBuiltinSchemes() to
> > start returning those newly registered ones.
> 
> Hmmm... I have not tested, but looks like localURLSchemes() is not making a
> copy of builtinLocalURLSchemes(), so that might be broken currently?
> 
> If this line is a copy, it's not obvious:
> 
>     static NeverDestroyed<URLSchemesMap> localSchemes =
> builtinLocalURLSchemes();

Looks like a copy to me.

[Michael Catanzaro]

Indeed... today I learned how the assignment operator works....

[Cédric Bellegarde]

Created attachment 336006 [details]
thread apply all bt full

[Michael Catanzaro]

(In reply to Cédric Bellegarde from comment #28)
> Created attachment 336006 [details]
> thread apply all bt full

It's useless, due to the gdb crash. :(

It's OK... I think we have a good idea what still needs to be tightened up here.

[Michael Catanzaro]

I went through and added locking all throughout SchemeRegistry.cpp. I'm going to upload a patch, but without r? because it is not good enough.

Problem #1 is SchemeRegistry::isBuiltinScheme, which calls URLParser::isSpecialScheme. After examining the implementation of that function, I think it's actually currently safe, but that's too fragile: it could change in the future, and threadsafe functions should not be calling class static functions, as a rule, unless they're also intended to be threadsafe. Adding locks in SchemeRegistry is fairly easy, but I don't think we want to do so in URLParser, and I'm not sure how to handle this well.

Next problem is that I think our hypothesis in comment #22 is not the only problem here. There is this FIXME you added in SecurityOrigin::shouldTreatAsPotentiallyTrustworthy:

    // FIXME: despite the following SchemeRegistry functions using locks internally, we still
    // have a potential thread-safety issue with the strings being passed in. This is because
    // String::hash() will be called during lookup and it potentially modifies the String for
    // caching the hash.

Looking at the backtrace, I'm fairly confident that is, in fact, the crash we are seeing here. Should we change all of SchemeRegistry to use AtomicString instead of normal String?

[Michael Catanzaro]

Created attachment 336396 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files


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

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #30)
> Looking at the backtrace, I'm fairly confident that is, in fact, the crash
> we are seeing here. Should we change all of SchemeRegistry to use
> AtomicString instead of normal String?

Nope:

bool String::isSafeToSendToAnotherThread() const
{
    // AtomicStrings are not safe to send between threads as ~StringImpl()
    // will try to remove them from the wrong AtomicStringTable.
    return isEmpty() || (m_impl->hasOneRef() && !m_impl->isAtomic());
}

There is String::isolatedCopy(), though. We could ensure that all user-provided strings get copied with that before adding them to the SchemeRegistry's URLSchemesMaps, localURLSchemes() and secureSchemes(). We should probably also use isolatedCopy() before adding them to the Vectors as well, to be properly thorough, or we could assume that Vector will never mutate its contained Strings.

[Chris Dumez]

Comment on attachment 336396 [details]
Patch

I think this patch is over-eager. I do not think we want to make ALL of these functions thread safe. We had to make 1 or 2 thread safe but there is a performance cost. We do not want to make ALL of them thread safe when it is not needed.

[Michael Catanzaro]

OK, maybe we can move the local schemes and service worker schemes out to a separate class, then? E.g. ThreadSafeSchemeRegistry. You shouldn't have to look at the implementation to figure out which functions are safe and which aren't. Not a big deal: the patch doesn't address the real issue anyway.

[Chris Dumez]

(In reply to Michael Catanzaro from comment #35)
> OK, maybe we can move the local schemes and service worker schemes out to a
> separate class, then? E.g. ThreadSafeSchemeRegistry. You shouldn't have to
> look at the implementation to figure out which functions are safe and which
> aren't. Not a big deal: the patch doesn't address the real issue anyway.

Not sure this is needed. How about comments in the header stating these 2 are thread safe.

[Michael Catanzaro]

(In reply to Chris Dumez from comment #36)
> Not sure this is needed. How about comments in the header stating these 2
> are thread safe.

OK

(In reply to Michael Catanzaro from comment #33)
> There is String::isolatedCopy(), though. We could ensure that all
> user-provided strings get copied with that before adding them to the
> SchemeRegistry's URLSchemesMaps, localURLSchemes() and secureSchemes(). We
> should probably also use isolatedCopy() before adding them to the Vectors as
> well, to be properly thorough, or we could assume that Vector will never
> mutate its contained Strings.

Nope, that's what https://lists.webkit.org/pipermail/webkit-dev/2018-February/029895.html warns against. Hm...

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #37)
> Nope, that's what
> https://lists.webkit.org/pipermail/webkit-dev/2018-February/029895.html
> warns against. Hm...

Nope, your very next mail in the thread explains it's OK. All right then....

[Michael Catanzaro]

(In reply to Chris Dumez from comment #34)
> I think this patch is over-eager. I do not think we want to make ALL of
> these functions thread safe. We had to make 1 or 2 thread safe but there is
> a performance cost. We do not want to make ALL of them thread safe when it
> is not needed.

An updated patch is forthcoming... again, without r? because it's again not good enough. I discovered that SecurityOrigin also uses SchemeRegistry::shouldTreatURLSchemeAsNoAccess, SchemeRegistry::canDisplayOnlyIfCanRequest, SchemeRegistry::shouldTreatURLSchemeAsDisplayIsolated, and SchemeRegistry::shouldPartitionCacheForURLScheme, none of which are currently protected. We've already established that SecurityOrigin objects can be used off the main thread, so either we make all these SchemeRegistry functions threadsafe, or we need to add assertions to ensure several SecurityOrigin functions are only ever used on the main thread.

It's really starting to look like protecting all of SchemeRegistry is a good idea, unless you think it's likely to appear in perf results?

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #39)
> so either we make all these SchemeRegistry
> functions threadsafe, or we need to add assertions to ensure several
> SecurityOrigin functions are only ever used on the main thread.

And the latter seems really awkward to me....

[Michael Catanzaro]

Created attachment 336450 [details]
Patch

[Chris Dumez]

Note that I talked about this with Daniel Bates today and it seems I went in the wrong direction trying to make this thread safe. I am currently working on updating existing code to stop constructing SecurityOrigin objects from background thread.

Once this is done, we can add an ASSERT(isMainThread()) assertion in the SecurityOrigin constructor and drop the locks in the SchemeRegistry.

[Michael Catanzaro]

(In reply to Chris Dumez from comment #42)
> Note that I talked about this with Daniel Bates today and it seems I went in
> the wrong direction trying to make this thread safe. I am currently working
> on updating existing code to stop constructing SecurityOrigin objects from
> background thread.

Yeah, I like your patch in bug #184024 better than what I was trying to do here. Please close this one once you've landed that!

[Michael Catanzaro]

Oops, looks like it's not fully solved yet.

[Michael Catanzaro]

Probably fixed by r230205