Bug 237445

Summary: [iOS] Books ASSERTs upon opening a book with a debug build of WebKit
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: New BugsAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, benjamin, cdumez, cgarcia, cmarcelo, ews-watchlist, sihui_liu, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
achristensen: review+, achristensen: commit-queue-
Patch for committing
none
Patch for committing
ews-feeder: commit-queue-
Patch for committing ews-feeder: commit-queue-

Description Myles C. Maxfield 2022-03-03 13:56:21 PST 
[iOS] Books crashes upon opening a book with a debug build of WebKit
Comment 1 Myles C. Maxfield 2022-03-03 13:59:57 PST 
Created attachment 453784 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2022-03-03 14:08:05 PST 
<rdar://problem/89776531>
Comment 3 Alex Christensen 2022-03-03 14:53:25 PST 
Comment on attachment 453784 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453784&action=review

Please add a test.  Just call lookUpContentRuleListForIdentifier with @"should-not-exist" and wait for it to return an error.  That used to hit this assertion, right?

> Source/WTF/ChangeLog:3
> +        [iOS] Books crashes upon opening a book with a debug build of WebKit

I would say "asserts" instead of "crashes"

> Source/WTF/wtf/FileSystem.cpp:446
> +    std::optional<FileSystem::MappedFileData> mappedFileOptional;

This seems unnecessary.  Why not just add an early return if makeSafeToUseMemoryMapForPath returns false?
Comment 4 Sihui Liu 2022-03-03 15:41:46 PST 
Comment on attachment 453784 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453784&action=review

> Source/WTF/wtf/cocoa/FileSystemCocoa.mm:220
> +    return !error && success;

Can you add log to print error if it's not nil? 
What is the error Books gets? I wonder if there's other issue we should fix
Comment 5 Alex Christensen 2022-03-03 18:39:52 PST 
Books gets this:
{Error Domain=NSPOSIXErrorDomain Code=2 "No such file or directory"}
Makes sense.
Comment 6 Myles C. Maxfield 2022-03-04 10:26:52 PST 
Comment on attachment 453784 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453784&action=review

>> Source/WTF/wtf/cocoa/FileSystemCocoa.mm:220
>> +    return !error && success;
> 
> Can you add log to print error if it's not nil? 
> What is the error Books gets? I wonder if there's other issue we should fix

Sure, I can add logging no problem.
Comment 7 Myles C. Maxfield 2022-03-04 11:42:33 PST 
Created attachment 453860 [details]
Patch for committing
Comment 8 Myles C. Maxfield 2022-03-04 11:45:33 PST 
Created attachment 453862 [details]
Patch for committing
Comment 9 Myles C. Maxfield 2022-03-04 15:01:18 PST 
Created attachment 453875 [details]
Patch for committing
Comment 10 Myles C. Maxfield 2022-03-04 15:08:31 PST 
Committed r290850 (248083@trunk): <https://commits.webkit.org/248083@trunk>