Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2562283004:
[Offline Pages] Use async log writing to avoid violating StrictMode. (Closed)
|
Created:
4 years ago by romax Modified:
4 years ago Reviewers:
dewittj CC:
chromium-reviews, dewittj+watch_chromium.org, fgorski+watch_chromium.org, romax+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, agrieve+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline Pages] Use async log writing to avoid violating StrictMode.
Fixed the crash caused by logging to files on UI thread and violating
StrictMode by changing log to AsyncTask.
BUG=673450
Committed: https://crrev.com/7fc9052b9a898a37d8a191522ee8c0a2ce86fa20
Cr-Commit-Position: refs/heads/master@{#438896}
Patch Set 1 #
Total comments: 2
Patch Set 2 : comments from dewittj. #Patch Set 3 : adding missing dot. #
Total comments: 6
Patch Set 4 : More comments. #Messages
Total messages: 18 (9 generated)
romax@chromium.org changed reviewers: + dewittj@chromium.org
PTAL, thanks!
https://codereview.chromium.org/2562283004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java (right): https://codereview.chromium.org/2562283004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java:188: mLogOutput = new FileWriter(outputFile); I believe this should also be done on the background thread, since it opens a file. Also, access to mLogOutput may need to be synchronized if you are using a thread pool.
addressed comment, PTAL! https://codereview.chromium.org/2562283004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java (right): https://codereview.chromium.org/2562283004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java:188: mLogOutput = new FileWriter(outputFile); On 2016/12/12 21:07:11, dewittj wrote: > I believe this should also be done on the background thread, since it opens a > file. Also, access to mLogOutput may need to be synchronized if you are using a > thread pool. Actaully the test thread is not the same as Chrome UI thread as I just checked... However I changed to SERIAL_EXECUTOR since log would be serialized eventually... And yes I missed the point to sync operations on thread pool. Thanks for pointing out!
https://codereview.chromium.org/2562283004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java (right): https://codereview.chromium.org/2562283004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java:74: protected synchronized Void doInBackground(String... strings) { I don't think this is right, since every log call will be run using a new LogTask. I think what you want is: protected Void doInBackground(String... strings) { try { synchronized(mLogOutput) { ... } } catch { ... } return null; } https://codereview.chromium.org/2562283004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java:77: mLogOutput.flush(); why flush? https://codereview.chromium.org/2562283004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java:187: public void setLogOutputFile(File outputFile) throws IOException { I'd add a comment and an assert here about the thread requirements, rather than an assert in the test.
addressed comments. PTAL https://codereview.chromium.org/2562283004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java (right): https://codereview.chromium.org/2562283004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java:74: protected synchronized Void doInBackground(String... strings) { On 2016/12/12 23:41:52, dewittj wrote: > I don't think this is right, since every log call will be run using a new > LogTask. I think what you want is: > > protected Void doInBackground(String... strings) { > try { > synchronized(mLogOutput) { > ... > } > } catch { > ... > } > return null; > } Done. https://codereview.chromium.org/2562283004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java:77: mLogOutput.flush(); On 2016/12/12 23:41:52, dewittj wrote: > why flush? I think it might be better to have part of the logs even if the test crashed later... https://codereview.chromium.org/2562283004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java:187: public void setLogOutputFile(File outputFile) throws IOException { On 2016/12/12 23:41:52, dewittj wrote: > I'd add a comment and an assert here about the thread requirements, rather than > an assert in the test. Done.
lgtm, thanks
The CQ bit was checked by romax@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by romax@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481830814091790,
"parent_rev": "33605e75b9b3bd10929b17965df5bdcfd9461a59", "commit_rev":
"f798db328830103930f23ee1ab19ba1d119da1ec"}
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Use async log writing to avoid violating StrictMode. Fixed the crash caused by logging to files on UI thread and violating StrictMode by changing log to AsyncTask. BUG=673450 ========== to ========== [Offline Pages] Use async log writing to avoid violating StrictMode. Fixed the crash caused by logging to files on UI thread and violating StrictMode by changing log to AsyncTask. BUG=673450 Review-Url: https://codereview.chromium.org/2562283004 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Use async log writing to avoid violating StrictMode. Fixed the crash caused by logging to files on UI thread and violating StrictMode by changing log to AsyncTask. BUG=673450 Review-Url: https://codereview.chromium.org/2562283004 ========== to ========== [Offline Pages] Use async log writing to avoid violating StrictMode. Fixed the crash caused by logging to files on UI thread and violating StrictMode by changing log to AsyncTask. BUG=673450 Committed: https://crrev.com/7fc9052b9a898a37d8a191522ee8c0a2ce86fa20 Cr-Commit-Position: refs/heads/master@{#438896} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7fc9052b9a898a37d8a191522ee8c0a2ce86fa20 Cr-Commit-Position: refs/heads/master@{#438896} |
