Fix crashes in ContentRuleListStore::lookupContentRuleList
Created attachment 431618 [details] Patch
Comment on attachment 431618 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431618&action=review > Source/WTF/ChangeLog:13 > + This reverts the moveFile part of r276879 on Cocoa platforms. So this is purely speculative? We have no reproduction case?
True. There is one person in the radar that claims this happens a lot. After landing I'm going to ask them to check if it fixes it for them.
Comment on attachment 431618 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431618&action=review >> Source/WTF/ChangeLog:13 >> + This reverts the moveFile part of r276879 on Cocoa platforms. > > So this is purely speculative? We have no reproduction case? There are crashing in shipping. The move to std::filesystem didn't ship yet so why do we think this will help?
Comment on attachment 431618 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431618&action=review >>> Source/WTF/ChangeLog:13 >>> + This reverts the moveFile part of r276879 on Cocoa platforms. >> >> So this is purely speculative? We have no reproduction case? > > There are crashing in shipping. The move to std::filesystem didn't ship yet so why do we think this will help? Given that this makes our code worse, I don't think we should make this change *speculatively*. There are multiple reasons for that could cause crashes when reading from a mmap'd file, the most common one being that the file got modified on disk after it was mmap'd. The way we usually deal with this is by deleting the existing file before writing a new version (keeps existing mmap'd handles working and seeing the old version).
Comment on attachment 431618 [details] Patch The crash data from rdar://78816611 shows that on May 17, 2021 something changed and considerably increased this crash. There isn't much code in ContentRuleListStore::lookupContentRuleList and r276879 modified it, so this reverts that. I don't think it makes the code worse. It just makes it like it used to be.
(In reply to Alex Christensen from comment #6) > Comment on attachment 431618 [details] > Patch > > The crash data from rdar://78816611 shows that on May 17, 2021 something > changed and considerably increased this crash. There isn't much code in > ContentRuleListStore::lookupContentRuleList and r276879 modified it, so this > reverts that. I don't think it makes the code worse. It just makes it like > it used to be. And the radar shows there are 40 crashes with Safari 14.0.2 with exactly the same crash trace: Thread 18 Crashed ↩:: Dispatch queue: ContentRuleListStore Read Queue 0 com.apple.JavaScriptCore 0x00007fff373eca36 WTF::Persistence::Decoder::operator>>(WTF::Optional<unsigned int>&) + 38 1 com.apple.WebKit 0x00007fff3c96614f WTF::Detail::CallableWrapper<WTF::Optional<API::ContentRuleListMetaData> API::decodeContentRuleListMetaData<WebKit::NetworkCache::Data>(WebKit::NetworkCache::Data const&)::'lambda'(unsigned char const*, unsigned long), bool, unsigned char const*, unsigned long>::call(unsigned char const*, unsigned long) + 75 2 com.apple.WebKit 0x00007fff3c965f13 API::openAndMapOrCopyContentRuleList(WTF::String const&) + 304 3 com.apple.WebKit 0x00007fff3c965ad2 WTF::Detail::CallableWrapper<API::ContentRuleListStore::lookupContentRuleList(WTF::String const&, WTF::CompletionHandler<void (WTF::RefPtr<API::ContentRuleList, WTF::DumbPtrTraits<API::ContentRuleList> >, std::__1::error_code)>)::$_0, void>::call() + 68 4 libdispatch.dylib 0x00007fff201a55dd _dispatch_call_block_and_release + 12 5 libdispatch.dylib 0x00007fff201a67c7 _dispatch_client_callout + 8 6 libdispatch.dylib 0x00007fff201ac5fe _dispatch_lane_serial_drain + 606 7 libdispatch.dylib 0x00007fff201ad0cb _dispatch_lane_invoke + 375 8 libdispatch.dylib 0x00007fff201b6c5d _dispatch_workloop_worker_thread + 819 9 libsystem_pthread.dylib 0x00007fff2034e499 _pthread_wqthread + 314 10 libsystem_pthread.dylib 0x00007fff2034d467 start_wqthread + 15 How do you explain those? I am sorry but I don't think it makes sense to revert a change, speculatively without an explanation for why the crashes already happened before that change.
Comment on attachment 431618 [details] Patch r- since there are identical crashes before the change that this patch reverts and I did not see an explanation for that.
Created attachment 431628 [details] Patch
I think it does make sense to revert a change like this because it happened at the same time as a considerable increase in crash rate. But to make it more likely to get reviewed, I'm just doing the WebKit part of this then we'll see if it is fixed.
Created attachment 431629 [details] Patch
Comment on attachment 431629 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431629&action=review r=me > Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:469 > + if (!moveFile(legacyPath, path)) { Are we sure there isn't already a file at the destination path? If there might be, we likely want to delete it first (in case it is mmap'd).
Created attachment 431683 [details] Patch
r278995