[WK2][EFL] Crash when calling WebOpenPanelResultListenerProxy::cancel() after PageClient is destroyed

[Chris Dumez]

WebOpenPanelResultListenerProxy currently holds a reference to the WebPageProxy (RefPtr) but WebPageProxy keeps a raw pointer to the PageClient.

It is therefore possible for the client to call WebOpenPanelResultListenerProxy::cancel() after PageClient is destroyed. At this point, WebOpenPanelResultListenerProxy has the last reference to WebPageProxy:

WebOpenPanelResultListenerProxy::cancel() will call WebPageProxy::didCancelForOpenPanel() which invalidate and destroy the WebOpenPanelResultListenerProxy.
Since WebOpenPanelResultListenerProxy has the last reference to WebPageProxy, WebOpenPanelResultListenerProxy::invalidate() will destroy WebPageProxy.
The WebPageProxy destructor will call WebPageProxy::close() which will attempt to call m_pageClient->pageClosed(). This will crash because the PageClient was destroyed already.

I was able to reproduce the issue by opening a new window in MiniBrowser, open a file chooser dialog and close the window while the file chooser dialog is still open.
The chooser dialog will get closed with the parent and WebOpenPanelResultListenerProxy::cancel() will be called. 

Here is the backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x0000000000f2b3a0 in ?? ()
(gdb) bt
#0  0x0000000000f2b3a0 in ?? ()
#1  0x00007ffff7874348 in WebKit::WebPageProxy::close (this=0x87bab0) at /home/chris/unencrypted/WebKit/Source/WebKit2/UIProcess/WebPageProxy.cpp:431
#2  0x00007ffff7873501 in WebKit::WebPageProxy::~WebPageProxy (this=0x87bab0, __in_chrg=<optimized out>)
    at /home/chris/unencrypted/WebKit/Source/WebKit2/UIProcess/WebPageProxy.cpp:253
#3  0x00007ffff7873874 in WebKit::WebPageProxy::~WebPageProxy (this=0x87bab0, __in_chrg=<optimized out>)
    at /home/chris/unencrypted/WebKit/Source/WebKit2/UIProcess/WebPageProxy.cpp:265
#4  0x00007ffff77ce806 in WTF::ThreadSafeRefCounted<WebKit::APIObject>::deref (this=0x87bab8)
    at /home/chris/unencrypted/WebKit/Source/WTF/wtf/ThreadSafeRefCounted.h:137
#5  0x00007ffff783618f in WTF::derefIfNotNull<WebKit::WebPageProxy> (ptr=0x87bab0) at /home/chris/unencrypted/WebKit/Source/WTF/wtf/PassRefPtr.h:53
#6  0x00007ffff7836106 in WTF::RefPtr<WebKit::WebPageProxy>::operator= (this=0x6399d0, optr=0x0) at /home/chris/unencrypted/WebKit/Source/WTF/wtf/RefPtr.h:126
#7  0x00007ffff786b7f1 in WebKit::WebOpenPanelResultListenerProxy::invalidate (this=0x6399c0)
    at /home/chris/unencrypted/WebKit/Source/WebKit2/UIProcess/WebOpenPanelResultListenerProxy.cpp:78
#8  0x00007ffff787fc4f in WebKit::WebPageProxy::didCancelForOpenPanel (this=0x87bab0) at /home/chris/unencrypted/WebKit/Source/WebKit2/UIProcess/WebPageProxy.cpp:3168
#9  0x00007ffff786b7ca in WebKit::WebOpenPanelResultListenerProxy::cancel (this=0x6399c0)
    at /home/chris/unencrypted/WebKit/Source/WebKit2/UIProcess/WebOpenPanelResultListenerProxy.cpp:73
#10 0x00007ffff79f40cb in Ewk_File_Chooser_Request::~Ewk_File_Chooser_Request (this=0xe1ace0, __in_chrg=<optimized out>)
    at /home/chris/unencrypted/WebKit/Source/WebKit2/UIProcess/API/efl/ewk_file_chooser_request.cpp:52
