Bug 227100 – Fix crashes in ContentRuleListStore::lookupContentRuleList

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Fix crashes in ContentRuleListStore::lookupContentRuleList

[Alex Christensen]

Created attachment 431618 [details]
Patch

[Chris Dumez]

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]

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 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 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 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]

(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 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]

Created attachment 431628 [details]
Patch

[Alex Christensen]

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]

Created attachment 431629 [details]
Patch

[Chris Dumez]

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]

Created attachment 431683 [details]
Patch

[Alex Christensen]

r278995