Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1579723002:
Add a file observer to monitor if there is a crash (Closed)
DescriptionAdd a file observer to monitor if there is a crash
It detects MOVE_TO for child processes.
BUG=561064
Committed: https://crrev.com/f9572cd88e31de59067379933bb3e51d796567eb
Cr-Commit-Position: refs/heads/master@{#373872}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Add check for if chrome is running on an experiment #
Total comments: 4
Patch Set 3 : do not use file observer for browser crash, and move checking for experiment to handleTabCrash #
Total comments: 2
Patch Set 4 : call "start watching" static method instead of calling the startWatching method from an instance #
Total comments: 6
Patch Set 5 : nit addressed #
Total comments: 21
Patch Set 6 : address yfriedma's comments #Patch Set 7 : fix the experiment code; use PathUtils to get the cache directory #Patch Set 8 : do not add the file observer as a singleton but as a regular instance #
Total comments: 4
Patch Set 9 : use AsyncTask to instantiate the file observer #
Total comments: 1
Patch Set 10 : #
Messages
Total messages: 43 (13 generated)
mlliu@chromium.org changed reviewers: + acleung@chromium.org, sievers@chromium.org, yfriedman@chromium.org
Please review this code change which adds a FileObserver to monitor if there is a minidump to upload. A brief design doc can be found at https://docs.google.com/document/d/140SeLzT76JuVJXSGnaWItQFumWx91JfEhZ5u_vXx46o/
https://codereview.chromium.org/1579723002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java (right): https://codereview.chromium.org/1579723002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:21: * This class is not thread-safe and must only be used on the UI thread. This is a bit confusing, since onEvent() actually happens on a 'special FileObserver thread'. But maybe nobody really needs to worry about this outside this class. In fact, we probably don't even need getInstance(), maybe a static startWatching() method is enough? https://codereview.chromium.org/1579723002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:45: FileObserver.CREATE | FileObserver.MOVED_TO); re CREATE: how do we avoid trying to upload incomplete files?
https://codereview.chromium.org/1579723002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java (right): https://codereview.chromium.org/1579723002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:21: * This class is not thread-safe and must only be used on the UI thread. On 2016/01/11 20:10:51, sievers wrote: > This is a bit confusing, since onEvent() actually happens on a 'special > FileObserver thread'. > > But maybe nobody really needs to worry about this outside this class. In fact, > we probably don't even need getInstance(), maybe a static startWatching() method > is enough? I will remove this line of comment. And yes, getInstance() is unnecessary, and can be removed as well. https://codereview.chromium.org/1579723002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:45: FileObserver.CREATE | FileObserver.MOVED_TO); On 2016/01/11 20:10:51, sievers wrote: > re CREATE: how do we avoid trying to upload incomplete files? Good point. To avoid uploading incomplete files, we can use CLOSE_WRITE instead of CREATE.
https://codereview.chromium.org/1579723002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java (right): https://codereview.chromium.org/1579723002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:36: private static final Pattern MINIDUMP_MIME_FIRST_TRY_PATTERN = Do we ever see a file without the MIME contents? In other words: Is .dmp the one without MIME content? Do we ever see it? https://codereview.chromium.org/1579723002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:85: public static boolean isMinidumpMIMEFirstTry(String path) { How is this different than: return MINIDUMP_MIME_FIRST_TRY_PATTERN.matcher(path).find()? https://codereview.chromium.org/1579723002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java (right): https://codereview.chromium.org/1579723002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:48: /* When a miniudump is detected, upload it to Google crash server */ Should it be '/**' instead? https://codereview.chromium.org/1579723002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (left): https://codereview.chromium.org/1579723002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2434: * Performs any subclass-specific tasks when the Tab crashes. Do you think we should create an experiment first? https://codereview.chromium.org/1579723002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2451: RecordUserAction.record("MobileBreakpadUploadAttempt"); We should probably keep this logic.
https://codereview.chromium.org/1579723002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java (right): https://codereview.chromium.org/1579723002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:36: private static final Pattern MINIDUMP_MIME_FIRST_TRY_PATTERN = On 2016/01/11 21:58:42, acleung1 wrote: > Do we ever see a file without the MIME contents? > > In other words: Is .dmp the one without MIME content? Do we ever see it? Yes. .dmp will be first created and then the MIME file. If we match by MINIDUMP_FIRST_TRY_PATTERN, the file observer will first detect the .dmp file and then the MIME file https://codereview.chromium.org/1579723002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/crash/CrashFileManager.java:85: public static boolean isMinidumpMIMEFirstTry(String path) { On 2016/01/11 21:58:42, acleung1 wrote: > How is this different than: > > return MINIDUMP_MIME_FIRST_TRY_PATTERN.matcher(path).find()? It's the same. I will modify the code. https://codereview.chromium.org/1579723002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java (right): https://codereview.chromium.org/1579723002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:48: /* When a miniudump is detected, upload it to Google crash server */ On 2016/01/11 21:58:43, acleung1 wrote: > Should it be '/**' instead? Yes. https://codereview.chromium.org/1579723002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (left): https://codereview.chromium.org/1579723002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2434: * Performs any subclass-specific tasks when the Tab crashes. On 2016/01/11 21:58:43, acleung1 wrote: > Do you think we should create an experiment first? Yes. I will create an experiment. https://codereview.chromium.org/1579723002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2451: RecordUserAction.record("MobileBreakpadUploadAttempt"); On 2016/01/11 21:58:43, acleung1 wrote: > We should probably keep this logic. If this logic is kept, MinidumpUploadService will try to upload the minidump again, and it won't be able to find any minidump at that time because the file was uploaded when MinidumpUploadService received intent from MinidumpDirectoryObserver. This is not wrong though
mlliu@chromium.org changed reviewers: + nikunjb@google.com
new patch uploaded. Please review. Thanks! ps: The CL of adding experiment for Canary and Dev is cl/112738873
https://codereview.chromium.org/1579723002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java (right): https://codereview.chromium.org/1579723002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:45: FileObserver.CLOSE_WRITE | FileObserver.MOVED_TO); so can we just not handle browser crashes here and avoid nasty races? https://codereview.chromium.org/1579723002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java (right): https://codereview.chromium.org/1579723002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:143: mTab.handleTabCrash(); Is it intentional to bypass the UMA recording in that routine? Either way, the intentions are probably more obvious if you pull the bypass into Tab.java.
Patchset #3 (id:40001) has been deleted
new patch uploaded https://codereview.chromium.org/1579723002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java (right): https://codereview.chromium.org/1579723002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:45: FileObserver.CLOSE_WRITE | FileObserver.MOVED_TO); On 2016/01/22 19:09:33, sievers wrote: > so can we just not handle browser crashes here and avoid nasty races? Yes, I removed FileObserver.CLOSE_WRITE https://codereview.chromium.org/1579723002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java (right): https://codereview.chromium.org/1579723002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:143: mTab.handleTabCrash(); On 2016/01/22 19:09:33, sievers wrote: > Is it intentional to bypass the UMA recording in that routine? > Either way, the intentions are probably more obvious if you pull the bypass into > Tab.java. Done.
new patch uploaded. Please review. Thanks!
On 2016/01/25 23:04:40, Menglin wrote: > new patch uploaded. Please review. Thanks! LGTM.
https://codereview.chromium.org/1579723002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java (right): https://codereview.chromium.org/1579723002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:348: MinidumpDirectoryObserver.getInstance().startWatching(); nit: can you add a static method in MinidumpDirectoryObserver to start it so that we actually don't have to expose a pointer to the instance at all? (Just because that thing operates autonomously and on multiple threads, it'd be nice to kid of keep people from adding other public methods in the future, or at least force them to think carefully about it.)
New patch uploaded. Please review. Thanks! https://codereview.chromium.org/1579723002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java (right): https://codereview.chromium.org/1579723002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:348: MinidumpDirectoryObserver.getInstance().startWatching(); On 2016/01/27 19:11:10, sievers wrote: > nit: can you add a static method in MinidumpDirectoryObserver to start it so > that we actually don't have to expose a pointer to the instance at all? (Just > because that thing operates autonomously and on multiple threads, it'd be nice > to kid of keep people from adding other public methods in the future, or at > least force them to think carefully about it.) Done. Thanks for the comment!
lgtm https://codereview.chromium.org/1579723002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java (right): https://codereview.chromium.org/1579723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:39: // The file observer detects MOVED_TO for renderer crash and GPU crash; nit: I'd just say 'child processes'. There's also the utility process and making assumptions about the specific types is basically what your patch is fixing :) https://codereview.chromium.org/1579723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:49: public void onEvent(int event, String path) { nit: Can you put a comment that this is happening on an off-thread so that nobody adds some code here that's not thread-safe? https://codereview.chromium.org/1579723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:54: RecordUserAction.record("MobileBreakpadUploadAttempt"); This stat is still a bit broken, since the name suggests that we'd want to bump it with every try. But then recording UMA here requires native which means this code shouldn't be in the upload service (since it would crash on intent redelivery where native is not loaded).
https://codereview.chromium.org/1579723002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java (right): https://codereview.chromium.org/1579723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:39: // The file observer detects MOVED_TO for renderer crash and GPU crash; On 2016/01/29 23:52:34, sievers wrote: > nit: I'd just say 'child processes'. There's also the utility process and making > assumptions about the specific types is basically what your patch is fixing :) Done. https://codereview.chromium.org/1579723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:49: public void onEvent(int event, String path) { On 2016/01/29 23:52:34, sievers wrote: > nit: Can you put a comment that this is happening on an off-thread so that > nobody adds some code here that's not thread-safe? Done. https://codereview.chromium.org/1579723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:54: RecordUserAction.record("MobileBreakpadUploadAttempt"); On 2016/01/29 23:52:34, sievers wrote: > This stat is still a bit broken, since the name suggests that we'd want to bump > it with every try. > But then recording UMA here requires native which means this code shouldn't be > in the upload service (since it would crash on intent redelivery where native is > not loaded). I will leave it as it is for now and fix that in a separate cl. Thanks
The CQ bit was checked by mlliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from acleung@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1579723002/#ps100001 (title: "nit addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1579723002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1579723002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
mlliu@chromium.org changed reviewers: - acleung@chromium.org, nikunjb@google.com, sievers@chromium.org
Hi Yaron, could you kindly review and lgtm this code change? acleung and sievers have given it lgtm. Thanks Menglin
https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java (right): https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:31: ThreadUtils.assertOnUiThread(); why? Do you just need a stable thread to avoid locking? https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:40: super(new File(sContext.getCacheDir(), CrashFileManager.CRASH_DUMP_DIR).toString(), This can cause a StrictMode violation since internally it'll do file.exists and create it if it didn't exist previously. https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:41: FileObserver.MOVED_TO); Is this CL description out of date? I don't see a mention of create https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:45: * When a miniudump is detected, upload it to Google crash server minidump https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:49: // This is happening on an off-thread. What's an "off-thread"? How about: "This is executed on a thread dedicated to FileObserver" https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:52: sContext.startService(intent); You should try/catch this for a security exception, same as the previous location https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2467: if (TextUtils.equals("false", VariationsAssociatedData.getVariationParamValue( I don't think this is right - the function said it returns an empty string if the experiment isn't defined or field is missing. This means that if you don't have a field trial, we suddenly stop uploading anything. I think this should be: if (!TextUtils.equals("true"), ...)
Description was changed from ========== Add a file observer to monitor if there is a crash The file observer is added as a singleton. It detects MOVE_TO for renderer/gpu crashes. It detects CREATE for browser crashes. BUG=561064 ========== to ========== Add a file observer to monitor if there is a crash The file observer is added as a singleton. It detects MOVE_TO for child processes. BUG=561064 ==========
Hi Yaron, new patch uploaded. Please review when you have a chance. Thanks Menglin https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java (right): https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:31: ThreadUtils.assertOnUiThread(); On 2016/02/01 20:22:40, Yaron wrote: > why? Do you just need a stable thread to avoid locking? Done. https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:40: super(new File(sContext.getCacheDir(), CrashFileManager.CRASH_DUMP_DIR).toString(), On 2016/02/01 20:22:40, Yaron wrote: > This can cause a StrictMode violation since internally it'll do file.exists and > create it if it didn't exist previously. I removed the assertOnUiThread(), does that resolve the violation? https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:41: FileObserver.MOVED_TO); On 2016/02/01 20:22:40, Yaron wrote: > Is this CL description out of date? I don't see a mention of create Done. https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:45: * When a miniudump is detected, upload it to Google crash server On 2016/02/01 20:22:40, Yaron wrote: > minidump Done. https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:49: // This is happening on an off-thread. On 2016/02/01 20:22:40, Yaron wrote: > What's an "off-thread"? How about: > "This is executed on a thread dedicated to FileObserver" Done. https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:52: sContext.startService(intent); On 2016/02/01 20:22:40, Yaron wrote: > You should try/catch this for a security exception, same as the previous > location Done.
https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java (right): https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:31: ThreadUtils.assertOnUiThread(); On 2016/02/02 02:11:29, Menglin wrote: > On 2016/02/01 20:22:40, Yaron wrote: > > why? Do you just need a stable thread to avoid locking? > > Done. Does this really need to be a singleton and have the overhead of the various statics? Why can't you just new it up once and let it run? https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:40: super(new File(sContext.getCacheDir(), CrashFileManager.CRASH_DUMP_DIR).toString(), On 2016/02/02 02:11:29, Menglin wrote: > On 2016/02/01 20:22:40, Yaron wrote: > > This can cause a StrictMode violation since internally it'll do file.exists > and > > create it if it didn't exist previously. > > I removed the assertOnUiThread(), does that resolve the violation? No, because it's still called from the UI thread. The call to getCacheDir needs to not be done from the UI Thread. I think you can use PathUtils.getCacheDirectory instead. https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2467: if (TextUtils.equals("false", VariationsAssociatedData.getVariationParamValue( On 2016/02/01 20:22:40, Yaron wrote: > I don't think this is right - the function said it returns an empty string if > the experiment isn't defined or field is missing. This means that if you don't > have a field trial, we suddenly stop uploading anything. > > I think this should be: if (!TextUtils.equals("true"), ...) I don't see this addressed
Hi Yaron, Thank you very much for your comments! I've addressed all those. ptal when you have a chance Menglin https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java (right): https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:31: ThreadUtils.assertOnUiThread(); On 2016/02/03 21:51:42, Yaron wrote: > On 2016/02/02 02:11:29, Menglin wrote: > > On 2016/02/01 20:22:40, Yaron wrote: > > > why? Do you just need a stable thread to avoid locking? > > > > Done. > > Does this really need to be a singleton and have the overhead of the various > statics? Why can't you just new it up once and let it run? I was not sure so I manually tested on both. case1: not a singleton step1: open the browser, induce renderer crash step2: open a second tab, induce renderer crash what to expect: detects a minidump after step1 and another minidump after step2 what happened: detects a minidump after step1, but wasn't able to detect the second minidump after step2 case2: singleton same steps as above. what happened: was able to detect each minidump. I suspect the file observer some how "died" with the child process if it's not a singleton, so it was able to detect the following minidumps https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:40: super(new File(sContext.getCacheDir(), CrashFileManager.CRASH_DUMP_DIR).toString(), On 2016/02/03 21:51:42, Yaron wrote: > On 2016/02/02 02:11:29, Menglin wrote: > > On 2016/02/01 20:22:40, Yaron wrote: > > > This can cause a StrictMode violation since internally it'll do file.exists > > and > > > create it if it didn't exist previously. > > > > I removed the assertOnUiThread(), does that resolve the violation? > > No, because it's still called from the UI thread. The call to getCacheDir needs > to not be done from the UI Thread. I think you can use > PathUtils.getCacheDirectory instead. Done. https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2467: if (TextUtils.equals("false", VariationsAssociatedData.getVariationParamValue( On 2016/02/03 21:51:42, Yaron wrote: > On 2016/02/01 20:22:40, Yaron wrote: > > I don't think this is right - the function said it returns an empty string if > > the experiment isn't defined or field is missing. This means that if you don't > > have a field trial, we suddenly stop uploading anything. > > > > I think this should be: if (!TextUtils.equals("true"), ...) > > I don't see this addressed Yes. You are right.
https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java (right): https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:31: ThreadUtils.assertOnUiThread(); On 2016/02/04 00:17:23, Menglin wrote: > On 2016/02/03 21:51:42, Yaron wrote: > > On 2016/02/02 02:11:29, Menglin wrote: > > > On 2016/02/01 20:22:40, Yaron wrote: > > > > why? Do you just need a stable thread to avoid locking? > > > > > > Done. > > > > Does this really need to be a singleton and have the overhead of the various > > statics? Why can't you just new it up once and let it run? > > I was not sure so I manually tested on both. > > case1: not a singleton > step1: open the browser, induce renderer crash > step2: open a second tab, induce renderer crash > what to expect: detects a minidump after step1 and another minidump after step2 > what happened: detects a minidump after step1, but wasn't able to detect the > second minidump after step2 > > case2: singleton > same steps as above. > what happened: was able to detect each minidump. > > I suspect the file observer some how "died" with the child process if it's not a > singleton, so it was able to detect the following minidumps IT really shouldn't. Can you upload a patchset with the code? Were you maintain a reference to the instance of MinidumpDirectoryObserver cause if not, it would get GC'd since FileObserver only maintains a weak reference. That would mean it's just racey. I really would prefer clear ownership though to this being static
Hi Yaron, Thank you for your comments. Please review when you have a chance. Thanks! Menglin https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java (right): https://codereview.chromium.org/1579723002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpDirectoryObserver.java:31: ThreadUtils.assertOnUiThread(); On 2016/02/04 01:50:26, Yaron wrote: > On 2016/02/04 00:17:23, Menglin wrote: > > On 2016/02/03 21:51:42, Yaron wrote: > > > On 2016/02/02 02:11:29, Menglin wrote: > > > > On 2016/02/01 20:22:40, Yaron wrote: > > > > > why? Do you just need a stable thread to avoid locking? > > > > > > > > Done. > > > > > > Does this really need to be a singleton and have the overhead of the various > > > statics? Why can't you just new it up once and let it run? > > > > I was not sure so I manually tested on both. > > > > case1: not a singleton > > step1: open the browser, induce renderer crash > > step2: open a second tab, induce renderer crash > > what to expect: detects a minidump after step1 and another minidump after > step2 > > what happened: detects a minidump after step1, but wasn't able to detect the > > second minidump after step2 > > > > case2: singleton > > same steps as above. > > what happened: was able to detect each minidump. > > > > I suspect the file observer some how "died" with the child process if it's not > a > > singleton, so it was able to detect the following minidumps > > IT really shouldn't. Can you upload a patchset with the code? Were you maintain > a reference to the instance of MinidumpDirectoryObserver cause if not, it would > get GC'd since FileObserver only maintains a weak reference. That would mean > it's just racey. I really would prefer clear ownership though to this being > static Now MinidumpDirectoryObserver is not a singleton, but the instance is maintained in ChromeBrowserInitializer. I tested it, and it worked out correctly. Thank you for your comment!
https://codereview.chromium.org/1579723002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java (right): https://codereview.chromium.org/1579723002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:65: private MinidumpDirectoryObserver mMinidumpDirectoryObserver = new MinidumpDirectoryObserver(); This is still a strict mode violation. It's instantiated when the constructor for CBI is called. That's currently done on the UI thread. https://codereview.chromium.org/1579723002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:352: mMinidumpDirectoryObserver.startWatching(); kick off a new async task here, something like this; new AsyncTask<doInBackground, return new AsyncTask<Void, MinidumpDirectoryObserver, Void>() { @Override doInBackground(Void...) { return new Minidump.. } onPostExecute(Minidump..) { mMini.. = minidump; mMini.startWatching() ..
new patch uploaded. PTAL. Thanks https://codereview.chromium.org/1579723002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java (right): https://codereview.chromium.org/1579723002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:65: private MinidumpDirectoryObserver mMinidumpDirectoryObserver = new MinidumpDirectoryObserver(); On 2016/02/04 22:37:33, Yaron wrote: > This is still a strict mode violation. It's instantiated when the constructor > for CBI is called. That's currently done on the UI thread. Done. https://codereview.chromium.org/1579723002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:352: mMinidumpDirectoryObserver.startWatching(); On 2016/02/04 22:37:33, Yaron wrote: > kick off a new async task here, something like this; > new AsyncTask<doInBackground, return new AsyncTask<Void, > MinidumpDirectoryObserver, Void>() { > @Override > doInBackground(Void...) { > return new Minidump.. > } > onPostExecute(Minidump..) { > mMini.. = minidump; > mMini.startWatching() > .. Done.
lgtm, thanks https://codereview.chromium.org/1579723002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java (right): https://codereview.chromium.org/1579723002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:366: }.execute(); Err and then this should be executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR); cause we don't need to synchronize on anything and may as well use the thread pool
Description was changed from ========== Add a file observer to monitor if there is a crash The file observer is added as a singleton. It detects MOVE_TO for child processes. BUG=561064 ========== to ========== Add a file observer to monitor if there is a crash It detects MOVE_TO for child processes. BUG=561064 ==========
The CQ bit was checked by mlliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from acleung@chromium.org, sievers@chromium.org, yfriedman@chromium.org Link to the patchset: https://codereview.chromium.org/1579723002/#ps200001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1579723002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1579723002/200001
Message was sent while issue was closed.
Description was changed from ========== Add a file observer to monitor if there is a crash It detects MOVE_TO for child processes. BUG=561064 ========== to ========== Add a file observer to monitor if there is a crash It detects MOVE_TO for child processes. BUG=561064 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Add a file observer to monitor if there is a crash It detects MOVE_TO for child processes. BUG=561064 ========== to ========== Add a file observer to monitor if there is a crash It detects MOVE_TO for child processes. BUG=561064 Committed: https://crrev.com/f9572cd88e31de59067379933bb3e51d796567eb Cr-Commit-Position: refs/heads/master@{#373872} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/f9572cd88e31de59067379933bb3e51d796567eb Cr-Commit-Position: refs/heads/master@{#373872} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
