147036 2015-07-17 01:31:53 -0700 [GTK] WebProcessMain::platformFinalize() is not called when web process finished with exit(0) 2017-08-07 05:43:25 -0700 1 1 1 Unclassified WebKit WebKit2 528+ (Nightly build) Unspecified Unspecified RESOLVED WONTFIX https://bugs.webkit.org/show_bug.cgi?id=168126 Gtk P2 Normal --- 1 cgarcia webkit-unassigned andersca darin mcatanzaro oldest_to_newest 1110103 0 cgarcia 2015-07-17 01:31:53 -0700 This happens very often when the web process finishes from Connection::didFailToSendSyncMessage(). At least for the GTK+ port, we could stop the main loop instead of calling exit, and that way, the platformFinalize() method will be called. 1110104 1 256961 cgarcia 2015-07-17 01:37:07 -0700 Created attachment 256961 Patch If stopping the run loop works also for other ports we could get rid of the platform ifdef. 1110644 2 256961 mrobinson 2015-07-20 09:58:13 -0700 Comment on attachment 256961 Patch Looks good to me, but I guess we need a review from an Apple person. 1110847 3 256961 darin 2015-07-20 17:38:10 -0700 Comment on attachment 256961 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256961&action=review > Source/WebKit2/Platform/IPC/Connection.cpp:863 > +#if PLATFORM(GTK) > + RunLoop::main().stop(); > +#else > exit(0); > +#endif Anders, do you think we should just do the RunLoop::stop thing on all platforms? I need more context. What’s the critical code in platformFinalize that always needs to be called? 1110854 4 mcatanzaro 2015-07-20 18:00:20 -0700 For GTK it is the code Carlos is adding to save the clipboard contents in the clipboard manager, so that you can paste data that you copied from a web process after closing the web process. 1110894 5 cgarcia 2015-07-20 22:24:55 -0700 (In reply to comment #4) > For GTK it is the code Carlos is adding to save the clipboard contents in > the clipboard manager, so that you can paste data that you copied from a web > process after closing the web process. And also the soup cache dump when using the shared secondary process model, which is very important to keep the disk cache resources in sync with the index. 1111959 6 cgarcia 2015-07-24 03:03:32 -0700 ping? This patch doesn't actually change any cross-platform behaviour, so maybe we can unblock this by landing the patch and decide whether to remove the platform #ifdef later if the solution is valid for other ports. 1112309 7 mcatanzaro 2015-07-26 11:19:08 -0700 I agree with Carlos, especially since this is needed to fix a major data loss bug. Martin/Darin, can you please grant review? 1112355 8 darin 2015-07-26 21:51:38 -0700 (In reply to comment #4) > For GTK it is the code Carlos is adding to save the clipboard contents in > the clipboard manager, so that you can paste data that you copied from a web > process after closing the web process. It would be much better to do that earlier, perhaps based on some coalescing timer. You don’t want to lose the clipboard if the web process crashes. Generally it’s not a good pattern to have any code that runs only on exit. One of the main benefits of having a web process is better behavior if a crash occurs. 1112377 9 cgarcia 2015-07-27 00:37:49 -0700 (In reply to comment #8) > (In reply to comment #4) > > For GTK it is the code Carlos is adding to save the clipboard contents in > > the clipboard manager, so that you can paste data that you copied from a web > > process after closing the web process. > > It would be much better to do that earlier, perhaps based on some coalescing > timer. You don’t want to lose the clipboard if the web process crashes. > Generally it’s not a good pattern to have any code that runs only on exit. > One of the main benefits of having a web process is better behavior if a > crash occurs. gtk_clipboard_store() is expected to be called only when the application is about to quit, and that's what all GTK+ applications do, because GTK calls it after the main loop quits. We could use a compiler destructor to do all the cleanup though, but I still thing exit(0) is not the best way to finish the web process when we can just quit the main loop, and finish properly from main. 1112410 10 mcatanzaro 2015-07-27 07:43:21 -0700 (In reply to comment #9) > gtk_clipboard_store() is expected to be called only when the application is > about to quit, and that's what all GTK+ applications do, because GTK calls > it after the main loop quits. Well... that is certainly the common usage, but I think it's fine to call gtk_clipboard_store() whenever we receive data. Otherwise, it's hard to think of why else the API exists, if not to be used. The benefit would be as Darin mentions. The cost would be performance in the case of huge clipboard selections, but that's not a super practical concern, and clipboard managers can reject selections that are too big. So I think either approach is fine. (In reply to comment #9) > I still thing exit(0) is not the best way to finish > the web process when we can just quit the main loop, and finish properly > from main. Agreed. 1112563 11 darin 2015-07-27 16:04:48 -0700 (In reply to comment #9) > I still thing exit(0) is not the best way to finish > the web process when we can just quit the main loop, and finish properly > from main. I think we should continue to use exit(0); seems it would be faster. But I understand that you might not agree. 1112693 12 cgarcia 2015-07-28 01:53:26 -0700 (In reply to comment #11) > (In reply to comment #9) > > I still thing exit(0) is not the best way to finish > > the web process when we can just quit the main loop, and finish properly > > from main. > > I think we should continue to use exit(0); seems it would be faster. But I > understand that you might not agree. So, would you be opposed to leave the exit(0) for other ports and quit the main loop for GTK+ only? Otherwise I will just remove the platformFinalize and use a compiler destructor instead. 1112791 13 darin 2015-07-28 11:55:28 -0700 (In reply to comment #12) > So, would you be opposed to leave the exit(0) for other ports and quit the > main loop for GTK+ only? Yes, I am opposed to it. Not necessarily strongly opposed, but I don’t want to do this in WebKit. > Otherwise I will just remove the platformFinalize > and use a compiler destructor instead. In case it’s not clear, I’m opposed to that too. WebKit should not have code that relies on running when the process exits. Not good architecture. 1112929 14 mcatanzaro 2015-07-28 16:41:51 -0700 (In reply to comment #13) > WebKit should not have code that relies on running when the process exits. > Not good architecture. FWIW we already have such code, but I don't know where or why, and it's causing crashes: bug #145347 1113073 15 cgarcia 2015-07-29 05:47:15 -0700 (In reply to comment #10) > (In reply to comment #9) > > gtk_clipboard_store() is expected to be called only when the application is > > about to quit, and that's what all GTK+ applications do, because GTK calls > > it after the main loop quits. > > Well... that is certainly the common usage, but I think it's fine to call > gtk_clipboard_store() whenever we receive data. Otherwise, it's hard to > think of why else the API exists, if not to be used. The fact that the API exists doesn't mean it's convenient to be used at any moment. gtk_clipboard_store uses a nested main loop with a timeout of 10 milliseconds. The nested main loop uses the default main context, so any other sources sent to the default main context (like an IPC message handler) would be handled by the nested main loop. That can cause problems, I had a lot of issues with the nested main loop used by gtk_enumerate_printers, because it handled other sources, for example. So, the safest way to use this API, which is how everybody uses it (included firefox for example), is to call it when the application finishes, right after the main loop quits. 256961 2015-07-17 01:37:07 -0700 2015-07-29 23:10:25 -0700 Patch wk2-platform-finalize.diff text/plain 1495 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQyL0No YW5nZUxvZwppbmRleCAwZWY3NmI5Li4wODM2YTg4IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 Mi9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCkBAIC0xLDUgKzEsMjAg QEAKIDIwMTUtMDctMTcgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29t PgogCisgICAgICAgIFtHVEtdIFdlYlByb2Nlc3NNYWluOjpwbGF0Zm9ybUZpbmFsaXplKCkgaXMg bm90IGNhbGxlZCB3aGVuIHdlYiBwcm9jZXNzIGZpbmlzaGVkIHdpdGggZXhpdCgwKQorICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTQ3MDM2CisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGhpcyBoYXBwZW5zIHZl cnkgb2Z0ZW4gd2hlbiB0aGUgd2ViIHByb2Nlc3MgZmluaXNoZXMgZnJvbQorICAgICAgICBDb25u ZWN0aW9uOjpkaWRGYWlsVG9TZW5kU3luY01lc3NhZ2UoKS4gQXQgbGVhc3QgZm9yIHRoZSBHVEsr CisgICAgICAgIHBvcnQsIHdlIGNvdWxkIHN0b3AgdGhlIG1haW4gbG9vcCBpbnN0ZWFkIG9mIGNh bGxpbmcgZXhpdCwgYW5kCisgICAgICAgIHRoYXQgd2F5LCB0aGUgcGxhdGZvcm1GaW5hbGl6ZSgp IG1ldGhvZCB3aWxsIGJlIGNhbGxlZC4KKworICAgICAgICAqIFBsYXRmb3JtL0lQQy9Db25uZWN0 aW9uLmNwcDoKKyAgICAgICAgKElQQzo6Q29ubmVjdGlvbjo6ZGlkRmFpbFRvU2VuZFN5bmNNZXNz YWdlKToKKworMjAxNS0wNy0xNyAgQ2FybG9zIEdhcmNpYSBDYW1wb3MgIDxjZ2FyY2lhQGlnYWxp YS5jb20+CisKICAgICAgICAgW0dUS10gQ2xlYW51cCBQYXN0ZWJvYXJkSGVscGVyCiAgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNDcwMzUKIApkaWZmIC0t Z2l0IGEvU291cmNlL1dlYktpdDIvUGxhdGZvcm0vSVBDL0Nvbm5lY3Rpb24uY3BwIGIvU291cmNl L1dlYktpdDIvUGxhdGZvcm0vSVBDL0Nvbm5lY3Rpb24uY3BwCmluZGV4IDA3YTkzYjUuLmNlOTg2 NTQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQyL1BsYXRmb3JtL0lQQy9Db25uZWN0aW9uLmNw cAorKysgYi9Tb3VyY2UvV2ViS2l0Mi9QbGF0Zm9ybS9JUEMvQ29ubmVjdGlvbi5jcHAKQEAgLTg1 Niw3ICs4NTYsMTEgQEAgdm9pZCBDb25uZWN0aW9uOjpkaWRGYWlsVG9TZW5kU3luY01lc3NhZ2Uo KQogICAgIGlmICghbV9zaG91bGRFeGl0T25TeW5jTWVzc2FnZVNlbmRGYWlsdXJlKQogICAgICAg ICByZXR1cm47CiAKKyNpZiBQTEFURk9STShHVEspCisgICAgUnVuTG9vcDo6bWFpbigpLnN0b3Ao KTsKKyNlbHNlCiAgICAgZXhpdCgwKTsKKyNlbmRpZgogfQogCiB2b2lkIENvbm5lY3Rpb246OmVu cXVldWVJbmNvbWluZ01lc3NhZ2Uoc3RkOjp1bmlxdWVfcHRyPE1lc3NhZ2VEZWNvZGVyPiBpbmNv bWluZ01lc3NhZ2UpCg==