Bug 117625 – [EFL][WK2] Fix crash issue on ewk_context_favicon_database_directory_set()

• Component: WebKit EFL
• Status: RESOLVED WONTFIX

[Changhyup Jwa]

Currently WK2 doesn't support changing icon database path.
So ewk_context_favicon_database_directory_set() should return false on its' second function call.
But there's a crash if DB is not opened yet by first function call.
This issue can be avoided on WK1 by enabling icon database on first function call and check this flag on second function call.
So, I also appended enabling and disabling logic on WK2 and made test case for this operation.

[Changhyup Jwa]

Created attachment 204674 [details]
Patch

[Chris Dumez]

Comment on attachment 204674 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/WKContext.cpp:287
> +bool WKContextSetIconDatabasePath(WKContextRef contextRef, WKStringRef iconDatabasePath)

You should avoid changing C API if possible. Most C API setters don't return bool so this seems a bit inconsistent.

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:-161
> -    if (iconDatabase->isOpen())

I remember we added this check for a specific reason. iirc the icon database path can only be set once. When the path is set the first time, the icon database gets enabled / opened. Therefore, checking for isOpen() was used to make sure we set the icon database path only once (and avoid assertion hit in debug mode).

[Changhyup Jwa]

Created attachment 204675 [details]
Patch

[Changhyup Jwa]

(In reply to comment #2)
> (From update of attachment 204674 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204674&action=review
> 
> > Source/WebKit2/UIProcess/API/C/WKContext.cpp:287
> > +bool WKContextSetIconDatabasePath(WKContextRef contextRef, WKStringRef iconDatabasePath)
> 
> You should avoid changing C API if possible. Most C API setters don't return bool so this seems a bit inconsistent.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:-161
> > -    if (iconDatabase->isOpen())
> 
> I remember we added this check for a specific reason. iirc the icon database path can only be set once. When the path is set the first time, the icon database gets enabled / opened. Therefore, checking for isOpen() was used to make sure we set the icon database path only once (and avoid assertion hit in debug mode).

Oh, attached another patch at the same time.

I'll check what you commented. Thanks

[Changhyup Jwa]

(In reply to comment #2)
> (From update of attachment 204674 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204674&action=review
> 
> > Source/WebKit2/UIProcess/API/C/WKContext.cpp:287
> > +bool WKContextSetIconDatabasePath(WKContextRef contextRef, WKStringRef iconDatabasePath)
> 
> You should avoid changing C API if possible. Most C API setters don't return bool so this seems a bit inconsistent.
> 

Without changing C API(returning value), can I add WK C APIs to enable and check IconDatabase's enabled flag? Because isOpen() returns false before IconDatabase's sync thread opens DB.

> > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:-161
> > -    if (iconDatabase->isOpen())
> 
> I remember we added this check for a specific reason. iirc the icon database path can only be set once. When the path is set the first time, the icon database gets enabled / opened. Therefore, checking for isOpen() was used to make sure we set the icon database path only once (and avoid assertion hit in debug mode).

As you commented, icon database path should be set once. But without this patch, EwkContext::setFaviconDatabaseDirectoryPath() tries to open DB before sync thread's DB opening is done.

[Gyuyoung Kim]

Comment on attachment 204674 [details]
Patch

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

>>>> Source/WebKit2/UIProcess/API/C/WKContext.cpp:287
>>>> +bool WKContextSetIconDatabasePath(WKContextRef contextRef, WKStringRef iconDatabasePath)
>>> 
>>> You should avoid changing C API if possible. Most C API setters don't return bool so this seems a bit inconsistent.
>> 
>> Oh, attached another patch at the same time.
>> 
>> I'll check what you commented. Thanks
> 
> Without changing C API(returning value), can I add WK C APIs to enable and check IconDatabase's enabled flag? Because isOpen() returns false before IconDatabase's sync thread opens DB.

I think this patch also deals with common area in WK2. So, you should get a review by WK2 owner. IMHO, you're able to split this patch into common WK2 and EFL WK2.

> Source/WebKit2/UIProcess/API/C/WKContextPrivate.h:57
> +WK_EXPORT bool WKContextSetIconDatabasePath(WKContextRef context, WKStringRef iconDatabasePath);

It looks you need to consider all ports which are using this APIs.

[Changhyup Jwa]

(In reply to comment #6)
> (From update of attachment 204674 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204674&action=review
> 
> >>>> Source/WebKit2/UIProcess/API/C/WKContext.cpp:287
> >>>> +bool WKContextSetIconDatabasePath(WKContextRef contextRef, WKStringRef iconDatabasePath)
> >>> 
> >>> You should avoid changing C API if possible. Most C API setters don't return bool so this seems a bit inconsistent.
> >> 
> >> Oh, attached another patch at the same time.
> >> 
> >> I'll check what you commented. Thanks
> > 
> > Without changing C API(returning value), can I add WK C APIs to enable and check IconDatabase's enabled flag? Because isOpen() returns false before IconDatabase's sync thread opens DB.
> 
> I think this patch also deals with common area in WK2. So, you should get a review by WK2 owner. IMHO, you're able to split this patch into common WK2 and EFL WK2.
> 

Ok, I'll split this patch as you commented.

> > Source/WebKit2/UIProcess/API/C/WKContextPrivate.h:57
> > +WK_EXPORT bool WKContextSetIconDatabasePath(WKContextRef context, WKStringRef iconDatabasePath);
> 
> It looks you need to consider all ports which are using this APIs.

Yes, it's type changing from void to bool. So I think there's no regression.

Thanks for your comment.

[Gyuyoung Kim]

Comment on attachment 204675 [details]
Patch

Cleared review? from attachment 204675 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug or this bug again.

[Michael Catanzaro]

Closing this bug because the EFL port has been removed from trunk.

If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.