WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(3.57 KB, patch)
2021-06-16 18:07 PDT
,
Alex Christensen
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(3.57 KB, patch)
2021-06-16 18:18 PDT
,
Alex Christensen
no flags
Details
Formatted Diff
Diff
Patch
(3.75 KB, patch)
2021-06-17 09:39 PDT
,
Alex Christensen
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Alex Christensen
Comment 1
2021-06-16 16:26:41 PDT
Created
attachment 431618
[details]
Patch
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
Created
attachment 431628
[details]
Patch
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
Created
attachment 431629
[details]
Patch
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
Created
attachment 431683
[details]
Patch
Alex Christensen
Comment 14
2021-06-17 10:17:43 PDT
r278995
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug