Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2087413002:
Add process type to MicroDumpExtraInfo (Closed)
|
Created:
4 years, 6 months ago by hush (inactive) Modified:
4 years, 5 months ago CC:
android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/breakpad/breakpad/src.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd process type to MicroDumpExtraInfo
BUG=616774
R=primiano@chromium.org, torne@chromium.org
Committed: https://chromium.googlesource.com/breakpad/breakpad/+/5adeef6117c5577949438e1061b6894dfcbe7133
Patch Set 1 #
Total comments: 3
Patch Set 2 : spell #Patch Set 3 : rebase #
Messages
Total messages: 27 (4 generated)
Patchset #1 (id:1) has been deleted
hush@chromium.org changed reviewers: + torne@chromium.org
PTAL https://codereview.chromium.org/2087413002/diff/20001/client/linux/microdump_... File client/linux/microdump_writer/microdump_writer.cc (right): https://codereview.chromium.org/2087413002/diff/20001/client/linux/microdump_... client/linux/microdump_writer/microdump_writer.cc:238: LogAppend("P "); Hello Torne, do you know if this letter matter? I see we've already have V, O, A, L, G, S etc. Does anything tries to parse the string based on the first letter of each line?
Actually, hold on. This may not be the right place. We've already dumping the process type here: https://cs.chromium.org/chromium/src/components/crash/content/app/breakpad_li... Somehow webview gets an empty process type
On 2016/06/22 20:59:27, hush wrote: > Actually, hold on. This may not be the right place. > We've already dumping the process type here: > > https://cs.chromium.org/chromium/src/components/crash/content/app/breakpad_li... > Somehow webview gets an empty process type In fact, michalebai just told me that is the place for Chrome to upload its crash information. So yeah, this CL should be the correct place. The server side needs to know how to parse "P " for process type though.
LGTM % spelling https://codereview.chromium.org/2087413002/diff/20001/client/linux/microdump_... File client/linux/microdump_writer/microdump_writer.cc (right): https://codereview.chromium.org/2087413002/diff/20001/client/linux/microdump_... client/linux/microdump_writer/microdump_writer.cc:238: LogAppend("P "); On 2016/06/22 20:46:01, hush wrote: > Hello Torne, do you know if this letter matter? > I see we've already have V, O, A, L, G, S etc. Does anything tries to parse the > string based on the first letter of each line? Yes, the processor uses the letter to determine what data the line contains, since they aren't otherwise self-explaining. This should be fine. I believe it just ignores unknown lines, but you could check with Maria to be sure? https://codereview.chromium.org/2087413002/diff/20001/client/linux/microdump_... client/linux/microdump_writer/microdump_writer.cc:242: LogAppend("UNKOWN"); Missing an "N" :)
Hello Maria, WebView will add a line in its microdump indicating what the process type is. It begins with "P " and looks like: P webview-browser, P webview-renderer, P singleprocess P UNKNOWN Can you make some changes in the server side parsing script for this line?
Hello Hui, I filed b/29610934 to track this. Qq, is the code that actually populates the process type in the extra-info already in place? When do we expect it to go live? Thanks. On 2016/06/23 19:05:10, hush wrote: > Hello Maria, > WebView will add a line in its microdump indicating what the process type is. > It begins with "P " and looks like: > P webview-browser, > P webview-renderer, > P singleprocess > P UNKNOWN > Can you make some changes in the server side parsing script for this line?
On 2016/06/23 20:44:32, mmandlis wrote: > Hello Hui, > > I filed b/29610934 to track this. > Qq, is the code that actually populates the process type in the extra-info > already in place? When do we expect it to go live? > > Thanks. > > > On 2016/06/23 19:05:10, hush wrote: > > Hello Maria, > > WebView will add a line in its microdump indicating what the process type is. > > It begins with "P " and looks like: > > P webview-browser, > > P webview-renderer, > > P singleprocess > > P UNKNOWN > > Can you make some changes in the server side parsing script for this line? Not live yet. This is just a heads up. I'm not sure how quick this other CL can go https://codereview.chromium.org/2086483006/ I hope it's next week. :)
hush@chromium.org changed reviewers: + primiano@chromium.org
Hello Primiano, can you take a look?
On 2016/07/14 22:38:29, hush wrote: > Hello Primiano, can you take a look? And how do I land it?
I ran the tests with "make check" and they passed.
LGTM if necessary. Are you sure you cannot tell the process type from the stack trace?
> And how do I land it? Get a breakpad committer to do that. I think that me, toby and Maria are all committers. Make sure you roll in chrome after landing.
On 2016/07/15 18:23:40, Primiano Tucci wrote: > LGTM if necessary. > Are you sure you cannot tell the process type from the stack trace? You can tell the difference between renderer and browser, yes. But it's difficult to know if webview crashed in single process browser, or multiprocess browser just from the stack trace.
On 2016/07/15 18:24:24, Primiano Tucci wrote: > > And how do I land it? > Get a breakpad committer to do that. I think that me, toby and Maria are all > committers. > Make sure you roll in chrome after landing. Can you land it? Thanks Primiano!
> I ran the tests with "make check" and they passed. I ran the tests with "make check" and they failed. ... PASS: src/processor/minidump_stackwalk_test FAIL: src/processor/microdump_stackwalk_test Please fix the tests.
On 2016/07/18 15:15:51, primiano CORP (USE chromium) wrote: > > I ran the tests with "make check" and they passed. > I ran the tests with "make check" and they failed. > > ... > PASS: src/processor/minidump_stackwalk_test > FAIL: src/processor/microdump_stackwalk_test > > > Please fix the tests. hmm.... what's the failure log message? I synced, applied my change and ran them again and they still passed.
On 2016/07/18 17:53:09, hush wrote: > On 2016/07/18 15:15:51, primiano CORP (USE chromium) wrote: > > > I ran the tests with "make check" and they passed. > > I ran the tests with "make check" and they failed. > > > > ... > > PASS: src/processor/minidump_stackwalk_test > > FAIL: src/processor/microdump_stackwalk_test > > > > > > Please fix the tests. > > hmm.... what's the failure log message? I synced, applied my change and ran them > again and they still passed. I did the same. I did the same on an upstream breakpad checkout? Are you checking TOT (origin/master)? Probably you are just looking at the current branch. Do you have a full breakpad checkout, or are you running that from the chrome three?
On 2016/07/19 12:40:16, Primiano Tucci wrote: > On 2016/07/18 17:53:09, hush wrote: > > On 2016/07/18 15:15:51, primiano CORP (USE chromium) wrote: > > > > I ran the tests with "make check" and they passed. > > > I ran the tests with "make check" and they failed. > > > > > > ... > > > PASS: src/processor/minidump_stackwalk_test > > > FAIL: src/processor/microdump_stackwalk_test > > > > > > > > > Please fix the tests. > > > > hmm.... what's the failure log message? I synced, applied my change and ran > them > > again and they still passed. > > I did the same. I did the same on an upstream breakpad checkout? Are you > checking TOT (origin/master)? Probably you are just looking at the current > branch. > Do you have a full breakpad checkout, or are you running that from the chrome > three? I have a full breakpad checkout that is separate from the one in Chrome (the one in chrome tree can't make and run tests actually) And yes, I rebased on top of origin/master
On 2016/07/19 16:11:35, hush wrote: > On 2016/07/19 12:40:16, Primiano Tucci wrote: > > On 2016/07/18 17:53:09, hush wrote: > > > On 2016/07/18 15:15:51, primiano CORP (USE chromium) wrote: > > > > > I ran the tests with "make check" and they passed. > > > > I ran the tests with "make check" and they failed. > > > > > > > > ... > > > > PASS: src/processor/minidump_stackwalk_test > > > > FAIL: src/processor/microdump_stackwalk_test > > > > > > > > > > > > Please fix the tests. > > > > > > hmm.... what's the failure log message? I synced, applied my change and ran > > them > > > again and they still passed. > > > > I did the same. I did the same on an upstream breakpad checkout? Are you > > checking TOT (origin/master)? Probably you are just looking at the current > > branch. > > Do you have a full breakpad checkout, or are you running that from the chrome > > three? > > I have a full breakpad checkout that is separate from the one in Chrome (the one > in chrome tree can't make and run tests actually) > And yes, I rebased on top of origin/master How are you running checks? I just do literally: $ ./configure $ make check and my head is commit 965424f183596292e7df89404f16c420bdcdf9e1 Author: John Budorick <jbudorick@chromium.org> Date: Fri Jul 15 20:49:44 2016 [Android] Guard some NDK workarounds by major version. BUG=599327 R=mark@chromium.org Review URL: https://codereview.chromium.org/2152153003 .
On 2016/07/19 16:14:21, Primiano Tucci wrote: > On 2016/07/19 16:11:35, hush wrote: > > On 2016/07/19 12:40:16, Primiano Tucci wrote: > > > On 2016/07/18 17:53:09, hush wrote: > > > > On 2016/07/18 15:15:51, primiano CORP (USE chromium) wrote: > > > > > > I ran the tests with "make check" and they passed. > > > > > I ran the tests with "make check" and they failed. > > > > > > > > > > ... > > > > > PASS: src/processor/minidump_stackwalk_test > > > > > FAIL: src/processor/microdump_stackwalk_test > > > > > > > > > > > > > > > Please fix the tests. > > > > > > > > hmm.... what's the failure log message? I synced, applied my change and > ran > > > them > > > > again and they still passed. > > > > > > I did the same. I did the same on an upstream breakpad checkout? Are you > > > checking TOT (origin/master)? Probably you are just looking at the current > > > branch. > > > Do you have a full breakpad checkout, or are you running that from the > chrome > > > three? > > > > I have a full breakpad checkout that is separate from the one in Chrome (the > one > > in chrome tree can't make and run tests actually) > > And yes, I rebased on top of origin/master > > How are you running checks? > I just do literally: > $ ./configure > $ make check > > and my head is > commit 965424f183596292e7df89404f16c420bdcdf9e1 > Author: John Budorick <mailto:jbudorick@chromium.org> > Date: Fri Jul 15 20:49:44 2016 > > [Android] Guard some NDK workarounds by major version. > > BUG=599327 > mailto:R=mark@chromium.org > > Review URL: https://codereview.chromium.org/2152153003 . I just followed the README. So I did: ./configure && make && make check. And my current head is commit 0ebdc4a10a506e2a4a3a039c479b40219a84b760 Author: Thomas Zimmermann <tzimmermann@mozilla.com> Date: Tue Jul 19 17:00:51 2016 +0100 Don't define |r_debug| and |link_map| on Android releases 21 and later NDKs for Android 21 and later have the data structures |r_debug| and |link_map| defined in their header files. Defining them multiple times generates a compiler error. This patch protects both data structures from definition on Android 21 and later. BUG=629088 R=rmcilroy@chromium.org Review URL: https://codereview.chromium.org/2156173002 . Patch from Thomas Zimmermann <tzimmermann@mozilla.com>.
> > How are you running checks? > > I just do literally: > > $ ./configure > > $ make check All that seems right. I updated and am at 0ebdc4a10a506e2a4a3a039c479b40219a84b760 as well now. THe funny thing is that I just realized that that check fails even without your CL. So it feels that master is broken already :/. --- ./src/processor/testdata/microdump.stackwalk-arm.out 2016-03-08 08:42:06.872626674 +0000 +++ - 2016-07-19 17:20:01.851334612 +0100 @@ -60,4 +60,4 @@ 0xf6fee000 - 0xf6ff1fff libstdc++.so ??? 0xf6ff2000 - 0xf700afff libm.so ??? 0xf700c000 - 0xf7012fff liblog.so ??? -0xf7014000 - 0xf706dfff libc.so ??? (WARNING: No symbols, libc.so, 167F187B09A27F7444EF989603AAFD3D0) +0xf7014000 - 0xf706dfff libc.so ??? (WARNING: Corrupt symbols, libc.so, 167F187B09A27F7444EF989603AAFD3D0) FAIL src/processor/microdump_stackwalk_test (exit status: 1)
Description was changed from ========== Add process type to MicroDumpExtraInfo BUG=616774 ========== to ========== Add process type to MicroDumpExtraInfo BUG=616774 R=primiano@chromium.org, torne@chromium.org Committed: https://chromium.googlesource.com/breakpad/breakpad/+/5adeef6117c5577949438e1... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) manually as 5adeef6117c5577949438e1061b6894dfcbe7133 (presubmit successful).
Message was sent while issue was closed.
OK looks like the failure is consistenly locally to only my machine. will investigate separately and land this. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
