Bug 134663 – [EFL] Fix crash caused by invalid cursor image.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[KwangHyuk]

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.

[KwangHyuk]

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.

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

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.

[KwangHyuk]

>> 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. :)

[KwangHyuk]

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.

[Gyuyoung Kim]

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 ?

[KwangHyuk]

(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.

[Build Bot]

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

[Build Bot]

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

[Ryuan Choi]

(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.

[Gyuyoung Kim]

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" ?

[KwangHyuk]

(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. :)

[KwangHyuk]

Created attachment 234497 [details]
Patch

Apply reviewer's comment.

[Gyuyoung Kim]

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:

[KwangHyuk]

Created attachment 234559 [details]
Patch

[Gyuyoung Kim]

(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.

[KwangHyuk]

(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.

[KwangHyuk]

Created attachment 234617 [details]
patch

[WebKit Commit Bot]

Comment on attachment 234617 [details]
patch

Clearing flags on attachment: 234617

Committed r170914: <http://trac.webkit.org/changeset/170914>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.