Summary: | [GTK] When the WebProcess crashes, a signal should be emitted | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joaquim Rocha <jrocha> | ||||||||||||||||||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | andersca, benjamin, buildbot, cgarcia, commit-queue, dglazkov, gustavo, laszlo.gombos, mrobinson, rniwa, webkit.review.bot, xan.lopez | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Attachments: |
|
Description
Joaquim Rocha
2012-12-17 07:46:43 PST
Created attachment 179743 [details]
Patch
Comment on attachment 179743 [details]
Patch
Add cq?
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API Comment on attachment 179743 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179743&action=review Looks good, expect I'm not totally certain you are handling use via the C API. > Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:122 > - notImplemented(); > + webkitWebViewWebProcessCrashed(WEBKIT_WEB_VIEW(m_viewWidget)); Is it safe to assume that m_viewWidget is a WebKitWebView here? For instance, when using the C API it could be a WebKitWebViewBase. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1281 > + * This signal is emitted when the web process crashed. Probably want to expand this a bit. Explain when the web process will start again (on a new load, typically). Comment on attachment 179743 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179743&action=review We need unit tests for this too. >> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:122 >> + webkitWebViewWebProcessCrashed(WEBKIT_WEB_VIEW(m_viewWidget)); > > Is it safe to assume that m_viewWidget is a WebKitWebView here? For instance, when using the C API it could be a WebKitWebViewBase. Right. I think this could be used for internal stuff that we might want to do when the web process crashes, or just keep it unimplemented. To expose a signal in the API, we could use the processDidCrash callback of the Page Loader client. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:118 > + WEB_PROCESS_CRASHED, > + I'm not sure the web view is the best place for the signal. You might start a download from a web context and get a crash (there's a different callback for that in the download client), without having a view to emit the signal. Maybe we could have two different signals, one in the web view and the other in the web context or even the download object. In a multi web process model we definitely want to know which web page caused the crash to only show the crash message in the appropriate tab in the browser. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1280 > + * WebKitWebView::web-process-crashed: > + * @web_view: the #WebKitWebView on which the signal is emitted > + * For now this signal is only emitted when the web process crashes, I wonder what is going to happen when the network process crashes, for example, once the network process is implemented. Maybe we could call the signal process-crashed, and pass the process that crashed as an enum. If eventually more type of processed are added and we want to notify when they crash we just need to add the process type to the enum. Created attachment 179914 [details]
Patch
Comment on attachment 179914 [details] Patch Attachment 179914 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15411093 New failing tests: inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html Created attachment 190991 [details]
process-crashed.diff
Emit the signal from the WebContext, as agreed with Carlos, plus unit tests for this.
Attachment 190991 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebContextPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensions.cpp', u'Source/WebKit2/UIProcess/API/gtk/tests/WebExtensionTest.cpp']" exit_code: 1
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:181: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:182: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:183: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:184: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:185: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensions.cpp:49: Extra space before ( in function call [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensions.cpp:60: Extra space before ( in function call [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensions.cpp:61: Extra space before ( in function call [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensions.cpp:61: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensions.cpp:70: Use 0 instead of NULL. [readability/null] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h"
Total errors found: 10 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 190991 [details] process-crashed.diff View in context: https://bugs.webkit.org/attachment.cgi?id=190991&action=review I think there's another processCrashed callback in the downloads client, that's why we emit the signal in the web context indeed, we should also emit the signal in that case, but we can do it in a separate patch with a new test case for it. > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:177 > + * WebKitWebContext::process-crashed: > + * @context: the #WebKitWebContext > + * > + * This signal is emitted when a process crashes. I think this is only emitted when the web process crashes, and as I commented we might want to emit a signal for other processed, but I think it's unlikely. So, we either, leave this as process-crashed and add an enum parameter with the process type, or we name this web-process-crashed. > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h:102 > + void (* process_crashed) (WebKitWebContext *); > + This class is not inheritable in this moment, and you are not adding the offset to this in the class definition anyway, so just remove it. > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h:-105 > - void (*_webkit_reserved3) (void); Don't remove the padding for now, we are not abi stable yet. I plan to add padding to all classes before 2.0 release. >> Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensions.cpp:49 >> +static void process_crashed_cb (WebKitWebContext* context, WebViewTest* test) > > Extra space before ( in function call [whitespace/parens] [4] This should be processCrashedCallback > Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensions.cpp:75 > + g_main_loop_run(test->m_mainLoop); > + g_assert(!result); > + g_assert(processCrashed == TRUE); Check the result before running the main loop. Since the main loop is finished in the callback we don't probably need the processCrashed variable, we can leave it to make sure the main loop is not finished elsewhere but I think it's redundant. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensions.cpp:86 > + WebViewTest::add("WebKitWebExtension", "process-crashed", testWebExtensionAbortProcess); Nit: what we are testing is process-crashed signal so I would use WebKitWebContext instead of WebKitWebExtension and testWebContextProcessCrashed instead of testWebExtensionAbortProcess (In reply to comment #10) > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:177 > > + * WebKitWebContext::process-crashed: > > + * @context: the #WebKitWebContext > > + * > > + * This signal is emitted when a process crashes. > > I think this is only emitted when the web process crashes, and as I commented we might want to emit a signal for other processed, but I think it's unlikely. So, we either, leave this as process-crashed and add an enum parameter with the process type, or we name this web-process-crashed. I think making it extensible without knowing what will happen in the future is weird, so I'd say web-process-crashed for now. > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h:102 > > + void (* process_crashed) (WebKitWebContext *); > > + > > This class is not inheritable in this moment, and you are not adding the offset to this in the class definition anyway, so just remove it. Well I *am* adding the offset to the class definition ;) Seems you meant the signal definition, which I indeed forgot to modify for this. But since apparently it's useless to extend this class I'll just get rid of it as you suggest. > > > Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensions.cpp:75 > > + g_main_loop_run(test->m_mainLoop); > > + g_assert(!result); > > + g_assert(processCrashed == TRUE); > > Check the result before running the main loop. Since the main loop is finished in the callback we don't probably need the processCrashed variable, we can leave it to make sure the main loop is not finished elsewhere but I think it's redundant. OK, I'll just rely on the mainloop finishing. > > > Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensions.cpp:86 > > + WebViewTest::add("WebKitWebExtension", "process-crashed", testWebExtensionAbortProcess); > > Nit: what we are testing is process-crashed signal so I would use WebKitWebContext instead of WebKitWebExtension and testWebContextProcessCrashed instead of testWebExtensionAbortProcess OK. (In reply to comment #11) > > I think this is only emitted when the web process crashes, and as I commented we might want to emit a signal for other processed, but I think it's unlikely. So, we either, leave this as process-crashed and add an enum parameter with the process type, or we name this web-process-crashed. > > I think making it extensible without knowing what will happen in the future is weird, so I'd say web-process-crashed for now. Thinking about this, are we going to have multiple web processes soon? If the answer is yes how will we be able to tell which one crashed from the signal? They won't share the context? Created attachment 191032 [details]
process-crashed.diff
Updated per review comments.
Attachment 191032 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebContextPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensions.cpp', u'Source/WebKit2/UIProcess/API/gtk/tests/WebExtensionTest.cpp']" exit_code: 1
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:181: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:182: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:183: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:184: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:185: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 5 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 191033 [details]
process-crashed.diff
Upload the right patch, even.
Attachment 191033 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebContextPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensions.cpp', u'Source/WebKit2/UIProcess/API/gtk/tests/WebExtensionTest.cpp']" exit_code: 1
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:181: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:182: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:183: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:184: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:185: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 5 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #11) > > > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h:102 > > > + void (* process_crashed) (WebKitWebContext *); > > > + > > > > This class is not inheritable in this moment, and you are not adding the offset to this in the class definition anyway, so just remove it. > > Well I *am* adding the offset to the class definition ;) Seems you meant the signal definition, which I indeed forgot to modify for this. But since apparently it's useless to extend this class I'll just get rid of it as you suggest. With class definition I actually mean in the class_init, sorry, I reviewed this too fast. (In reply to comment #12) > (In reply to comment #11) > > > I think this is only emitted when the web process crashes, and as I commented we might want to emit a signal for other processed, but I think it's unlikely. So, we either, leave this as process-crashed and add an enum parameter with the process type, or we name this web-process-crashed. > > > > I think making it extensible without knowing what will happen in the future is weird, so I'd say web-process-crashed for now. > > Thinking about this, are we going to have multiple web processes soon? If the answer is yes how will we be able to tell which one crashed from the signal? They won't share the context? A we context represents a web process, so the web context emitting the signal i the one that crashed. Web views attached to that context can connect to the signal and show the error page, while other web views will be unaffected. Comment on attachment 191033 [details] process-crashed.diff View in context: https://bugs.webkit.org/attachment.cgi?id=191033&action=review Excellent! > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:186 > } What I'm doing in other cases for the signal definition since this new style started to rely on my emacs automatic indentation is: signals[WEB_PROCESS_CRASHED] = g_signal_new( "web-process-crashed", G_TYPE_FROM_CLASS(gObjectClass), .... That way I don't have to manually indent the lines, the style bot is happy and the code is kind of readable. Comment on attachment 191033 [details] process-crashed.diff View in context: https://bugs.webkit.org/attachment.cgi?id=191033&action=review Looks good to me. Some minor nits follow... > Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp:138 > +static void processDidCrash(WKPageRef page, const void *clientInfo) Nit: the asterisk is in the wrong place. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:186 >> } > > What I'm doing in other cases for the signal definition since this new style started to rely on my emacs automatic indentation is: > > signals[WEB_PROCESS_CRASHED] = g_signal_new( > "web-process-crashed", > G_TYPE_FROM_CLASS(gObjectClass), > .... > > That way I don't have to manually indent the lines, the style bot is happy and the code is kind of readable. Yeah, I've switched this style too. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensions.cpp:62 > + "/org/webkit/gtk/WebExtensionTest" , "org.webkit.gtk.WebExtensionTest", test->m_mainLoop)); Nit: extra space after the second " Created attachment 191675 [details]
process-crashed.diff
Fix style nits.
Comment on attachment 191675 [details] process-crashed.diff View in context: https://bugs.webkit.org/attachment.cgi?id=191675&action=review After looking at the implementation I've changed my mind, and I think it's better to move the signal to the web view. It will make the api easier to use. And for the download case, we can always add the signal to WebKitDownload, to notify the download that the web process crashed. > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:838 > +void webkitWebContextWebProcessCrashed(WebKitWebContext* context) > +{ > + g_signal_emit(context, signals[WEB_PROCESS_CRASHED], 0); > +} This is very problematic in the end, because we are emitting the signal in the context but for every web page. Created attachment 192206 [details]
web-process-crashed.diff
this moves the signal to WebKitWebView as discussed.
Comment on attachment 192206 [details] web-process-crashed.diff View in context: https://bugs.webkit.org/attachment.cgi?id=192206&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1305 > + signals[WEB_PROCESS_CRASHED] = g_signal_new("web-process-crashed", I usually move also the first parameter to the next line so that all parameters are lined up. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1308 > + 0, 0, 0, You should use G_STRUCT_OFFSET(WebKitWebViewClass, web_process_crashed), Also maybe we could make this true handled, and if not handled by the user we can show a default crashed page like we do for error pages. For now we could just make this true handled without a default implementation and add it later. Comment on attachment 192206 [details] web-process-crashed.diff Attachment 192206 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17027464 (In reply to comment #24) > (From update of attachment 192206 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=192206&action=review > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1305 > > + signals[WEB_PROCESS_CRASHED] = g_signal_new("web-process-crashed", > > I usually move also the first parameter to the next line so that all parameters are lined up. > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1308 > > + 0, 0, 0, > > You should use G_STRUCT_OFFSET(WebKitWebViewClass, web_process_crashed), Yeah, forgot that. > Also maybe we could make this true handled, and if not handled by the user we can show a default crashed page like we do for error pages. For now we could just make this true handled without a default implementation and add it later. If we do this then we'll have to solve the issue of showing a web page in a WebView whose web process just crashed. I assume it will be possible, but as we have seen it's apparently causing some bugs/crashes in Epiphany. Created attachment 192354 [details]
web-process-crashed.diff
The signal is now a true handled signal, but with no default action (yet).
Comment on attachment 192354 [details] web-process-crashed.diff View in context: https://bugs.webkit.org/attachment.cgi?id=192354&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1304 > + */ You should document here the return value of the signal. Returns: %TRUE to stop other handlers from being invoked for the event. %FALSE to propagate the event further. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2945 > +gboolean webkitWebViewWebProcessCrashed(WebKitWebView* webView) Do we need to return the signal result? I think this is only useful for the event propagation. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensions.cpp:48 > +static gboolean webProcessCrashedCallback(WebKitWebContext* context, WebViewTest* test) This should be WebKitWebView, not WebKitWebContext. We can omit the parameter name since it's unused. Created attachment 192372 [details]
0001-GTK-When-the-WebProcess-crashes-a-signal-should-be-e.patch
(In reply to comment #28) > View in context: https://bugs.webkit.org/attachment.cgi?id=192354& > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2945 > > +gboolean webkitWebViewWebProcessCrashed(WebKitWebView* webView) > > Do we need to return the signal result? I think this is only useful for the event propagation. AFAIK it still tells you whether someone handled the signal in the end or not, which might be useful to know, but I'll just remove it since we don't use the information at this point. Comment on attachment 192372 [details]
0001-GTK-When-the-WebProcess-crashes-a-signal-should-be-e.patch
Perfect, thank you!
Adding WebKit2 owners to CC Comment on attachment 192372 [details] 0001-GTK-When-the-WebProcess-crashes-a-signal-should-be-e.patch Attachment 192372 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17112017 New failing tests: editing/selection/selection-modify-crash.html Ping. Can any owner review or just rs this patch? Comment on attachment 192372 [details]
0001-GTK-When-the-WebProcess-crashes-a-signal-should-be-e.patch
Sorry for the delay.
This looks good to me, but feel free to have a GTK reviewer have a second look.
(In reply to comment #35) > (From update of attachment 192372 [details]) > Sorry for the delay. > This looks good to me, but feel free to have a GTK reviewer have a second look. Thanks for the review, I already approved this. Comment on attachment 192372 [details]
0001-GTK-When-the-WebProcess-crashes-a-signal-should-be-e.patch
Thanks!
Comment on attachment 192372 [details] 0001-GTK-When-the-WebProcess-crashes-a-signal-should-be-e.patch Rejecting attachment 192372 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 192372, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: aving rejects to file Source/WebKit2/UIProcess/API/gtk/tests/TestWebExtensions.cpp.rej patching file Source/WebKit2/UIProcess/API/gtk/tests/WebExtensionTest.cpp Hunk #2 FAILED at 31. Hunk #3 succeeded at 76 (offset 19 lines). 1 out of 3 hunks FAILED -- saving rejects to file Source/WebKit2/UIProcess/API/gtk/tests/WebExtensionTest.cpp.rej Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Benjamin Poulain']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.appspot.com/results/150263 Comment on attachment 192372 [details] 0001-GTK-When-the-WebProcess-crashes-a-signal-should-be-e.patch Pushed this manually to trunk after rebasing the patch, r148665. Closing bug. |