Bug 147036 – [GTK] WebProcessMain::platformFinalize() is not called when web process finished with exit(0)

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit2

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 256961 [details]
Patch

If stopping the run loop works also for other ports we could get rid of the platform ifdef.

[Martin Robinson]

Comment on attachment 256961 [details]
Patch

Looks good to me, but I guess we need a review from an Apple person.

[Darin Adler]

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?

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

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.

[Michael Catanzaro]

I agree with Carlos, especially since this is needed to fix a major data loss bug. Martin/Darin, can you please grant review?

[Darin Adler]

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

[Carlos Garcia Campos]

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

[Michael Catanzaro]

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

[Darin Adler]

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

[Carlos Garcia Campos]

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

[Darin Adler]

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

[Michael Catanzaro]

(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

[Carlos Garcia Campos]

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