Bug 134663

Summary: [EFL] Fix crash caused by invalid cursor image.
Product: WebKit Reporter: KwangHyuk <hyuki.kim>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, bunhere, cdumez, commit-queue, gyuyoung.kim, lucas.de.marchi, rniwa, ryuan.choi, sergio
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
none
Patch
gyuyoung.kim: review+
Patch
none
patch none

Description KwangHyuk 2014-07-06 08:19:37 PDT
From the map.naver.com, crash occurred when user repeats the below operation several times.
1) moves mouse from map to the outside of mini browser
2) moves mouse from the outside of mini browser to the nearest landmark on the map area.
Comment 1 KwangHyuk 2014-07-06 08:22:53 PDT
Created attachment 234458 [details]
Patch

Add code to prevent referring of unavailable cursor image on EwkView since the custom cursor image is invalid once a mouse is out of the webview.
Comment 2 Gyuyoung Kim 2014-07-06 08:44:08 PDT
Comment on attachment 234458 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:223
> +    smartData->mouse_in = 1;

Use true instead of 1.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:233
> +    smartData->mouse_in = 0;

Use true instead of 0.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:224
> +    Eina_Bool mouse_in;

I'm not sure whether we should maintain this flag as a field of Ewk_View_Smart_Data since this flag is only used internally. Any reason we should open this flag as public ? If not, it looks we maintain it as one of EwkView member internally.
Comment 3 Gyuyoung Kim 2014-07-06 08:48:45 PDT
Comment on attachment 234458 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:233
>> +    smartData->mouse_in = 0;
> 
> Use true instead of 0.

Typo: true -> false.
Comment 4 KwangHyuk 2014-07-06 09:00:53 PDT
>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:224
>> +    Eina_Bool mouse_in;

> I'm not sure whether we should maintain this flag as a field of Ewk_View_Smart_Data since this flag is only used internally. Any reason we should open this flag as public ? If not, it looks we maintain it as one of EwkView member internally.

Thank you and agree with you.

I found better approach. it will be updated soon. :)
Comment 5 KwangHyuk 2014-07-06 09:11:36 PDT
Created attachment 234461 [details]
Patch

Remove calling of updateCursor since the custom cursor image is invalid once a mouse is out of the webview. in addition, EVAS_CALLBACK_MOUSE_IN callback subscribtion is also removed because it's handling updateCursor only.
Comment 6 Gyuyoung Kim 2014-07-06 09:17:36 PDT
Comment on attachment 234461 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:-225
> -    self->updateCursor();

This callback was added by r165730 in order to restore previous cursor before going out of webview.
http://trac.webkit.org/changeset/165730

