Bug 185715 - Conversion between SecurityOriginData and DatabaseIdentifier is asymmetric when port is null
Summary: Conversion between SecurityOriginData and DatabaseIdentifier is asymmetric wh...
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: Sihui Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-05-17 01:21 PDT by Sihui Liu
Modified: 2018-05-22 14:14 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.67 KB, patch)
2018-05-17 01:25 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (13.03 KB, patch)
2018-05-17 18:47 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (13.20 KB, patch)
2018-05-18 11:35 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sihui Liu 2018-05-17 01:21:09 PDT
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.
Comment 1 Sihui Liu 2018-05-17 01:25:20 PDT
Created attachment 340561 [details]
Patch
Comment 2 Sihui Liu 2018-05-17 18:47:39 PDT
Created attachment 340672 [details]
Patch
Comment 3 Chris Dumez 2018-05-17 19:38:45 PDT
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.
Comment 4 Sihui Liu 2018-05-18 11:35:55 PDT
Created attachment 340716 [details]
Patch
Comment 5 Sihui Liu 2018-05-18 11:51:59 PDT
(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.
Comment 6 Sihui Liu 2018-05-18 16:33:21 PDT
<rdar://problem/32620436>
Comment 7 Chris Dumez 2018-05-18 16:34:48 PDT
Comment on attachment 340716 [details]
Patch

I will let someone else review due to the ObjC SPI bits. WebCore code LGTM.
Comment 8 mitz 2018-05-18 16:38:23 PDT
(In reply to Sihui Liu from comment #6)
> <rdar://problem/32620436>

Is this right?
Comment 9 Sihui Liu 2018-05-18 17:11:21 PDT
(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 10 Geoffrey Garen 2018-05-21 11:09:18 PDT
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 11 Chris Dumez 2018-05-21 14:08:01 PDT
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 12 Geoffrey Garen 2018-05-22 13:46:31 PDT
Comment on attachment 340716 [details]
Patch

r=me
Comment 13 WebKit Commit Bot 2018-05-22 14:14:05 PDT
Comment on attachment 340716 [details]
Patch

Clearing flags on attachment: 340716

Committed r232079: <https://trac.webkit.org/changeset/232079>
Comment 14 WebKit Commit Bot 2018-05-22 14:14:06 PDT
All reviewed patches have been landed.  Closing bug.