RESOLVED FIXED 227100
Fix crashes in ContentRuleListStore::lookupContentRuleList
https://bugs.webkit.org/show_bug.cgi?id=227100
Summary Fix crashes in ContentRuleListStore::lookupContentRuleList
Alex Christensen
Reported 2021-06-16 16:12:15 PDT
Fix crashes in ContentRuleListStore::lookupContentRuleList
Attachments
Patch (5.67 KB, patch)
2021-06-16 16:26 PDT, Alex Christensen
no flags
Patch (3.57 KB, patch)
2021-06-16 18:07 PDT, Alex Christensen
ews-feeder: commit-queue-
Patch (3.57 KB, patch)
2021-06-16 18:18 PDT, Alex Christensen
no flags
Patch (3.75 KB, patch)
2021-06-17 09:39 PDT, Alex Christensen
no flags
Alex Christensen
Comment 1 2021-06-16 16:26:41 PDT
Chris Dumez
Comment 2 2021-06-16 16:29:31 PDT
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?
Alex Christensen
Comment 3 2021-06-16 16:30:09 PDT
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.
Chris Dumez
Comment 4 2021-06-16 16:52:46 PDT
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?
Chris Dumez
Comment 5 2021-06-16 17:02:34 PDT
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).
Alex Christensen
Comment 6 2021-06-16 17:55:06 PDT
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.
Chris Dumez
Comment 7 2021-06-16 18:03:55 PDT
(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.
Chris Dumez
Comment 8 2021-06-16 18:04:41 PDT
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.
Alex Christensen
Comment 9 2021-06-16 18:07:56 PDT
Alex Christensen
Comment 10 2021-06-16 18:10:43 PDT
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.
Alex Christensen
Comment 11 2021-06-16 18:18:18 PDT
Chris Dumez
Comment 12 2021-06-16 18:30:27 PDT
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).
Alex Christensen
Comment 13 2021-06-17 09:39:11 PDT
Alex Christensen
Comment 14 2021-06-17 10:17:43 PDT
Note You need to log in before you can comment on or make changes to this bug.