Can we restore previous cursor when coming back to inside WebView without this cursor update ?
Comment 7 KwangHyuk 2014-07-06 10:18:26 PDT
(In reply to comment #6)
> (From update of attachment 234461 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234461&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:-225
> > -    self->updateCursor();
> 
> This callback was added by r165730 in order to restore previous cursor before going out of webview.
> http://trac.webkit.org/changeset/165730
> 
> Can we restore previous cursor when coming back to inside WebView without this cursor update ?

Re-entrance point can be always changed randomly according to user's intention.
In addition, cursor image is newly updated whenever mouse re-enter the webview again.
So, I can' sure whether restoration is good idea.

However, currently custom cursor seems to be destroyed as soon as it was rendered.
So, at least, updateCursor on mouse in callback seems to be removed.
Comment 8 Build Bot 2014-07-06 11:16:21 PDT
Comment on attachment 234461 [details]
Patch

Attachment 234461 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5361787385937920

New failing tests:
media/W3C/video/networkState/networkState_during_loadstart.html
Comment 9 Build Bot 2014-07-06 11:16:27 PDT
Created attachment 234463 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 10 Ryuan Choi 2014-07-06 17:51:01 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 234461 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=234461&action=review
> > 
> > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:-225
> > > -    self->updateCursor();
> > 
> > This callback was added by r165730 in order to restore previous cursor before going out of webview.
> > http://trac.webkit.org/changeset/165730
> > 
> > Can we restore previous cursor when coming back to inside WebView without this cursor update ?
> 
> Re-entrance point can be always changed randomly according to user's intention.
> In addition, cursor image is newly updated whenever mouse re-enter the webview again.
> So, I can' sure whether restoration is good idea.
> 
> However, currently custom cursor seems to be destroyed as soon as it was rendered.
> So, at least, updateCursor on mouse in callback seems to be removed.

I got your point, but it just mean that we move back to previous state.

IMO, One other solution is removing Evas Object based cursor.
Recently, I realized that evas object cursor is not useful to collaborate with multiple windows.

After removed evas object cursor, I think that we can implement custom cursor as xpixmap which EwkView maintains.
Although it tightly depends on X, ewebkit now only support X backend and we can make general function like applyFallbackCursor having several backend until EFL implement common APIs for the cursor.
Comment 11 Gyuyoung Kim 2014-07-06 19:32:45 PDT
Comment on attachment 234461 [details]
Patch

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

>>>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:-225
>>>> -    self->updateCursor();
>>> 
>>> This callback was added by r165730 in order to restore previous cursor before going out of webview.
>>> http://trac.webkit.org/changeset/165730
>>> 
>>> Can we restore previous cursor when coming back to inside WebView without this cursor update ?
>> 
>> Re-entrance point can be always changed randomly according to user's intention.
>> In addition, cursor image is newly updated whenever mouse re-enter the webview again.
>> So, I can' sure whether restoration is good idea.
>> 
>> However, currently custom cursor seems to be destroyed as soon as it was rendered.
>> So, at least, updateCursor on mouse in callback seems to be removed.
> 
> I got your point, but it just mean that we move back to previous state.
> 
> IMO, One other solution is removing Evas Object based cursor.
> Recently, I realized that evas object cursor is not useful to collaborate with multiple windows.
> 
> After removed evas object cursor, I think that we can implement custom cursor as xpixmap which EwkView maintains.
> Although it tightly depends on X, ewebkit now only support X backend and we can make general function like applyFallbackCursor having several backend until EFL implement common APIs for the cursor.

How about keeping void EwkViewEventHandler<EVAS_CALLBACK_MOUSE_IN>::handleEvent(void* data, Evas*, Evas_Object*, void*) with "notImplemented", then add "FIXME: bla bla" ?
Comment 12 KwangHyuk 2014-07-06 20:54:12 PDT
(In reply to comment #11)
> (From update of attachment 234461 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234461&action=review
> 
> >>>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:-225
> >>>> -    self->updateCursor();
> >>> 
> >>> This callback was added by r165730 in order to restore previous cursor before going out of webview.
> >>> http://trac.webkit.org/changeset/165730
> >>> 
> >>> Can we restore previous cursor when coming back to inside WebView without this cursor update ?
> >> 
> >> Re-entrance point can be always changed randomly according to user's intention.
> >> In addition, cursor image is newly updated whenever mouse re-enter the webview again.
> >> So, I can' sure whether restoration is good idea.
> >> 
> >> However, currently custom cursor seems to be destroyed as soon as it was rendered.
> >> So, at least, updateCursor on mouse in callback seems to be removed.
> > 
> > I got your point, but it just mean that we move back to previous state.
> > 
> > IMO, One other solution is removing Evas Object based cursor.
> > Recently, I realized that evas object cursor is not useful to collaborate with multiple windows.
> > 
> > After removed evas object cursor, I think that we can implement custom cursor as xpixmap which EwkView maintains.
> > Although it tightly depends on X, ewebkit now only support X backend and we can make general function like applyFallbackCursor having several backend until EFL implement common APIs for the cursor.
> 
> How about keeping void EwkViewEventHandler<EVAS_CALLBACK_MOUSE_IN>::handleEvent(void* data, Evas*, Evas_Object*, void*) with "notImplemented", then add "FIXME: bla bla" ?

Thank you for your idea. :)
Comment 13 KwangHyuk 2014-07-07 10:31:22 PDT
Created attachment 234497 [details]
Patch

Apply reviewer's comment.
Comment 14 Gyuyoung Kim 2014-07-07 18:58:41 PDT
Comment on attachment 234497 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:222
> +    // FIXME : self->updateCursor() call is removed because crash occurs when invalid cursor image is referred.

nit: FIXME : -> FIXME:
Comment 15 KwangHyuk 2014-07-08 06:32:37 PDT
Created attachment 234559 [details]
Patch
Comment 16 Gyuyoung Kim 2014-07-08 21:10:32 PDT
(In reply to comment #15)
> Created an attachment (id=234559) [details]
> Patch

Once you got r+ed, you don't need to request r? again. Just request cq?, then committer can land the patch.
Comment 17 KwangHyuk 2014-07-08 21:13:11 PDT
(In reply to comment #16)
> (In reply to comment #15)
> > Created an attachment (id=234559) [details] [details]
> > Patch
> 
> Once you got r+ed, you don't need to request r? again. Just request cq?, then committer can land the patch.

Ok, good tip.
Comment 18 KwangHyuk 2014-07-08 21:14:55 PDT
Created attachment 234617 [details]
patch
Comment 19 WebKit Commit Bot 2014-07-09 00:29:40 PDT
Comment on attachment 234617 [details]
patch

Clearing flags on attachment: 234617

Committed r170914: <http://trac.webkit.org/changeset/170914>
Comment 20 WebKit Commit Bot 2014-07-09 00:29:47 PDT
All reviewed patches have been landed.  Closing bug.