Bug 183197

Summary: Another crash in SecurityOrigin::shouldTreatAsPotentiallyTrustworthy
Product: WebKit Reporter: Cédric Bellegarde <cedric.bellegarde>
Component: WebKitGTKAssignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, cdumez, cgarcia, cturner, dbates, ews-watchlist, mcatanzaro, mkwst
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=183066
https://bugs.webkit.org/show_bug.cgi?id=184024
https://bugs.webkit.org/show_bug.cgi?id=184468
Attachments:
Description Flags
Backtrace
none
New backtrace
none
Backtrace with demangle off
none
thread apply all bt full
none
Patch
none
Patch none

Description Cédric Bellegarde 2018-02-27 22:34:36 PST
Created attachment 334729 [details]
Backtrace

Backtrace as attachment. Happened while browsing webkitgtk.org!
Comment 1 Cédric Bellegarde 2018-02-27 22:35:19 PST
Version: 2.19.91
Comment 2 Cédric Bellegarde 2018-03-06 09:16:48 PST
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.
Comment 3 Michael Catanzaro 2018-03-06 11:20:00 PST
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.
Comment 4 Cédric Bellegarde 2018-03-06 23:48:48 PST
What to I need to pass to cmake?
Comment 5 Carlos Garcia Campos 2018-03-06 23:53:38 PST
This might be fixed by r228972, which was merged in 2.19.92, could you try with the new release, please?
Comment 6 Cédric Bellegarde 2018-03-07 00:06:29 PST
Ok, build launched ;)
Comment 7 Michael Catanzaro 2018-03-07 09:42:44 PST
(In reply to Cédric Bellegarde from comment #4)
> What to I need to pass to cmake?

For future reference: -DCMAKE_BUILD_TYPE=Debug
Comment 8 Cédric Bellegarde 2018-03-09 04:46:07 PST
Does not happen with 2.19.92
Comment 9 Cédric Bellegarde 2018-03-13 03:39:55 PDT
Just happened with 2.20.

Will build in debug mode and post backtrace here.
Comment 10 Cédric Bellegarde 2018-03-15 11:24:09 PDT
Just happened on Fedora, building a package in Debug mode now:
https://copr.fedorainfracloud.org/coprs/gnumdk/gnumdk/build/728357/
Comment 11 Cédric Bellegarde 2018-03-15 11:43:26 PDT
Seems Copr is unable to build webkitgtk in Debug mode, any idea?
Comment 12 Michael Catanzaro 2018-03-15 17:33:55 PDT
(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....
Comment 13 Michael Catanzaro 2018-03-15 17:41:23 PDT
(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.
Comment 14 Cédric Bellegarde 2018-03-16 02:46:47 PDT
Created attachment 335925 [details]
New backtrace

Fedora 28 with WebKitGTK 2.20
Comment 15 Cédric Bellegarde 2018-03-16 03:05:11 PDT
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,
Comment 16 Charlie Turner 2018-03-16 03:31:37 PDT
(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.
Comment 17 Cédric Bellegarde 2018-03-16 03:40:25 PDT
Created attachment 335927 [details]
Backtrace with demangle off
Comment 18 Michael Catanzaro 2018-03-16 09:45:36 PDT
It might be helpful to use 'thread apply all bt full', because this is a thread-safety issue
Comment 19 Chris Dumez 2018-03-16 09:48:34 PDT
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.
Comment 20 Chris Dumez 2018-03-16 09:49:42 PDT
(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()..
Comment 21 Michael Catanzaro 2018-03-16 09:54:08 PDT
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.
Comment 22 Chris Dumez 2018-03-16 09:57:22 PDT
(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().
Comment 23 Michael Catanzaro 2018-03-16 10:04:40 PDT
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.
Comment 24 Chris Dumez 2018-03-16 10:08:55 PDT
(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.
Comment 25 Michael Catanzaro 2018-03-16 12:55:34 PDT
(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();
Comment 26 Chris Dumez 2018-03-16 12:56:38 PDT
(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.
Comment 27 Michael Catanzaro 2018-03-16 13:21:35 PDT
Indeed... today I learned how the assignment operator works....
Comment 28 Cédric Bellegarde 2018-03-17 04:19:44 PDT
Created attachment 336006 [details]
thread apply all bt full
Comment 29 Michael Catanzaro 2018-03-17 10:56:41 PDT
(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.
Comment 30 Michael Catanzaro 2018-03-23 11:30:58 PDT
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?
Comment 31 Michael Catanzaro 2018-03-23 11:32:15 PDT
Created attachment 336396 [details]
Patch
Comment 32 EWS Watchlist 2018-03-23 11:34:26 PDT
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.
Comment 33 Michael Catanzaro 2018-03-23 11:42:49 PDT
(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.
Comment 34 Chris Dumez 2018-03-23 11:50:07 PDT
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.
Comment 35 Michael Catanzaro 2018-03-23 12:20:21 PDT
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.
Comment 36 Chris Dumez 2018-03-23 12:21:16 PDT
(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.
Comment 37 Michael Catanzaro 2018-03-23 17:19:03 PDT
(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...
Comment 38 Michael Catanzaro 2018-03-23 17:20:19 PDT
(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....
Comment 39 Michael Catanzaro 2018-03-23 18:08:50 PDT
(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?
Comment 40 Michael Catanzaro 2018-03-23 18:14:31 PDT
(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....
Comment 41 Michael Catanzaro 2018-03-23 18:18:39 PDT
Created attachment 336450 [details]
Patch
Comment 42 Chris Dumez 2018-03-26 14:10:07 PDT
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.
Comment 43 Michael Catanzaro 2018-03-26 15:50:14 PDT
(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!
Comment 44 Michael Catanzaro 2018-03-27 18:42:01 PDT
Oops, looks like it's not fully solved yet.
Comment 45 Michael Catanzaro 2018-04-05 12:02:51 PDT
Probably fixed by r230205