Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2086483006:
Dump process type into breakpad. (Closed)
|
Created:
4 years, 6 months ago by hush (inactive) Modified:
4 years, 4 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, android-webview-reviews_chromium.org, sadrul, kalyank Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDump process type into breakpad.
The process type can be "webview", "browser", or "renderer".
BUG=616774
Committed: https://crrev.com/e1a6b0a706a4badbe6131215da9e335d62ac5c93
Cr-Commit-Position: refs/heads/master@{#407869}
Patch Set 1 : Dump process type into breakpad. #
Total comments: 8
Patch Set 2 : comments #
Total comments: 6
Patch Set 3 : review #Patch Set 4 : change the process names #
Total comments: 1
Patch Set 5 : comment #
Messages
Total messages: 38 (13 generated)
Patchset #1 (id:1) has been deleted
hush@chromium.org changed reviewers: + torne@chromium.org
PTAL https://codereview.chromium.org/2086483006/diff/20001/components/crash/conten... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2086483006/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:104: const char kWebViewSingleProcessType[] = "webview-singleprocess"; I hope this change does not break anything that tries to strictly match "webview" (like somewhere in the server side?)
https://codereview.chromium.org/2086483006/diff/20001/components/crash/conten... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2086483006/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:104: const char kWebViewSingleProcessType[] = "webview-singleprocess"; On 2016/06/22 21:34:19, hush wrote: > I hope this change does not break anything that tries to strictly match > "webview" (like somewhere in the server side?) Since we don't upload the string currently I don't think this can break anything on the server side. I'm not sure "webview-singleprocess" is a good string here, though. It seems like it would make more sense to either: 1) have *all* the webview process types identify that they're webview, for the avoidance of doubt, or 2) have the string just be "singleprocess" or "single" or similar, and *also* use it for chrome if chrome is running with kSingleProcess set (and then possibly also change minidumps similarly)? I'd lean toward 2.. what do you think? https://codereview.chromium.org/2086483006/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:923: if (process_type == kWebViewSingleProcessType) { There's a problem here, which is not introduced by your change (it's already wrong), but we should probably address it - I'm fairly sure that the special handling of DumpWithoutCrashing needs to *always* be applied to WebView, not just when it's running in single-process mode, which is currently what happens here. You might want to check with Toby about this. If that's the case, then we can't rely on process_type to know whether to do something special for webview and would need to configure this in some other way.
hush@chromium.org changed reviewers: + tobiasjs@chromium.org
https://codereview.chromium.org/2086483006/diff/20001/components/crash/conten... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2086483006/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:104: const char kWebViewSingleProcessType[] = "webview-singleprocess"; On 2016/06/23 13:40:10, Torne wrote: > On 2016/06/22 21:34:19, hush wrote: > > I hope this change does not break anything that tries to strictly match > > "webview" (like somewhere in the server side?) > > Since we don't upload the string currently I don't think this can break anything > on the server side. > > I'm not sure "webview-singleprocess" is a good string here, though. It seems > like it would make more sense to either: > 1) have *all* the webview process types identify that they're webview, for the > avoidance of doubt, or > 2) have the string just be "singleprocess" or "single" or similar, and *also* > use it for chrome if chrome is running with kSingleProcess set (and then > possibly also change minidumps similarly)? > > I'd lean toward 2.. what do you think? Does chrome actually run in single process mode in any production distribution? I'm just not sure what the benefit is. It seems to be doing #1 is enough (and easiest, least amount of work) https://codereview.chromium.org/2086483006/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:923: if (process_type == kWebViewSingleProcessType) { On 2016/06/23 13:40:10, Torne wrote: > There's a problem here, which is not introduced by your change (it's already > wrong), but we should probably address it - I'm fairly sure that the special > handling of DumpWithoutCrashing needs to *always* be applied to WebView, not > just when it's running in single-process mode, which is currently what happens > here. You might want to check with Toby about this. If that's the case, then we > can't rely on process_type to know whether to do something special for webview > and would need to configure this in some other way. Added Toby to the CL. Toby, should SetDumpWithoutCrashingFunction be always called for webview instead of singleprocess webview only?
https://codereview.chromium.org/2086483006/diff/20001/components/crash/conten... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2086483006/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:923: if (process_type == kWebViewSingleProcessType) { On 2016/06/23 19:13:14, hush wrote: > On 2016/06/23 13:40:10, Torne wrote: > > There's a problem here, which is not introduced by your change (it's already > > wrong), but we should probably address it - I'm fairly sure that the special > > handling of DumpWithoutCrashing needs to *always* be applied to WebView, not > > just when it's running in single-process mode, which is currently what happens > > here. You might want to check with Toby about this. If that's the case, then > we > > can't rely on process_type to know whether to do something special for webview > > and would need to configure this in some other way. > > Added Toby to the CL. > Toby, should SetDumpWithoutCrashingFunction be always called for webview instead > of singleprocess webview only? We should definitely set this for both single and multiprocess webview. At the moment GSA really only care about getting minidumps on demand from the browser process, so although we need to work out what to do for the renderer at some point, doing this in single process and multiprocess-browser should be enough.
Description was changed from ========== Dump process type into breakpad. The process type can be "webview-singleprocess", "browser", or "renderer". BUG=616774 ========== to ========== Dump process type into breakpad. The process type can be "webview-singleprocess", "webview-multiprocess-browser", or "webview-multiprocess-renderer". BUG=616774 ==========
https://codereview.chromium.org/2086483006/diff/20001/components/crash/conten... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2086483006/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:104: const char kWebViewSingleProcessType[] = "webview-singleprocess"; On 2016/06/23 19:13:14, hush wrote: > On 2016/06/23 13:40:10, Torne wrote: > > On 2016/06/22 21:34:19, hush wrote: > > > I hope this change does not break anything that tries to strictly match > > > "webview" (like somewhere in the server side?) > > > > Since we don't upload the string currently I don't think this can break > anything > > on the server side. > > > > I'm not sure "webview-singleprocess" is a good string here, though. It seems > > like it would make more sense to either: > > 1) have *all* the webview process types identify that they're webview, for > the > > avoidance of doubt, or > > 2) have the string just be "singleprocess" or "single" or similar, and *also* > > use it for chrome if chrome is running with kSingleProcess set (and then > > possibly also change minidumps similarly)? > > > > I'd lean toward 2.. what do you think? > > Does chrome actually run in single process mode in any production distribution? > I'm just not sure what the benefit is. It seems to be doing #1 is enough (and > easiest, least amount of work) Anyway... I went with #1 in the next patch set. Let me know if you have any problems.. https://codereview.chromium.org/2086483006/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:923: if (process_type == kWebViewSingleProcessType) { On 2016/06/24 15:39:08, Tobias Sargeant wrote: > On 2016/06/23 19:13:14, hush wrote: > > On 2016/06/23 13:40:10, Torne wrote: > > > There's a problem here, which is not introduced by your change (it's already > > > wrong), but we should probably address it - I'm fairly sure that the special > > > handling of DumpWithoutCrashing needs to *always* be applied to WebView, not > > > just when it's running in single-process mode, which is currently what > happens > > > here. You might want to check with Toby about this. If that's the case, then > > we > > > can't rely on process_type to know whether to do something special for > webview > > > and would need to configure this in some other way. > > > > Added Toby to the CL. > > Toby, should SetDumpWithoutCrashingFunction be always called for webview > instead > > of singleprocess webview only? > > We should definitely set this for both single and multiprocess webview. At the > moment GSA really only care about getting minidumps on demand from the browser > process, so although we need to work out what to do for the renderer at some > point, doing this in single process and multiprocess-browser should be enough. All right. I uploaded new patch set with a todo in your name for the renderer process.
sgurun@chromium.org changed reviewers: + sgurun@chromium.org
https://codereview.chromium.org/2086483006/diff/40001/components/crash/conten... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2086483006/diff/40001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:104: // Keep these 3 string for webview in sync with the ones in aw_main_delegate.cc. please do not duplicate the consts, especially since it is easy to fix it here.
https://codereview.chromium.org/2086483006/diff/40001/android_webview/lib/mai... File android_webview/lib/main/aw_main_delegate.cc (right): https://codereview.chromium.org/2086483006/diff/40001/android_webview/lib/mai... android_webview/lib/main/aw_main_delegate.cc:51: const char kWebViewSingleProcessType[] = "webview-singleprocess"; These seem really long; bikeshedding a bit, but it seems like webview-single, webview-browser, webview-renderer is sufficient (the constants are named that way after all) :) https://codereview.chromium.org/2086483006/diff/40001/android_webview/lib/mai... android_webview/lib/main/aw_main_delegate.cc:176: process_type = kWebViewRendererProcessType; Having a couple of hardcoded cases where we change the process type from one string to another string seems weird - if we actually want all the webview process types to be prefixed with "webview" then why not just actually do that and prepend it to whatever the real process type is? I still think it'd be fine to just use the real process type as-is, though.
https://codereview.chromium.org/2086483006/diff/40001/android_webview/lib/mai... File android_webview/lib/main/aw_main_delegate.cc (right): https://codereview.chromium.org/2086483006/diff/40001/android_webview/lib/mai... android_webview/lib/main/aw_main_delegate.cc:51: const char kWebViewSingleProcessType[] = "webview-singleprocess"; On 2016/06/25 14:05:16, Torne wrote: > These seem really long; bikeshedding a bit, but it seems like webview-single, > webview-browser, webview-renderer is sufficient (the constants are named that > way after all) :) The problem with just "webview-browser" is only unambiguous when it appears together with the other 2 process types. A crash/ dashboard reader who does not read our code here will not know if this is for single process webview or webview browser process in multiprocess mode. Let's just be verbose and disperse any possible doubt.. https://codereview.chromium.org/2086483006/diff/40001/android_webview/lib/mai... android_webview/lib/main/aw_main_delegate.cc:176: process_type = kWebViewRendererProcessType; On 2016/06/25 14:05:16, Torne wrote: > Having a couple of hardcoded cases where we change the process type from one > string to another string seems weird - if we actually want all the webview > process types to be prefixed with "webview" then why not just actually do that > and prepend it to whatever the real process type is? I still think it'd be fine > to just use the real process type as-is, though. Just prepending "webview-" to process_type does not work very well because process_type for the browser process (single process or browser process in multiprocess mode) is an empty string. Then we get: webview- webview-renderer. This is not pretty. We need a way to tell the difference between single process and multiprocess. https://codereview.chromium.org/2086483006/diff/40001/components/crash/conten... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2086483006/diff/40001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:104: // Keep these 3 string for webview in sync with the ones in aw_main_delegate.cc. On 2016/06/24 23:57:57, sgurun wrote: > please do not duplicate the consts, especially since it is easy to fix it here. I put this section in the header file.
ping Torne?
On 2016/07/11 18:20:39, hush wrote: > ping Torne? It really seems simpler and more logical here to just leave the process types alone as much as possible - if it's the empty string, check if we're in single process mode and set it to either "browser" or "webview", or if you think the existing "webview" process type for single process isn't clear enough, "single"? I don't think having the webview- prefix adds anything, and it just makes the code more complicated (introducing the possibility of not always prefixing it if we have a new process type ever, etc).
On 2016/07/12 11:26:23, Torne wrote: > On 2016/07/11 18:20:39, hush wrote: > > ping Torne? > > It really seems simpler and more logical here to just leave the process types > alone as much as possible - if it's the empty string, check if we're in single > process mode and set it to either "browser" or "webview", or if you think the > existing "webview" process type for single process isn't clear enough, "single"? > > I don't think having the webview- prefix adds anything, and it just makes the > code more complicated (introducing the possibility of not always prefixing it if > we have a new process type ever, etc). alright. I just updated the process names to webview, browser, and renderer (which is the default) PTAL
lgtm % nit https://codereview.chromium.org/2086483006/diff/80001/components/crash/conten... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2086483006/diff/80001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:895: strdup(process_type.empty() ? "browser" : process_type.c_str()); Just a nit: It looks a bit weird having the remapping from empty to hardcoded "browser" here, but then having the webview codepath explicitly remap empty to a different, constant-defined "browser". Maybe just make the constant called kBrowserProcessType and use it in both places?
On 2016/07/14 10:29:09, Torne wrote: > lgtm % nit > > https://codereview.chromium.org/2086483006/diff/80001/components/crash/conten... > File components/crash/content/app/breakpad_linux.cc (right): > > https://codereview.chromium.org/2086483006/diff/80001/components/crash/conten... > components/crash/content/app/breakpad_linux.cc:895: strdup(process_type.empty() > ? "browser" : process_type.c_str()); > Just a nit: It looks a bit weird having the remapping from empty to hardcoded > "browser" here, but then having the webview codepath explicitly remap empty to a > different, constant-defined "browser". Maybe just make the constant called > kBrowserProcessType and use it in both places? Done
hush@chromium.org changed reviewers: + rsesek@chromium.org
Hello rsesek@ PTAL component/
What differentiates a crash report from webview from Chrome, if the process_type doesn't have a webview- prefix? I think that's an important distinction. Also, the CL description needs to be updated.
Description was changed from ========== Dump process type into breakpad. The process type can be "webview-singleprocess", "webview-multiprocess-browser", or "webview-multiprocess-renderer". BUG=616774 ========== to ========== Dump process type into breakpad. The process type can be "webview", "browser", or "renderer". BUG=616774 ==========
On 2016/07/14 19:02:24, Robert Sesek wrote: > What differentiates a crash report from webview from Chrome, if the process_type > doesn't have a webview- prefix? I think that's an important distinction. > > Also, the CL description needs to be updated. "Product, version" is the difference. See a sample crash report here. https://crash.corp.google.com/browse?q=product.name%3D%27AndroidWebView%27%20... And a sample chrome crash report: https://crash.corp.google.com/browse?q=product.name%3D%27Chrome_Android%27%20...
CL description updated.
lgtm
The CQ bit was checked by hush@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from torne@chromium.org Link to the patchset: https://codereview.chromium.org/2086483006/#ps100001 (title: "comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2016/07/14 19:02:24, Robert Sesek wrote: > What differentiates a crash report from webview from Chrome, if the process_type > doesn't have a webview- prefix? I think that's an important distinction. They come through a different submission pipeline and are filed under a different product entirely in crash. It's also possible to just look at the crashing app - if it's one of the Chrome packages it's Chrome, if it's not, it's WebView.
The CQ bit was checked by hush@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Dump process type into breakpad. The process type can be "webview", "browser", or "renderer". BUG=616774 ========== to ========== Dump process type into breakpad. The process type can be "webview", "browser", or "renderer". BUG=616774 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Dump process type into breakpad. The process type can be "webview", "browser", or "renderer". BUG=616774 ========== to ========== Dump process type into breakpad. The process type can be "webview", "browser", or "renderer". BUG=616774 Committed: https://crrev.com/e1a6b0a706a4badbe6131215da9e335d62ac5c93 Cr-Commit-Position: refs/heads/master@{#407869} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e1a6b0a706a4badbe6131215da9e335d62ac5c93 Cr-Commit-Position: refs/heads/master@{#407869} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
