Bug 237445 - [iOS] Books ASSERTs upon opening a book with a debug build of WebKit
Summary: [iOS] Books ASSERTs upon opening a book with a debug build of WebKit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-03-03 13:56 PST by Myles C. Maxfield
Modified: 2022-03-04 17:00 PST (History)
9 users (show)

See Also:

Attachments
Patch (9.16 KB, patch)
2022-03-03 13:59 PST, Myles C. Maxfield 		achristensen: review+
achristensen: commit-queue-
Details | Formatted Diff | Diff
Patch for committing (8.05 KB, patch)
2022-03-04 11:42 PST, Myles C. Maxfield 		no flags Details | Formatted Diff | Diff
Patch for committing (10.73 KB, patch)
2022-03-04 11:45 PST, Myles C. Maxfield 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for committing (10.82 KB, patch)
2022-03-04 15:01 PST, Myles C. Maxfield 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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>