Bug 183051 – [Curl] Cookie Database files are wrongfully getting deleted when the database is opened

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Christopher Reid]

The file stat logic in CookieJarDB::checkDatabaseCorruptionAndRemoveIfNeeded is backwards causing persistence issues. The database is wrongfully deleted when opened because it thinks it's corrupted.

[Christopher Reid]

Created attachment 334465 [details]
Patch

[Don Olmstead]

Comment on attachment 334465 [details]
Patch

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

LGTM. I'm not sure how liberally we should be using auto or not.

> Source/WebCore/platform/network/curl/CookieJarDB.cpp:166
> +    FileSystem::PlatformFileHandle handle = FileSystem::openFile(getCorruptionMarkerPath(), FileSystem::FileOpenMode::Write);

Maybe auto here?

[Christopher Reid]

(In reply to Don Olmstead from comment #2)
> Comment on attachment 334465 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334465&action=review
> 
> LGTM. I'm not sure how liberally we should be using auto or not.
> 
> > Source/WebCore/platform/network/curl/CookieJarDB.cpp:166
> > +    FileSystem::PlatformFileHandle handle = FileSystem::openFile(getCorruptionMarkerPath(), FileSystem::FileOpenMode::Write);
> 
> Maybe auto here?

It looks like auto is used quite a bit with openFile in the FileSystem implementation files so it makes sense to use it here too.
I'll update the patch.

[Christopher Reid]

Created attachment 334547 [details]
Patch

[Per Arne Vollan]

Comment on attachment 334547 [details]
Patch

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

R=me.

> Source/WebCore/platform/network/curl/CookieJarDB.cpp:167
> +    FileSystem::closeFile(handle);

Should we check whether 'handle' is valid before closing?

[Christopher Reid]

Thanks for the review!

(In reply to Per Arne Vollan from comment #5)
> Comment on attachment 334547 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334547&action=review
> 
> R=me.
> 
> > Source/WebCore/platform/network/curl/CookieJarDB.cpp:167
> > +    FileSystem::closeFile(handle);
> 
> Should we check whether 'handle' is valid before closing?

Ah, right now all the closeFile implementations have a check but it seems safer to have a check here too in case more functionality is added to that file.

[Christopher Reid]

Created attachment 334550 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 334550 [details]
Patch for landing

Clearing flags on attachment: 334550

Committed r228971: <https://trac.webkit.org/changeset/228971>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/37846254>