Summary: | Another crash in SecurityOrigin::shouldTreatAsPotentiallyTrustworthy | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Cédric Bellegarde <cedric.bellegarde> | ||||||||||||||
Component: | WebKitGTK | Assignee: | 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: |
|
Version: 2.19.91 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. 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. What to I need to pass to cmake? This might be fixed by r228972, which was merged in 2.19.92, could you try with the new release, please? Ok, build launched ;) (In reply to Cédric Bellegarde from comment #4) > What to I need to pass to cmake? For future reference: -DCMAKE_BUILD_TYPE=Debug Does not happen with 2.19.92 Just happened with 2.20. Will build in debug mode and post backtrace here. Just happened on Fedora, building a package in Debug mode now: https://copr.fedorainfracloud.org/coprs/gnumdk/gnumdk/build/728357/ Seems Copr is unable to build webkitgtk in Debug mode, any idea? (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.... (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. Created attachment 335925 [details]
New backtrace
Fedora 28 with WebKitGTK 2.20
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, (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. Created attachment 335927 [details]
Backtrace with demangle off
It might be helpful to use 'thread apply all bt full', because this is a thread-safety issue 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. (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().. 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. (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(). 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. (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. (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(); (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. Indeed... today I learned how the assignment operator works.... Created attachment 336006 [details]
thread apply all bt full
(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. 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? Created attachment 336396 [details]
Patch
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.
(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 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.
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. (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. (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... (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.... (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? (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.... Created attachment 336450 [details]
Patch
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. (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! Oops, looks like it's not fully solved yet. Probably fixed by r230205 |
Created attachment 334729 [details] Backtrace Backtrace as attachment. Happened while browsing webkitgtk.org!