Here is current case: SecurityOriginData(port = null) -> DatabaseIdentifier(port = 0) -> SecurityOriginData(port = 0) which should be: SecurityOriginData(port = null) -> DatabaseIdentifier -> SecurityOriginData(port = null) Currently LocalStorageDatabase sometimes unlinks database files when they are still in use. We have code to close the database before deletion, but due to the conversion issue about identifier, we failed to delete the correct database.
Created attachment 340561 [details] Patch
Created attachment 340672 [details] Patch
Comment on attachment 340672 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340672&action=review Note that you could have written an API test specific to SecurityOriginData, instead of a higher level one, since you change is specific to SecurityOriginData. See SecurityOrigin.cpp in our API test folder for existing SecurityOrigin API tests. > Source/WebCore/ChangeLog:8 > + Fixed the issue of null port when coverting between SecurityOriginData and DatabaseIdentifier. Typo: coverting > Source/WebCore/page/SecurityOriginData.cpp:118 > + return SecurityOriginData {databaseIdentifier.substring(0, separator1), databaseIdentifier.substring(separator1 + 1, separator2 - separator1 - 1), std::nullopt}; We may want to store databaseIdentifier.substring(0, separator1) and databaseIdentifier.substring(separator1 + 1, separator2 - separator1 - 1) in local variables with meaningful names, to avoid duplication and improve readability. We could also use ", port ? make_optional<uint16_t>(port) : std::nullopt)" below. This may actually be nicer. Also, I know you copied the code below but we normally have spaces between the curly brackets. return SecurityOriginData { a, b, c }; > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataRecord.mm:153 > + auto array = [[NSMutableArray alloc] init]; initWithCapacity since we know the size in advance? Also, may be nice to use NSMutableArray<NSString *> instead of auto here. I think auto ends up being NSMutableArray which is not as strongly typed. > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataRecord.mm:155 > + [array addObject:[[NSString alloc] initWithCString:origin.toString().utf8().data() encoding:NSUTF8StringEncoding]]; a WTF String can implicitly be converted into a NSString. I think this should work: [array addObject:origin.toString()]; If not, maybe: [array addObject:(NSString *)origin.toString()]; > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataRecord.mm:156 > + return array; I suck at objc but aren't we supposed to autorelease here? since we transfer ownership. > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataRecordPrivate.h:49 > +- (NSArray<NSString *> *)_originsString WK_API_AVAILABLE(macosx(10.13.4), ios(11.3)); You always want this for new API: WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA)); > Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStorageDatabaseTracker.mm:60 > + configuration.get().websiteDataStore = [WKWebsiteDataStore defaultDataStore]; Isn't this the default? If so, do we even need to pass in a configuration? > Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStorageDatabaseTracker.mm:62 > + webView.get().UIDelegate = [[LocalStorageUIDelegate alloc] init]; Isn't this leaking? We may want to store [[LocalStorageUIDelegate alloc] init] in a local RetainPtr<>, then then assign localStorageUIDelegate.get() here. > Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStorageDatabaseTracker.mm:67 > + TestWebKitAPI::Util::sleep(1); Why is this needed? Are we sure this is not going to be flaky? > Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStorageDatabaseTracker.mm:75 > + EXPECT_TRUE([[origins objectAtIndex:0] isEqual:@"http://localhost"]); Is there a way to make EXPECT_STREQ() work here? It would make the output more useful when failing.
Created attachment 340716 [details] Patch
(In reply to Chris Dumez from comment #3) > Comment on attachment 340672 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=340672&action=review > > Note that you could have written an API test specific to SecurityOriginData, > instead of a higher level one, since you change is specific to > SecurityOriginData. See SecurityOrigin.cpp in our API test folder for > existing SecurityOrigin API tests. > This is the code path that I found the bug so it's natural to write an API test like this. Might also be a good start to write an API test for LocalStorageDatabaseTracker. > > Source/WebCore/page/SecurityOriginData.cpp:118 > > + return SecurityOriginData {databaseIdentifier.substring(0, separator1), databaseIdentifier.substring(separator1 + 1, separator2 - separator1 - 1), std::nullopt}; > > We may want to store databaseIdentifier.substring(0, separator1) and > databaseIdentifier.substring(separator1 + 1, separator2 - separator1 - 1) in > local variables with meaningful names, to avoid duplication and improve > readability. > > We could also use ", port ? make_optional<uint16_t>(port) : std::nullopt)" > below. This may actually be nicer. > > Also, I know you copied the code below but we normally have spaces between > the curly brackets. return SecurityOriginData { a, b, c }; > Done. I tried using "? A : B", but error for A and B are incompatible types. > > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataRecord.mm:156 > > + return array; > > I suck at objc but aren't we supposed to autorelease here? since we transfer > ownership. > Yes! I didn't know that. > > Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStorageDatabaseTracker.mm:62 > > + webView.get().UIDelegate = [[LocalStorageUIDelegate alloc] init]; > > Isn't this leaking? We may want to store [[LocalStorageUIDelegate alloc] > init] in a local RetainPtr<>, then then assign localStorageUIDelegate.get() > here. > Ditto. > > Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStorageDatabaseTracker.mm:67 > > + TestWebKitAPI::Util::sleep(1); > > Why is this needed? Are we sure this is not going to be flaky? > LocalStorageDatabase's update interval is 1 sec, where it would create database file if not exists and notify LocalStorageDatabaseTracker to update the origins. So 1 second should be fine theoretically.
<rdar://problem/32620436>
Comment on attachment 340716 [details] Patch I will let someone else review due to the ObjC SPI bits. WebCore code LGTM.
(In reply to Sihui Liu from comment #6) > <rdar://problem/32620436> Is this right?
(In reply to mitz from comment #8) > (In reply to Sihui Liu from comment #6) > > <rdar://problem/32620436> > > Is this right? This is the cause (or one of the causes) of the synthetic crash.
Comment on attachment 340716 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340716&action=review > Source/WebCore/page/SecurityOriginData.cpp:119 > + if (!port) Do we need to check portAbsent here? It's weird that we check portAbsent above but not here. > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataRecord.mm:154 > + NSMutableArray<NSString *> *array = [[NSMutableArray alloc] initWithCapacity:origins.size()]; Most ObjC classes provide a simpler alternative that performs alloc + init + autorelease for you. In this case, you can use "[NSMutableArray arrayWithCapacity:origins.size()]", and then no autorelease below. I would use that here. It's not a big deal, but it's a nice idiom to get familiar with.
Comment on attachment 340716 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340716&action=review >> Source/WebCore/page/SecurityOriginData.cpp:119 >> + if (!port) > > Do we need to check portAbsent here? It's weird that we check portAbsent above but not here. toInt() would have returned 0 if the String was empty (or null) so checking portAbsent does not seems required to me.
Comment on attachment 340716 [details] Patch r=me
Comment on attachment 340716 [details] Patch Clearing flags on attachment: 340716 Committed r232079: <https://trac.webkit.org/changeset/232079>
All reviewed patches have been landed. Closing bug.