Bug 147036

Summary: [GTK] WebProcessMain::platformFinalize() is not called when web process finished with exit(0)
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED WONTFIX    
Severity: Normal CC: andersca, darin, mcatanzaro
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=168126
Attachments:
Description Flags
Patch none

Description Carlos Garcia Campos 2015-07-17 01:31:53 PDT
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.
Comment 1 Carlos Garcia Campos 2015-07-17 01:37:07 PDT
Created attachment 256961 [details]
Patch

If stopping the run loop works also for other ports we could get rid of the platform ifdef.
Comment 2 Martin Robinson 2015-07-20 09:58:13 PDT
Comment on attachment 256961 [details]
Patch

Looks good to me, but I guess we need a review from an Apple person.
Comment 3 Darin Adler 2015-07-20 17:38:10 PDT
Comment on attachment 256961 [details]
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?
Comment 4 Michael Catanzaro 2015-07-20 18:00:20 PDT
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.
Comment 5 Carlos Garcia Campos 2015-07-20 22:24:55 PDT
(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.
Comment 6 Carlos Garcia Campos 2015-07-24 03:03:32 PDT
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.
Comment 7 Michael Catanzaro 2015-07-26 11:19:08 PDT
I agree with Carlos, especially since this is needed to fix a major data loss bug. Martin/Darin, can you please grant review?
Comment 8 Darin Adler 2015-07-26 21:51:38 PDT
(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.
Comment 9 Carlos Garcia Campos 2015-07-27 00:37:49 PDT
(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.
Comment 10 Michael Catanzaro 2015-07-27 07:43:21 PDT
(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.
Comment 11 Darin Adler 2015-07-27 16:04:48 PDT
(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.
Comment 12 Carlos Garcia Campos 2015-07-28 01:53:26 PDT
(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.
Comment 13 Darin Adler 2015-07-28 11:55:28 PDT
(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.
Comment 14 Michael Catanzaro 2015-07-28 16:41:51 PDT
(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
Comment 15 Carlos Garcia Campos 2015-07-29 05:47:15 PDT
(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.