100977 2012-11-01 12:22:54 -0700 [WK2][EFL] Crash when calling WebOpenPanelResultListenerProxy::cancel() after PageClient is destroyed 2012-11-02 11:27:50 -0700 1 1 1 Unclassified WebKit WebKit2 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 cdumez cdumez andersca ap gyuyoung.kim kenneth laszlo.gombos naginenis rakuco sam tonikitoo webkit.review.bot oldest_to_newest 756533 0 cdumez 2012-11-01 12:22:54 -0700 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 756545 1 171911 cdumez 2012-11-01 12:44:42 -0700 Created attachment 171911 Patch 757292 2 171911 darin 2012-11-02 10:07:30 -0700 Comment on attachment 171911 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 757311 3 cdumez 2012-11-02 10:34:35 -0700 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. 757316 4 172084 cdumez 2012-11-02 10:37:46 -0700 Created attachment 172084 Patch 757322 5 172084 ap 2012-11-02 10:43:21 -0700 Comment on attachment 172084 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. 757332 6 172086 cdumez 2012-11-02 10:51:32 -0700 Created attachment 172086 Patch for landing I removed the comment. Could someone please cq+ ? 757377 7 172086 webkit.review.bot 2012-11-02 11:27:45 -0700 Comment on attachment 172086 Patch for landing Clearing flags on attachment: 172086 Committed r133318: <http://trac.webkit.org/changeset/133318> 757378 8 webkit.review.bot 2012-11-02 11:27:50 -0700 All reviewed patches have been landed. Closing bug. 171911 2012-11-01 12:44:42 -0700 2012-11-02 10:37:46 -0700 Patch 100977_WebOpenPanelResultListenerProxy_crash.patch text/plain 1801 cdumez ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQyL0No YW5nZUxvZwppbmRleCBiZmIyYTZiLi4yYjFhZTdhIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 Mi9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjUg QEAKKzIwMTItMTEtMDEgIENocmlzdG9waGUgRHVtZXogIDxjaHJpc3RvcGhlLmR1bWV6QGludGVs LmNvbT4KKworICAgICAgICBbV0syXSBDcmFzaCB3aGVuIGNhbGxpbmcgV2ViT3BlblBhbmVsUmVz dWx0TGlzdGVuZXJQcm94eTo6Y2FuY2VsKCkgYWZ0ZXIgUGFnZUNsaWVudCBpcyBkZXN0cm95ZWQK KyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEwMDk3Nwor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEhhdmUgV2Vi T3BlblBhbmVsUmVzdWx0TGlzdGVuZXJQcm94eSBrZWVwIGEgcmF3IHBvaW50ZXIgdG8gdGhlIFdl YlBhZ2VQcm94eQorICAgICAgICBvYmplY3QgaW5zdGVhZCBvZiBhIFJlZlB0ciB0byBtYWtlIHN1 cmUgdGhhdCBXZWJPcGVuUGFuZWxSZXN1bHRMaXN0ZW5lclByb3h5CisgICAgICAgIGlzIG5vdCB0 aGUgbGFzdCBvbmUgaG9sZGluZyBhIHJlZmVyZW5jZSB0byB0aGUgV2ViUGFnZVByb3h5LgorCisg ICAgICAgIFRoZXJlIGlzIG5vIHJpc2sgaW4gV2ViT3BlblBhbmVsUmVzdWx0TGlzdGVuZXJQcm94 eSB1c2luZyB0aGUgV2ViUGFnZVByb3h5CisgICAgICAgIG9iamVjdCBhZnRlciBpdCBoYXMgYmVl biBkZXN0cm95ZWQgYmVjYXVzZSB0aGUgV2ViUGFnZVByb3h5IGRlc3RydWN0b3IgY2FsbHMKKyAg ICAgICAgV2ViT3BlblBhbmVsUmVzdWx0TGlzdGVuZXJQcm94eTo6aW52YWxpZGF0ZSgpIHdoaWNo IHNldHMgdGhlIFdlYlBhZ2VQcm94eQorICAgICAgICBwb2ludGVyIHRvIDAuCisKKyAgICAgICAg VGhpcyBjaGFuZ2UgbWFrZXMgc3VyZSB0aGF0IGNhbGxpbmcgV2ViT3BlblBhbmVsUmVzdWx0TGlz dGVuZXJQcm94eTo6Y2FuY2VsKCkKKyAgICAgICAgd2lsbCBub3QgY3Jhc2ggYWZ0ZXIgUGFnZUNs aWVudCBpcyBkZXN0cm95ZWQuCisKKyAgICAgICAgKiBVSVByb2Nlc3MvV2ViT3BlblBhbmVsUmVz dWx0TGlzdGVuZXJQcm94eS5oOgorICAgICAgICAoV2ViT3BlblBhbmVsUmVzdWx0TGlzdGVuZXJQ cm94eSk6CisKIDIwMTItMTEtMDEgIEh1YW5nIERvbmdzdW5nICA8bHV4dGVsbGFAY29tcGFueTEw MC5uZXQ+CiAKICAgICAgICAgQ29vcmRpbmF0ZWQgR3JhcGhpY3M6IENsZWFuIHVwIGhlYWRlcnMg b2YgQ29vcmRpbmF0ZWQgR3JhcGhpY3MuCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9VSVBy b2Nlc3MvV2ViT3BlblBhbmVsUmVzdWx0TGlzdGVuZXJQcm94eS5oIGIvU291cmNlL1dlYktpdDIv VUlQcm9jZXNzL1dlYk9wZW5QYW5lbFJlc3VsdExpc3RlbmVyUHJveHkuaAppbmRleCA1ZWMzNjlj Li41YmMzMTE4IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvV2ViT3BlblBh bmVsUmVzdWx0TGlzdGVuZXJQcm94eS5oCisrKyBiL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9X ZWJPcGVuUGFuZWxSZXN1bHRMaXN0ZW5lclByb3h5LmgKQEAgLTU2LDcgKzU2LDcgQEAgcHJpdmF0 ZToKIAogICAgIHZpcnR1YWwgVHlwZSB0eXBlKCkgY29uc3QgeyByZXR1cm4gQVBJVHlwZTsgfQog Ci0gICAgUmVmUHRyPFdlYlBhZ2VQcm94eT4gbV9wYWdlOworICAgIFdlYlBhZ2VQcm94eSogbV9w YWdlOwogfTsKIAogfSAvLyBuYW1lc3BhY2UgV2ViS2l0Cg== 172084 2012-11-02 10:37:46 -0700 2012-11-02 10:51:32 -0700 Patch 100977_webpageproxy_close.patch text/plain 1542 cdumez ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQyL0No YW5nZUxvZwppbmRleCAwMjVmZTdmLi5iZDE4ZGNmIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 Mi9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTcg QEAKKzIwMTItMTEtMDIgIENocmlzdG9waGUgRHVtZXogIDxjaHJpc3RvcGhlLmR1bWV6QGludGVs LmNvbT4KKworICAgICAgICBbV0syXVtFRkxdIENyYXNoIHdoZW4gY2FsbGluZyBXZWJPcGVuUGFu ZWxSZXN1bHRMaXN0ZW5lclByb3h5OjpjYW5jZWwoKSBhZnRlciBQYWdlQ2xpZW50IGlzIGRlc3Ry b3llZAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTAw OTc3CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQ2Fs bCBXZWJQYWdlUHJveHk6OmNsb3NlKCkgaW4gRXdrVmlld0ltcGwncyBkZXN0cnVjdG9yIHRvIGF2 b2lkIGNyYXNoZXMKKyAgICAgICAgaWYgb3RoZXIgb2JqZWN0cyBrZWVwIGEgcmVmZXJlbmNlIHRv IHRoZSBXZWJQYWdlUHJveHkgYW5kIHRyeSB0byB1c2UKKyAgICAgICAgaXQgYWZ0ZXIgdGhlIHZp ZXcgKGFuZCB0aGVyZWZvcmUgdGhlIFBhZ2VDbGllbnQpIGlzIGRlc3Ryb3llZC4KKworICAgICAg ICAqIFVJUHJvY2Vzcy9BUEkvZWZsL0V3a1ZpZXdJbXBsLmNwcDoKKyAgICAgICAgKEV3a1ZpZXdJ bXBsOjp+RXdrVmlld0ltcGwpOgorCiAyMDEyLTExLTAyICBCeXVuZ3dvbyBMZWUgIDxidzgwLmxl ZUBzYW1zdW5nLmNvbT4KIAogICAgICAgICBbRUZMXVtXSzJdIENoYW5nZSB0aGUgc2NvcGUgb2Yg bG9ja2luZyBpbiBXb3JrUXVldWVFZmwuY3BwLgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdDIv VUlQcm9jZXNzL0FQSS9lZmwvRXdrVmlld0ltcGwuY3BwIGIvU291cmNlL1dlYktpdDIvVUlQcm9j ZXNzL0FQSS9lZmwvRXdrVmlld0ltcGwuY3BwCmluZGV4IGNiMWFmZjIuLmYwYjM2MDAgMTAwNjQ0 Ci0tLSBhL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZWZsL0V3a1ZpZXdJbXBsLmNwcAor KysgYi9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL2VmbC9Fd2tWaWV3SW1wbC5jcHAKQEAg LTE1OCw2ICsxNTgsMTAgQEAgRXdrVmlld0ltcGw6OkV3a1ZpZXdJbXBsKEV2YXNfT2JqZWN0KiB2 aWV3LCBQYXNzUmVmUHRyPEV3a19Db250ZXh0PiBjb250ZXh0LCBQYXMKIAogRXdrVmlld0ltcGw6 On5Fd2tWaWV3SW1wbCgpCiB7CisgICAgLy8gQ2xvc2UgdGhlIHBhZ2UgaW4gY2FzZSBzb21lIG90 aGVyIG9iamVjdHMgaG9sZAorICAgIC8vIGEgcmVmZXJlbmNlIHRvIGl0IGFuZCBrZWVwIGl0IGFs aXZlLgorICAgIG1fcGFnZVByb3h5LT5jbG9zZSgpOworCiAgICAgLy8gVW5yZWdpc3RlciBpY29u IGNoYW5nZSBjYWxsYmFjay4KICAgICBFd2tfRmF2aWNvbl9EYXRhYmFzZSogaWNvbkRhdGFiYXNl ID0gbV9jb250ZXh0LT5mYXZpY29uRGF0YWJhc2UoKTsKICAgICBBU1NFUlQoaWNvbkRhdGFiYXNl KTsK 172086 2012-11-02 10:51:32 -0700 2012-11-02 11:27:45 -0700 Patch for landing 100977_webpageproxy_close.patch text/plain 1446 cdumez ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQyL0No YW5nZUxvZwppbmRleCAwMjVmZTdmLi4yMDhhZGI5IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 Mi9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTcg QEAKKzIwMTItMTEtMDIgIENocmlzdG9waGUgRHVtZXogIDxjaHJpc3RvcGhlLmR1bWV6QGludGVs LmNvbT4KKworICAgICAgICBbV0syXVtFRkxdIENyYXNoIHdoZW4gY2FsbGluZyBXZWJPcGVuUGFu ZWxSZXN1bHRMaXN0ZW5lclByb3h5OjpjYW5jZWwoKSBhZnRlciBQYWdlQ2xpZW50IGlzIGRlc3Ry b3llZAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTAw OTc3CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgQWxleGV5IFByb3NrdXJ5YWtvdi4KKworICAgICAg ICBDYWxsIFdlYlBhZ2VQcm94eTo6Y2xvc2UoKSBpbiBFd2tWaWV3SW1wbCdzIGRlc3RydWN0b3Ig dG8gYXZvaWQgY3Jhc2hlcworICAgICAgICBpZiBvdGhlciBvYmplY3RzIGtlZXAgYSByZWZlcmVu Y2UgdG8gdGhlIFdlYlBhZ2VQcm94eSBhbmQgdHJ5IHRvIHVzZQorICAgICAgICBpdCBhZnRlciB0 aGUgdmlldyAoYW5kIHRoZXJlZm9yZSB0aGUgUGFnZUNsaWVudCkgaXMgZGVzdHJveWVkLgorCisg ICAgICAgICogVUlQcm9jZXNzL0FQSS9lZmwvRXdrVmlld0ltcGwuY3BwOgorICAgICAgICAoRXdr Vmlld0ltcGw6On5Fd2tWaWV3SW1wbCk6CisKIDIwMTItMTEtMDIgIEJ5dW5nd29vIExlZSAgPGJ3 ODAubGVlQHNhbXN1bmcuY29tPgogCiAgICAgICAgIFtFRkxdW1dLMl0gQ2hhbmdlIHRoZSBzY29w ZSBvZiBsb2NraW5nIGluIFdvcmtRdWV1ZUVmbC5jcHAuCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2Vi S2l0Mi9VSVByb2Nlc3MvQVBJL2VmbC9Fd2tWaWV3SW1wbC5jcHAgYi9Tb3VyY2UvV2ViS2l0Mi9V SVByb2Nlc3MvQVBJL2VmbC9Fd2tWaWV3SW1wbC5jcHAKaW5kZXggY2IxYWZmMi4uMTBjNDc5ZSAx MDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL0FQSS9lZmwvRXdrVmlld0ltcGwu Y3BwCisrKyBiL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZWZsL0V3a1ZpZXdJbXBsLmNw cApAQCAtMTU4LDYgKzE1OCw4IEBAIEV3a1ZpZXdJbXBsOjpFd2tWaWV3SW1wbChFdmFzX09iamVj dCogdmlldywgUGFzc1JlZlB0cjxFd2tfQ29udGV4dD4gY29udGV4dCwgUGFzCiAKIEV3a1ZpZXdJ bXBsOjp+RXdrVmlld0ltcGwoKQogeworICAgIG1fcGFnZVByb3h5LT5jbG9zZSgpOworCiAgICAg Ly8gVW5yZWdpc3RlciBpY29uIGNoYW5nZSBjYWxsYmFjay4KICAgICBFd2tfRmF2aWNvbl9EYXRh YmFzZSogaWNvbkRhdGFiYXNlID0gbV9jb250ZXh0LT5mYXZpY29uRGF0YWJhc2UoKTsKICAgICBB U1NFUlQoaWNvbkRhdGFiYXNlKTsK