WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
105180
[GTK] When the WebProcess crashes, a signal should be emitted
https://bugs.webkit.org/show_bug.cgi?id=105180
Summary
[GTK] When the WebProcess crashes, a signal should be emitted
Joaquim Rocha
Reported
2012-12-17 07:46:43 PST
The GTK+ port does not deal with an eventual crash in the web process. I will upload a patch that introduces a new signal which can be used as notification that the WebProcess crashed thus allowing us to e.g. warn the user in Epiphany.
Attachments
Patch
(5.32 KB, patch)
2012-12-17 07:52 PST
,
Joaquim Rocha
no flags
Details
Formatted Diff
Diff
Patch
(5.91 KB, patch)
2012-12-18 03:24 PST
,
Joaquim Rocha
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
process-crashed.diff
(11.05 KB, patch)
2013-03-01 11:03 PST
,
Xan Lopez
cgarcia
: review-
Details
Formatted Diff
Diff
process-crashed.diff
(9.57 KB, patch)
2013-03-01 14:03 PST
,
Xan Lopez
no flags
Details
Formatted Diff
Diff
process-crashed.diff
(9.54 KB, patch)
2013-03-01 14:07 PST
,
Xan Lopez
no flags
Details
Formatted Diff
Diff
process-crashed.diff
(9.45 KB, patch)
2013-03-06 00:55 PST
,
Xan Lopez
cgarcia
: review-
Details
Formatted Diff
Diff
web-process-crashed.diff
(9.83 KB, patch)
2013-03-08 05:38 PST
,
Xan Lopez
buildbot
: commit-queue-
Details
Formatted Diff
Diff
web-process-crashed.diff
(10.07 KB, patch)
2013-03-09 15:10 PST
,
Xan Lopez
no flags
Details
Formatted Diff
Diff
0001-GTK-When-the-WebProcess-crashes-a-signal-should-be-e.patch
(10.16 KB, patch)
2013-03-10 00:36 PST
,
Xan Lopez
benjamin
: review+
commit-queue
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(9)
View All
Add attachment
proposed patch, testcase, etc.
Joaquim Rocha
Comment 1
2012-12-17 07:52:25 PST
Created
attachment 179743
[details]
Patch
Joaquim Rocha
Comment 2
2012-12-17 07:52:46 PST
Comment on
attachment 179743
[details]
Patch Add cq?
WebKit Review Bot
Comment 3
2012-12-17 07:54:50 PST
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
Martin Robinson
Comment 4
2012-12-17 08:38:54 PST
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).
Carlos Garcia Campos
Comment 5
2012-12-18 00:59:05 PST
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.
Joaquim Rocha
Comment 6
2012-12-18 03:24:05 PST
Created
attachment 179914
[details]
Patch
WebKit Review Bot
Comment 7
2012-12-18 04:07:24 PST
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
Xan Lopez
Comment 8
2013-03-01 11:03:03 PST
Created
attachment 190991
[details]
process-crashed.diff Emit the signal from the WebContext, as agreed with Carlos, plus unit tests for this.
WebKit Review Bot
Comment 9
2013-03-01 11:05:18 PST
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.
Carlos Garcia Campos
Comment 10
2013-03-01 11:17:10 PST
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
Xan Lopez
Comment 11
2013-03-01 11:24:10 PST
(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.
Xan Lopez
Comment 12
2013-03-01 11:25:24 PST
(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?
Xan Lopez
Comment 13
2013-03-01 14:03:28 PST
Created
attachment 191032
[details]
process-crashed.diff Updated per review comments.
WebKit Review Bot
Comment 14
2013-03-01 14:06:03 PST
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.
Xan Lopez
Comment 15
2013-03-01 14:07:50 PST
Created
attachment 191033
[details]
process-crashed.diff Upload the right patch, even.
WebKit Review Bot
Comment 16
2013-03-01 14:15:09 PST
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.
Carlos Garcia Campos
Comment 17
2013-03-01 23:28:03 PST
(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.
Carlos Garcia Campos
Comment 18
2013-03-01 23:29:35 PST
(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.
Carlos Garcia Campos
Comment 19
2013-03-01 23:34:44 PST
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.
Martin Robinson
Comment 20
2013-03-04 11:32:40 PST
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 "
Xan Lopez
Comment 21
2013-03-06 00:55:41 PST
Created
attachment 191675
[details]
process-crashed.diff Fix style nits.
Carlos Garcia Campos
Comment 22
2013-03-07 01:16:23 PST
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.
Xan Lopez
Comment 23
2013-03-08 05:38:12 PST
Created
attachment 192206
[details]
web-process-crashed.diff this moves the signal to WebKitWebView as discussed.
Carlos Garcia Campos
Comment 24
2013-03-08 10:17:20 PST
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.
Build Bot
Comment 25
2013-03-08 18:11:54 PST
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
Xan Lopez
Comment 26
2013-03-09 02:38:46 PST
(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.
Xan Lopez
Comment 27
2013-03-09 15:10:44 PST
Created
attachment 192354
[details]
web-process-crashed.diff The signal is now a true handled signal, but with no default action (yet).
Carlos Garcia Campos
Comment 28
2013-03-10 00:12:48 PST
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.
Xan Lopez
Comment 29
2013-03-10 00:36:24 PST
Created
attachment 192372
[details]
0001-GTK-When-the-WebProcess-crashes-a-signal-should-be-e.patch
Xan Lopez
Comment 30
2013-03-10 00:37:24 PST
(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.
Carlos Garcia Campos
Comment 31
2013-03-10 00:40:00 PST
Comment on
attachment 192372
[details]
0001-GTK-When-the-WebProcess-crashes-a-signal-should-be-e.patch Perfect, thank you!
Carlos Garcia Campos
Comment 32
2013-03-10 01:08:58 PST
Adding WebKit2 owners to CC
Build Bot
Comment 33
2013-03-10 04:39:19 PDT
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
Xan Lopez
Comment 34
2013-04-16 09:25:13 PDT
Ping. Can any owner review or just rs this patch?
Benjamin Poulain
Comment 35
2013-04-17 14:23:53 PDT
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.
Carlos Garcia Campos
Comment 36
2013-04-17 23:40:27 PDT
(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.
Xan Lopez
Comment 37
2013-04-18 00:11:39 PDT
Comment on
attachment 192372
[details]
0001-GTK-When-the-WebProcess-crashes-a-signal-should-be-e.patch Thanks!
WebKit Commit Bot
Comment 38
2013-04-18 00:12:54 PDT
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
Xan Lopez
Comment 39
2013-04-18 03:34:46 PDT
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
.
Xan Lopez
Comment 40
2013-04-18 03:35:04 PDT
Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug