Crash under WebCookieCache::clearForHost(): Thread[0] EXC_BAD_ACCESS (SIGSEGV) (KERN_INVALID_ADDRESS at 0xffffffffffffffff) [ 0] 0x00007fff45ec6c2a WebKit`WebKit::WebCookieCache::clearForHost(WTF::String const&) [inlined] WTF::StringImpl::ref() at StringImpl.h:1098:16 0x00007fff45ec6c1f: movq %rax, %r15 0x00007fff45ec6c22: movq (%r14), %rbx 0x00007fff45ec6c25: testq %rbx, %rbx 0x00007fff45ec6c28: je 0x525c2d ; <+143> [inlined] WTF::VectorBufferBase<WTF::String, WTF::FastMalloc>::VectorBufferBase() at Vector.h:383 -> 0x00007fff45ec6c2a: addl $0x2, (%rbx) 0x00007fff45ec6c2d: xorps %xmm0, %xmm0 0x00007fff45ec6c30: movaps %xmm0, -0x50(%rbp) 0x00007fff45ec6c34: movl $0x1, -0x48(%rbp) 0x00007fff45ec6c3b: movl $0x8, %edi [ 0] 0x00007fff45ec6c2a WebKit`WebKit::WebCookieCache::clearForHost(WTF::String const&) [inlined] void WTF::refIfNotNull<WTF::StringImpl>(WTF::StringImpl*) + 5 at RefPtr.h:38 [ 0] 0x00007fff45ec6c25 WebKit`WebKit::WebCookieCache::clearForHost(WTF::String const&) [inlined] WTF::RefPtr<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> >::RefPtr(WTF::RefPtr<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> > const&) + 3 at RefPtr.h:59 [ 0] 0x00007fff45ec6c22 WebKit`WebKit::WebCookieCache::clearForHost(WTF::String const&) [inlined] WTF::RefPtr<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> >::RefPtr(WTF::RefPtr<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> > const&) at RefPtr.h:59 [ 0] 0x00007fff45ec6c22 WebKit`WebKit::WebCookieCache::clearForHost(WTF::String const&) [inlined] WTF::String::String(WTF::String const&) at WTFString.h:132 [ 0] 0x00007fff45ec6c22 WebKit`WebKit::WebCookieCache::clearForHost(WTF::String const&) [inlined] WTF::String::String(WTF::String const&) at WTFString.h:132 [ 0] 0x00007fff45ec6c22 WebKit`WebKit::WebCookieCache::clearForHost(WTF::String const&) + 132 at WebCookieCache.cpp:101 [ 1] 0x00007fff45ec7614 WebKit`WebKit::WebCookieJar::cookies(WebCore::Document&, WTF::URL const&) const [inlined] WebKit::WebCookieCache::pruneCacheIfNecessary() + 63 at WebCookieCache.cpp:113:9 [ 1] 0x00007fff45ec75d5 WebKit`WebKit::WebCookieJar::cookies(WebCore::Document&, WTF::URL const&) const [inlined] WebKit::WebCookieCache::cookiesForDOM(WTF::URL const&, WebCore::SameSiteInfo const&, WTF::URL const&, WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebCore::IncludeSecureCookies) + 626 at WebCookieCache.cpp:55 [ 1] 0x00007fff45ec7363 WebKit`WebKit::WebCookieJar::cookies(WebCore::Document&, WTF::URL const&) const + 1133 at WebCookieJar.cpp:127 [ 2] 0x00007fff447799d6 WebCore`WebCore::Document::cookie() + 438 at Document.cpp:4997:49
<rdar://problem/60453086>
Created attachment 393681 [details] Patch
Comment on attachment 393681 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393681&action=review > Tools/TestWebKitAPI/Tests/WebKitCocoa/CookiePrivateBrowsing.mm:138 > + for (int i = 0; i < 100; i++) { unsigned
Created attachment 393684 [details] Patch
Comment on attachment 393684 [details] Patch Clearing flags on attachment: 393684 Committed r258521: <https://trac.webkit.org/changeset/258521>
All reviewed patches have been landed. Closing bug.
Comment on attachment 393684 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393684&action=review > Source/WebKit/WebProcess/WebPage/WebCookieCache.cpp:123 > + String hostToRemove = *m_hostsWithInMemoryStorage.random(); > + clearForHost(hostToRemove); To me this seems like a workaround. The fix would be in the clearForHost function. Generally speaking functions that take references or pointers or const references — including const String& — have no right to assume the reference is safe to re-use after they make state changes. One kind of fix is to change the function argument to String rather than const String&. I am interested in our programming discipline to avoid such things, going beyond any one bug fix.
Comment on attachment 393684 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393684&action=review >> Source/WebKit/WebProcess/WebPage/WebCookieCache.cpp:123 >> + clearForHost(hostToRemove); > > To me this seems like a workaround. The fix would be in the clearForHost function. Generally speaking functions that take references or pointers or const references — including const String& — have no right to assume the reference is safe to re-use after they make state changes. > > One kind of fix is to change the function argument to String rather than const String&. > > I am interested in our programming discipline to avoid such things, going beyond any one bug fix. I considered several fixes but this is the one I liked best in the end. You are right that I could have simply passed in a 'String' instead of a 'const String&' but this seems to go against our usual coding practices where we ask developers to pass by reference to const as much as possible to avoid refcounting churn. I chose not to make the fix in clearForHost() because clearForHost() has no way to know that the host it is being provided is part of the HashSet it is about to modify. As a matter of fact, in the common case (not the pruning case) the String does not come from the HashSet. I see your point though that it would be safer for clearForHost() to not expect its parameters to be safe to reuse after they make any state changes. Either way, you could imagine cases where it is not super obvious. For e.g.: 1. I get a string from set of Object A and call a method on Object B passing the string as parameter 2. Object B does some modification of the set of object A (via a method call for e.g.) -> The String is no longer valid, even though Object A did not modify its own state. Passing a String instead of a const String& makes it less error-prone for sure but then we do ref-counting churn that will not be necessary in a lot of cases. In the past, we have tried avoiding that.
Comment on attachment 393684 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393684&action=review >>> Source/WebKit/WebProcess/WebPage/WebCookieCache.cpp:123 >>> + clearForHost(hostToRemove); >> >> To me this seems like a workaround. The fix would be in the clearForHost function. Generally speaking functions that take references or pointers or const references — including const String& — have no right to assume the reference is safe to re-use after they make state changes. >> >> One kind of fix is to change the function argument to String rather than const String&. >> >> I am interested in our programming discipline to avoid such things, going beyond any one bug fix. > > I considered several fixes but this is the one I liked best in the end. You are right that I could have simply passed in a 'String' instead of a 'const String&' but this seems to go against our usual coding practices where we ask developers to pass by reference to const as much as possible to avoid refcounting churn. I chose not to make the fix in clearForHost() because clearForHost() has no way to know that the host it is being provided is part of the HashSet it is about to modify. As a matter of fact, in the common case (not the pruning case) the String does not come from the HashSet. > > I see your point though that it would be safer for clearForHost() to not expect its parameters to be safe to reuse after they make any state changes. Either way, you could imagine cases where it is not super obvious. For e.g.: > 1. I get a string from set of Object A and call a method on Object B passing the string as parameter > 2. Object B does some modification of the set of object A (via a method call for e.g.) > -> The String is no longer valid, even though Object A did not modify its own state. > > Passing a String instead of a const String& makes it less error-prone for sure but then we do ref-counting churn that will not be necessary in a lot of cases. In the past, we have tried avoiding that. Seems great for fixing this one bug. Not sure it’s scalable. To be clear, when I said "make state changes" I did not mean an object modifying its own state, I meant generally doing any nontrivial operation.
Reopening for alternative fix.
Created attachment 393698 [details] Alternative fix
Comment on attachment 393698 [details] Alternative fix Do you like this version of the fix better?
Comment on attachment 393698 [details] Alternative fix I do like that better
If we use link time optimization and use String everywhere we currently use const String&, would it remove all the unnecessary ref/deref pairs?
Comment on attachment 393698 [details] Alternative fix Clearing flags on attachment: 393698 Committed r258530: <https://trac.webkit.org/changeset/258530>