Bug 117625 - [EFL][WK2] Fix crash issue on ewk_context_favicon_database_directory_set()
Summary: [EFL][WK2] Fix crash issue on ewk_context_favicon_database_directory_set()
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-13 21:43 PDT by Changhyup Jwa
Modified: 2017-03-11 10:48 PST (History)
7 users (show)

See Also:


Attachments
Patch (9.57 KB, patch)
2013-06-13 22:20 PDT, Changhyup Jwa
no flags Details | Formatted Diff | Diff
Patch (9.52 KB, patch)
2013-06-13 22:48 PDT, Changhyup Jwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Changhyup Jwa 2013-06-13 21:43:36 PDT
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.
Comment 1 Changhyup Jwa 2013-06-13 22:20:36 PDT
Created attachment 204674 [details]
Patch
Comment 2 Chris Dumez 2013-06-13 22:46:22 PDT
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).
Comment 3 Changhyup Jwa 2013-06-13 22:48:46 PDT
Created attachment 204675 [details]
Patch
Comment 4 Changhyup Jwa 2013-06-13 22:50:18 PDT
(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
Comment 5 Changhyup Jwa 2013-06-13 23:19:16 PDT
(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.
Comment 6 Gyuyoung Kim 2013-06-13 23:45:40 PDT
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.
Comment 7 Changhyup Jwa 2013-06-13 23:50:43 PDT
(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.
Comment 8 Gyuyoung Kim 2013-07-16 18:48:53 PDT
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.
Comment 9 Michael Catanzaro 2017-03-11 10:48:35 PST
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.