#11 0x00007ffff79f4d63 in WTF::RefCounted<Ewk_File_Chooser_Request>::deref (this=0xe1ace0) at /home/chris/unencrypted/WebKit/Source/WTF/wtf/RefCounted.h:202
#12 0x00007ffff79f43a5 in ewk_file_chooser_request_unref (request=0xe1ace0)
    at /home/chris/unencrypted/WebKit/Source/WebKit2/UIProcess/API/efl/ewk_file_chooser_request.cpp:94
#13 0x000000000040405d in close_file_picker (fs_data=0xa59660) at /home/chris/unencrypted/WebKit/Tools/MiniBrowser/efl/main.c:319
#14 0x000000000040408f in on_filepicker_parent_deletion (user_data=0xa59660, evas=0x62d140, window=0x5e3ad0, event=0x0)
    at /home/chris/unencrypted/WebKit/Tools/MiniBrowser/efl/main.c:326
#15 0x00007ffff7e2e0f2 in evas_object_event_callback_call (obj=0x5e3ad0, type=EVAS_CALLBACK_DEL, event_info=0x0, event_id=13272) at evas_callbacks.c:232
#16 0x00007ffff7e4a753 in evas_object_del (obj=0x5e3ad0) at evas_object_main.c:449
#17 0x00007ffff710c54f in _elm_win_delete_request (ee=<optimized out>) at elm_win.c:1479
#18 0x00007ffff7faecd9 in _ecore_evas_x_event_window_delete_request (data=<optimized out>, type=<optimized out>, event=0xf51bc0) at ecore_evas_x.c:1112
#19 0x00007ffff7fd2f50 in _ecore_call_handler_cb (event=<optimized out>, type=<optimized out>, data=<optimized out>, func=<optimized out>) at ecore_private.h:319
#20 _ecore_event_call () at ecore_events.c:559
#21 0x00007ffff7fd771c in _ecore_main_loop_iterate_internal (once_only=0) at ecore_main.c:1900
#22 0x00007ffff7fd7be7 in ecore_main_loop_begin () at ecore_main.c:934
#23 0x000000000040593c in elm_main (argc=2, argv=0x7fffffffe198) at /home/chris/unencrypted/WebKit/Tools/MiniBrowser/efl/main.c:870
#24 0x000000000040597e in main (argc=2, argv=0x7fffffffe198) at /home/chris/unencrypted/WebKit/Tools/MiniBrowser/efl/main.c:874

[Chris Dumez]

Created attachment 171911 [details]
Patch

[Darin Adler]

Comment on attachment 171911 [details]
Patch

This change is OK, but doesn’t seem like the right fix for the bug.

At a time when the PageClient is no longer guaranteed valid the WebPageProxy should not have a pointer to it any more. That’s the pointer that should be 0, the pointer to the client. That’s the problem we need to fix. This change tweaks lifetimes in a way that makes the bug go away in your test case, but someone else could have a RefPtr to the WebPageProxy that could extend its lifetime past the life of the PageClient, and we’d see the same problem.

Lets find a real fix; please only make this code change if there’s some other reason it’s beneficial

[Chris Dumez]

Ok, I found that I can fix this by calling WebPageProxy::close() in our port's view destructor. The Qt port is already doing this at least.

[Chris Dumez]

Created attachment 172084 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 172084 [details]
Patch

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

So does the Mac port.

- (void)dealloc
{
    _data->_page->close();
...

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:162
> +    // Close the page in case some other objects hold
> +    // a reference to it and keep it alive.

I'm not convinced that this comment adds much value.

[Chris Dumez]

Created attachment 172086 [details]
Patch for landing

I removed the comment.

Could someone please cq+ ?

[WebKit Review Bot]

Comment on attachment 172086 [details]
Patch for landing

Clearing flags on attachment: 172086

Committed r133318: <http://trac.webkit.org/changeset/133318>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.