Conversion between SecurityOriginData and DatabaseIdentifier is asymmetric when port is null

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

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.

[Sihui Liu]

Created attachment 340561 [details]
Patch

[Sihui Liu]

Created attachment 340672 [details]
Patch

[Chris Dumez]

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.

[Sihui Liu]

Created attachment 340716 [details]
Patch

[Sihui Liu]

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

[Sihui Liu]

<rdar://problem/32620436>

[Chris Dumez]

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?

[Sihui Liu]

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

[Geoffrey Garen]

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.

[Chris Dumez]

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.

[Geoffrey Garen]

Comment on attachment 340716 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 340716 [details]
Patch

Clearing flags on attachment: 340716

Committed r232079: <https://trac.webkit.org/changeset/232079>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.