Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2630473004:
Implement Crash Handle API (Closed)
|
Created:
3 years, 11 months ago by michaelbai Modified:
3 years, 11 months ago CC:
chromium-reviews, android-webview-reviews_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement Crash Handle API
This patch implements basical feature for crash handle API.
The feature like 'render processs keep crashing after crash is handled' hasn't implemented.
BUG=666106
Review-Url: https://codereview.chromium.org/2630473004
Cr-Commit-Position: refs/heads/master@{#445256}
Committed: https://chromium.googlesource.com/chromium/src/+/6e10ee5b759fbe79d5e24096a337e9534012bbc8
Patch Set 1 #
Total comments: 30
Patch Set 2 : address comments #Patch Set 3 : Implement Crash Handle API #Patch Set 4 : use LOG(FATAL) #
Total comments: 4
Patch Set 5 : Move WebView no operation change to another patch #
Messages
Total messages: 32 (7 generated)
Description was changed from ========== Implement Crash Handle API This patch implements basical feature for crash handle API. The feature like 'render processs keep crashing after crash is handled' hasn't implemented. BUG=666106 ========== to ========== Implement Crash Handle API This patch implements basical feature for crash handle API. The feature like 'render processs keep crashing after crash is handled' hasn't implemented. BUG=666106 ==========
michaelbai@chromium.org changed reviewers: + boliu@chromium.org, sgurun@chromium.org, tobiasjs@chromium.org, torne@chromium.org
PTAL https://codereview.chromium.org/2630473004/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2630473004/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContents.java:1178: // process as detail. It doesn't have to be done in this patch, I will send email for further discussion.
boliu@chromium.org changed reviewers: - boliu@chromium.org
-me, enough reviewers already :)
https://codereview.chromium.org/2630473004/diff/1/android_webview/browser/aw_... File android_webview/browser/aw_browser_terminator.cc (right): https://codereview.chromium.org/2630473004/diff/1/android_webview/browser/aw_... android_webview/browser/aw_browser_terminator.cc:55: void OnRenderProcessGone(int child_process_id) { I think it would be good to name this and the below in a less similar manner to make it clear that this is providing a notification that the delegates will soon receive the termination reason. Otherwise (to me, at least) it looks like these are either/or methods. Also, what guarantee do we have that the list of delegates that we get at this point is identical to the list below? What if an AwContents is destroyed in the meantime (or, more confusingly, created. will the new AwContents cause the creation of a new renderer)? https://codereview.chromium.org/2630473004/diff/1/android_webview/browser/aw_... android_webview/browser/aw_browser_terminator.cc:126: OnRenderProcessGone(child_process_id); Don't do this here; OnChildExit can be called more than once per child (unavoidable because of the way the chrome child process notification system works) so you need to move it to after the early-out when we don't find a pipe. https://codereview.chromium.org/2630473004/diff/1/android_webview/browser/aw_... android_webview/browser/aw_browser_terminator.cc:141: return; You may need to organise for the *WithDetail delegate method to be called in this case, although I think there's actually no such thing as a webview renderer terminating normally. https://codereview.chromium.org/2630473004/diff/1/android_webview/browser/aw_... File android_webview/browser/aw_render_process_gone_delegate.h (right): https://codereview.chromium.org/2630473004/diff/1/android_webview/browser/aw_... android_webview/browser/aw_render_process_gone_delegate.h:16: // Two callbacks are called when a specific render process was gone. /a specific render process/the renderer process associated with this delegate/ https://codereview.chromium.org/2630473004/diff/1/android_webview/browser/aw_... android_webview/browser/aw_render_process_gone_delegate.h:17: // OnRenderProcessGone is called as soon as render process's termiation is termination https://codereview.chromium.org/2630473004/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2630473004/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContents.java:364: private boolean mIsNoOperation; Given that this is only set to true, and not reset back to false, it seems like it could be called mRendererIsGone, unless there's a different use for it. I'm also not sure what the point of the getter is, given that both the boolean and the getter are private (maybe the comment just needs to be expanded with your reasoning). https://codereview.chromium.org/2630473004/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContents.java:1211: mIsNoOperation = true; No need to set this to true here (especially if it's renamed to mRendererIsGone, then it's actually not the case that the renderer has gone). https://codereview.chromium.org/2630473004/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContents.java:2697: if (isNoOperation()) return; I think this should not silently not add the callback, because this violates the expectation that the visual state callback is always called once inserted. Either it should call the callback immediately, or it should also throw the IllegalStateException.
https://codereview.chromium.org/2630473004/diff/1/android_webview/browser/aw_... File android_webview/browser/aw_browser_terminator.cc (right): https://codereview.chromium.org/2630473004/diff/1/android_webview/browser/aw_... android_webview/browser/aw_browser_terminator.cc:72: // For some reason, FATAL message didn't show up in adb logcat. This normally works, I think; we should probably look into why, rather than just doing this? https://codereview.chromium.org/2630473004/diff/1/android_webview/browser/aw_... android_webview/browser/aw_browser_terminator.cc:111: << "). Terminating browser."; We're not terminating the browser necessarily any more, can we update this log message? https://codereview.chromium.org/2630473004/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2630473004/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContents.java:1187: private boolean isDestroyedOrNoOperation(int warnIfDestroyed) { It looks like you've changed every place that called isDestroyed to this, except for the one in destroy() and the one in onRenderProcessGoneWithDetail. It seems like we could just put this logic into isDestroyed to make this patch much smaller.
PTAL https://codereview.chromium.org/2630473004/diff/1/android_webview/browser/aw_... File android_webview/browser/aw_browser_terminator.cc (right): https://codereview.chromium.org/2630473004/diff/1/android_webview/browser/aw_... android_webview/browser/aw_browser_terminator.cc:55: void OnRenderProcessGone(int child_process_id) { On 2017/01/13 10:48:38, Tobias Sargeant wrote: > I think it would be good to name this and the below in a less similar manner to > make it clear that this is providing a notification that the delegates will soon > receive the termination reason. Otherwise (to me, at least) it looks like these > are either/or methods. > Changed to OnRenderProcessGoneDetail(), and rewrote the comments. > Also, what guarantee do we have that the list of delegates that we get at this > point is identical to the list below? What if an AwContents is destroyed in the > meantime (or, more confusingly, created. will the new AwContents cause the > creation of a new renderer)? No, there is no guarantee, change the description of Delegate class, basically, AwContents is destroyed, then delegate is gone, there is no meaning to notify it with detail information, new AwContents has new renderer, it seems there is no confusing here. https://codereview.chromium.org/2630473004/diff/1/android_webview/browser/aw_... android_webview/browser/aw_browser_terminator.cc:72: // For some reason, FATAL message didn't show up in adb logcat. On 2017/01/13 13:24:08, Torne wrote: > This normally works, I think; we should probably look into why, rather than just > doing this? We shouldn't try to fix LOG issue in this patch, I filed a bug to track it. https://codereview.chromium.org/2630473004/diff/1/android_webview/browser/aw_... android_webview/browser/aw_browser_terminator.cc:111: << "). Terminating browser."; On 2017/01/13 13:24:08, Torne wrote: > We're not terminating the browser necessarily any more, can we update this log > message? Done. https://codereview.chromium.org/2630473004/diff/1/android_webview/browser/aw_... android_webview/browser/aw_browser_terminator.cc:126: OnRenderProcessGone(child_process_id); On 2017/01/13 10:48:38, Tobias Sargeant wrote: > Don't do this here; OnChildExit can be called more than once per child > (unavoidable because of the way the chrome child process notification system > works) so you need to move it to after the early-out when we don't find a pipe. Thanks, it was added for experiment, I didn't think through it afterwards. https://codereview.chromium.org/2630473004/diff/1/android_webview/browser/aw_... android_webview/browser/aw_browser_terminator.cc:141: return; On 2017/01/13 10:48:38, Tobias Sargeant wrote: > You may need to organise for the *WithDetail delegate method to be called in > this case, although I think there's actually no such thing as a webview renderer > terminating normally. Here, we don't know whether render process crashed or was killed. https://codereview.chromium.org/2630473004/diff/1/android_webview/browser/aw_... File android_webview/browser/aw_render_process_gone_delegate.h (right): https://codereview.chromium.org/2630473004/diff/1/android_webview/browser/aw_... android_webview/browser/aw_render_process_gone_delegate.h:16: // Two callbacks are called when a specific render process was gone. On 2017/01/13 10:48:38, Tobias Sargeant wrote: > /a specific render process/the renderer process associated with this delegate/ Removed this https://codereview.chromium.org/2630473004/diff/1/android_webview/browser/aw_... android_webview/browser/aw_render_process_gone_delegate.h:17: // OnRenderProcessGone is called as soon as render process's termiation is On 2017/01/13 10:48:38, Tobias Sargeant wrote: > termination Done. https://codereview.chromium.org/2630473004/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2630473004/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContents.java:364: private boolean mIsNoOperation; On 2017/01/13 10:48:38, Tobias Sargeant wrote: > Given that this is only set to true, and not reset back to false, it seems like > it could be called mRendererIsGone, unless there's a different use for it. > The name indicates what we need to do if it is true. I actually don't have prefer between two names, given that torne want to keep using isDestroyed() everywhere, mIsNoOperation probably is good name here. > I'm also not sure what the point of the getter is, given that both the boolean > and the getter are private (maybe the comment just needs to be expanded with > your reasoning). This is copied from mIsDestroyed. https://codereview.chromium.org/2630473004/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContents.java:1187: private boolean isDestroyedOrNoOperation(int warnIfDestroyed) { On 2017/01/13 13:24:08, Torne wrote: > It looks like you've changed every place that called isDestroyed to this, except > for the one in destroy() and the one in onRenderProcessGoneWithDetail. It seems > like we could just put this logic into isDestroyed to make this patch much > smaller. Do you mean we still use isDestroyed() everywhere, but check mIsNoOperation in isDestroyed()? Is it good to do this? WebView hasn't actually been destroyed in this state. https://codereview.chromium.org/2630473004/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContents.java:1211: mIsNoOperation = true; On 2017/01/13 10:48:38, Tobias Sargeant wrote: > No need to set this to true here (especially if it's renamed to mRendererIsGone, > then it's actually not the case that the renderer has gone). Let's see if we will change the name https://codereview.chromium.org/2630473004/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContents.java:2697: if (isNoOperation()) return; On 2017/01/13 10:48:38, Tobias Sargeant wrote: > I think this should not silently not add the callback, because this violates the > expectation that the visual state callback is always called once inserted. > Either it should call the callback immediately, or it should also throw the > IllegalStateException. I don't know whether there is side-effect to insert callback or throw exception, since WebView should be destroyed soon, we are making it on operation here. it is probably OK to do nothing here? It seemed bo touch this code, add it for his comment +boliu@chromium.org
https://codereview.chromium.org/2630473004/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2630473004/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContents.java:2697: if (isNoOperation()) return; On 2017/01/13 21:01:38, michaelbai wrote: > On 2017/01/13 10:48:38, Tobias Sargeant wrote: > > I think this should not silently not add the callback, because this violates > the > > expectation that the visual state callback is always called once inserted. > > Either it should call the callback immediately, or it should also throw the > > IllegalStateException. > > I don't know whether there is side-effect to insert callback or throw exception, > since WebView should be destroyed soon, we are making it on operation here. it > is probably OK to do nothing here? It seemed bo touch this code, add it for his > comment mailto:+boliu@chromium.org throwing is bound to cause crashes I'd go with posting the callback immediately (to avoid re-entering into app code) instead
https://codereview.chromium.org/2630473004/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2630473004/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContents.java:2697: if (isNoOperation()) return; On 2017/01/13 21:06:12, boliu wrote: > On 2017/01/13 21:01:38, michaelbai wrote: > > On 2017/01/13 10:48:38, Tobias Sargeant wrote: > > > I think this should not silently not add the callback, because this violates > > the > > > expectation that the visual state callback is always called once inserted. > > > Either it should call the callback immediately, or it should also throw the > > > IllegalStateException. > > > > I don't know whether there is side-effect to insert callback or throw > exception, > > since WebView should be destroyed soon, we are making it on operation here. it > > is probably OK to do nothing here? It seemed bo touch this code, add it for > his > > comment mailto:+boliu@chromium.org > > throwing is bound to cause crashes > > I'd go with posting the callback immediately (to avoid re-entering into app > code) instead Done.
https://codereview.chromium.org/2630473004/diff/1/android_webview/browser/aw_... File android_webview/browser/aw_browser_terminator.cc (right): https://codereview.chromium.org/2630473004/diff/1/android_webview/browser/aw_... android_webview/browser/aw_browser_terminator.cc:72: // For some reason, FATAL message didn't show up in adb logcat. On 2017/01/13 21:01:38, michaelbai wrote: > On 2017/01/13 13:24:08, Torne wrote: > > This normally works, I think; we should probably look into why, rather than > just > > doing this? > > We shouldn't try to fix LOG issue in this patch, I filed a bug to track it. We use LOG(FATAL) in other places and I'm pretty sure it works in general; I think you should investigate this specific case and work out what's up rather than just assuming this is broken. https://codereview.chromium.org/2630473004/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2630473004/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContents.java:1187: private boolean isDestroyedOrNoOperation(int warnIfDestroyed) { On 2017/01/13 21:01:38, michaelbai wrote: > On 2017/01/13 13:24:08, Torne wrote: > > It looks like you've changed every place that called isDestroyed to this, > except > > for the one in destroy() and the one in onRenderProcessGoneWithDetail. It > seems > > like we could just put this logic into isDestroyed to make this patch much > > smaller. > > Do you mean we still use isDestroyed() everywhere, but check mIsNoOperation in > isDestroyed()? Is it good to do this? WebView hasn't actually been destroyed in > this state. If none of the callers actually care about the difference between these states (which appears to be true in all but the two cases I identified, and it's not clear that those even need to be different) then it seems like a big pain and a waste of effort to rename this all over the place. Is there any useful distinction?
https://codereview.chromium.org/2630473004/diff/1/android_webview/browser/aw_... File android_webview/browser/aw_browser_terminator.cc (right): https://codereview.chromium.org/2630473004/diff/1/android_webview/browser/aw_... android_webview/browser/aw_browser_terminator.cc:72: // For some reason, FATAL message didn't show up in adb logcat. On 2017/01/16 12:20:32, Torne wrote: > On 2017/01/13 21:01:38, michaelbai wrote: > > On 2017/01/13 13:24:08, Torne wrote: > > > This normally works, I think; we should probably look into why, rather than > > just > > > doing this? > > > > We shouldn't try to fix LOG issue in this patch, I filed a bug to track it. > > We use LOG(FATAL) in other places and I'm pretty sure it works in general; I > think you should investigate this specific case and work out what's up rather > than just assuming this is broken. Would you like point me where it still work? it will help me to figure out how it is broken for this specific case. Thanks! https://codereview.chromium.org/2630473004/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2630473004/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContents.java:1187: private boolean isDestroyedOrNoOperation(int warnIfDestroyed) { On 2017/01/16 12:20:32, Torne wrote: > On 2017/01/13 21:01:38, michaelbai wrote: > > On 2017/01/13 13:24:08, Torne wrote: > > > It looks like you've changed every place that called isDestroyed to this, > > except > > > for the one in destroy() and the one in onRenderProcessGoneWithDetail. It > > seems > > > like we could just put this logic into isDestroyed to make this patch much > > > smaller. > > > > Do you mean we still use isDestroyed() everywhere, but check mIsNoOperation in > > isDestroyed()? Is it good to do this? WebView hasn't actually been destroyed > in > > this state. > > If none of the callers actually care about the difference between these states > (which appears to be true in all but the two cases I identified, and it's not > clear that those even need to be different) then it seems like a big pain and a > waste of effort to rename this all over the place. > > Is there any useful distinction? I don't they are useful or not, there are two place now, once is in insertVisualStateCallback(), another is destroy() method.
> I don't they are useful or not, there are two place now, once is in > insertVisualStateCallback(), another is destroy() method. Isn't insertVisualStateCallback much more regular now?
On 2017/01/17 17:15:06, Tobias Sargeant wrote: > > I don't they are useful or not, there are two place now, once is in > > insertVisualStateCallback(), another is destroy() method. > > Isn't insertVisualStateCallback much more regular now? Correct! but torne's point is merging two state into isDestroyed().
On 2017/01/17 17:30:27, michaelbai wrote: > On 2017/01/17 17:15:06, Tobias Sargeant wrote: > > > I don't they are useful or not, there are two place now, once is in > > > insertVisualStateCallback(), another is destroy() method. > > > > Isn't insertVisualStateCallback much more regular now? > > Correct! but torne's point is merging two state into isDestroyed(). Yes, but I think that the only place you can't do that now is destroy()?
On 2017/01/17 17:09:42, michaelbai wrote: > https://codereview.chromium.org/2630473004/diff/1/android_webview/browser/aw_... > File android_webview/browser/aw_browser_terminator.cc (right): > > https://codereview.chromium.org/2630473004/diff/1/android_webview/browser/aw_... > android_webview/browser/aw_browser_terminator.cc:72: // For some reason, FATAL > message didn't show up in adb logcat. > On 2017/01/16 12:20:32, Torne wrote: > > On 2017/01/13 21:01:38, michaelbai wrote: > > > On 2017/01/13 13:24:08, Torne wrote: > > > > This normally works, I think; we should probably look into why, rather > than > > > just > > > > doing this? > > > > > > We shouldn't try to fix LOG issue in this patch, I filed a bug to track it. > > > > We use LOG(FATAL) in other places and I'm pretty sure it works in general; I > > think you should investigate this specific case and work out what's up rather > > than just assuming this is broken. > > Would you like point me where it still work? it will help me to figure out how > it is broken for this specific case. Thanks! > > https://codereview.chromium.org/2630473004/diff/1/android_webview/java/src/or... > File android_webview/java/src/org/chromium/android_webview/AwContents.java > (right): > > https://codereview.chromium.org/2630473004/diff/1/android_webview/java/src/or... > android_webview/java/src/org/chromium/android_webview/AwContents.java:1187: > private boolean isDestroyedOrNoOperation(int warnIfDestroyed) { > On 2017/01/16 12:20:32, Torne wrote: > > On 2017/01/13 21:01:38, michaelbai wrote: > > > On 2017/01/13 13:24:08, Torne wrote: > > > > It looks like you've changed every place that called isDestroyed to this, > > > except > > > > for the one in destroy() and the one in onRenderProcessGoneWithDetail. It > > > seems > > > > like we could just put this logic into isDestroyed to make this patch much > > > > smaller. > > > > > > Do you mean we still use isDestroyed() everywhere, but check mIsNoOperation > in > > > isDestroyed()? Is it good to do this? WebView hasn't actually been destroyed > > in > > > this state. > > > > If none of the callers actually care about the difference between these states > > (which appears to be true in all but the two cases I identified, and it's not > > clear that those even need to be different) then it seems like a big pain and > a > > waste of effort to rename this all over the place. > > > > Is there any useful distinction? > > I don't they are useful or not, there are two place now, once is in > insertVisualStateCallback(), another is destroy() method. Hmm, insertVisualStateCallback (or anything else that has a callback) is kinda useful. We promise to never fire callbacks after destroy, so we just drop the callback. But if renderer crashed, in theory we should fire the callback, to avoid app doesn't get into a weird state. fwiw, we discussed this before in the internal thread "Continue discussing crash API"
On 2017/01/17 17:37:55, Tobias Sargeant wrote: > On 2017/01/17 17:30:27, michaelbai wrote: > > On 2017/01/17 17:15:06, Tobias Sargeant wrote: > > > > I don't they are useful or not, there are two place now, once is in > > > > insertVisualStateCallback(), another is destroy() method. > > > > > > Isn't insertVisualStateCallback much more regular now? > > > > Correct! but torne's point is merging two state into isDestroyed(). > > Yes, but I think that the only place you can't do that now is destroy()? Once they are merged, isDestroyed() equals iDestroyedOrNoOperation(), but we can't throw exception if WebView is in NoOperation state, right? Do I miss something?
On 2017/01/17 17:42:48, michaelbai wrote: > Once they are merged, isDestroyed() equals iDestroyedOrNoOperation(), but we > can't throw exception if WebView is in NoOperation state, right? Do I miss > something? No, you're right, and I shouldn't respond to CLs during meetings. Sorry.
https://codereview.chromium.org/2630473004/diff/1/android_webview/browser/aw_... File android_webview/browser/aw_browser_terminator.cc (right): https://codereview.chromium.org/2630473004/diff/1/android_webview/browser/aw_... android_webview/browser/aw_browser_terminator.cc:72: // For some reason, FATAL message didn't show up in adb logcat. On 2017/01/17 17:09:42, michaelbai wrote: > On 2017/01/16 12:20:32, Torne wrote: > > On 2017/01/13 21:01:38, michaelbai wrote: > > > On 2017/01/13 13:24:08, Torne wrote: > > > > This normally works, I think; we should probably look into why, rather > than > > > just > > > > doing this? > > > > > > We shouldn't try to fix LOG issue in this patch, I filed a bug to track it. > > > > We use LOG(FATAL) in other places and I'm pretty sure it works in general; I > > think you should investigate this specific case and work out what's up rather > > than just assuming this is broken. > > Would you like point me where it still work? it will help me to figure out how > it is broken for this specific case. Thanks! It seems Android O issue, the message showed up now.
If there's actually cases where we really need to do something different that's visible to the developer (like the visual state callback; will defer to bo/toby there) then renaming it is fine. I was only objecting to renaming it when the only cases that handled it differently seemed unimportant/trivial to deal with.
I think I replied all comments, PTAL
https://codereview.chromium.org/2630473004/diff/60001/android_webview/browser... File android_webview/browser/aw_browser_terminator.cc (right): https://codereview.chromium.org/2630473004/diff/60001/android_webview/browser... android_webview/browser/aw_browser_terminator.cc:70: LOG(FATAL) << "Render process's abnormal termination wasn't handled by" When you rebase, you'll need crash_reporter::SuppressDumpGeneration(); immediately before this. https://codereview.chromium.org/2630473004/diff/60001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2630473004/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2695: nativeInsertVisualStateCallback(mNativeAwContents, requestId, callback); You need to change this to fire the callback immediately if mIsNoOperation.
https://codereview.chromium.org/2630473004/diff/60001/android_webview/browser... File android_webview/browser/aw_browser_terminator.cc (right): https://codereview.chromium.org/2630473004/diff/60001/android_webview/browser... android_webview/browser/aw_browser_terminator.cc:70: LOG(FATAL) << "Render process's abnormal termination wasn't handled by" On 2017/01/19 17:28:03, Tobias Sargeant wrote: > When you rebase, you'll need > > crash_reporter::SuppressDumpGeneration(); > > immediately before this. Thanks https://codereview.chromium.org/2630473004/diff/60001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2630473004/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2695: nativeInsertVisualStateCallback(mNativeAwContents, requestId, callback); On 2017/01/19 17:28:03, Tobias Sargeant wrote: > You need to change this to fire the callback immediately if mIsNoOperation. So, nativeInsertVisualStateCallback doesn't fire callback immediately? The method's behavior does't change no matter WebView is no operation or not, this is what we expected, isn't it?
On 2017/01/19 17:33:31, michaelbai wrote: > https://codereview.chromium.org/2630473004/diff/60001/android_webview/browser... > File android_webview/browser/aw_browser_terminator.cc (right): > > https://codereview.chromium.org/2630473004/diff/60001/android_webview/browser... > android_webview/browser/aw_browser_terminator.cc:70: LOG(FATAL) << "Render > process's abnormal termination wasn't handled by" > On 2017/01/19 17:28:03, Tobias Sargeant wrote: > > When you rebase, you'll need > > > > crash_reporter::SuppressDumpGeneration(); > > > > immediately before this. > > Thanks > > https://codereview.chromium.org/2630473004/diff/60001/android_webview/java/sr... > File android_webview/java/src/org/chromium/android_webview/AwContents.java > (right): > > https://codereview.chromium.org/2630473004/diff/60001/android_webview/java/sr... > android_webview/java/src/org/chromium/android_webview/AwContents.java:2695: > nativeInsertVisualStateCallback(mNativeAwContents, requestId, callback); > On 2017/01/19 17:28:03, Tobias Sargeant wrote: > > You need to change this to fire the callback immediately if mIsNoOperation. > > So, nativeInsertVisualStateCallback doesn't fire callback immediately? The > method's behavior does't change no matter WebView is no operation or not, this > is what we expected, isn't it? It inserts the callback into a list of callbacks to call when the visual state next updates. If the renderer has crashed, the callback will never fire since the state will never update again. Toby's suggestion is that the callback should skip the normal processing and instead fire immediately so that the app can't be stuck waiting forever for it. i.e. he's expecting you to have the behaviour change in the case where mIsNoOperation=true :)
On 2017/01/20 12:08:25, Torne wrote: > On 2017/01/19 17:33:31, michaelbai wrote: > > > https://codereview.chromium.org/2630473004/diff/60001/android_webview/browser... > > File android_webview/browser/aw_browser_terminator.cc (right): > > > > > https://codereview.chromium.org/2630473004/diff/60001/android_webview/browser... > > android_webview/browser/aw_browser_terminator.cc:70: LOG(FATAL) << "Render > > process's abnormal termination wasn't handled by" > > On 2017/01/19 17:28:03, Tobias Sargeant wrote: > > > When you rebase, you'll need > > > > > > crash_reporter::SuppressDumpGeneration(); > > > > > > immediately before this. > > > > Thanks > > > > > https://codereview.chromium.org/2630473004/diff/60001/android_webview/java/sr... > > File android_webview/java/src/org/chromium/android_webview/AwContents.java > > (right): > > > > > https://codereview.chromium.org/2630473004/diff/60001/android_webview/java/sr... > > android_webview/java/src/org/chromium/android_webview/AwContents.java:2695: > > nativeInsertVisualStateCallback(mNativeAwContents, requestId, callback); > > On 2017/01/19 17:28:03, Tobias Sargeant wrote: > > > You need to change this to fire the callback immediately if mIsNoOperation. > > > > So, nativeInsertVisualStateCallback doesn't fire callback immediately? The > > method's behavior does't change no matter WebView is no operation or not, this > > is what we expected, isn't it? > > It inserts the callback into a list of callbacks to call when the visual state > next updates. If the renderer has crashed, the callback will never fire since > the state will never update again. > > Toby's suggestion is that the callback should skip the normal processing and > instead fire immediately so that the app can't be stuck waiting forever for it. > i.e. he's expecting you to have the behaviour change in the case where > mIsNoOperation=true :) lgtm on behalf of Toby
As discussed, moved WebView no operation change to another patch.
The CQ bit was checked by michaelbai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org Link to the patchset: https://codereview.chromium.org/2630473004/#ps80001 (title: "Move WebView no operation change to another patch")
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": 80001, "attempt_start_ts": 1484962884056650,
"parent_rev": "a7c18cdafd7db2229539a5e0b5b19c7cb8c6d862", "commit_rev":
"6e10ee5b759fbe79d5e24096a337e9534012bbc8"}
Message was sent while issue was closed.
Description was changed from ========== Implement Crash Handle API This patch implements basical feature for crash handle API. The feature like 'render processs keep crashing after crash is handled' hasn't implemented. BUG=666106 ========== to ========== Implement Crash Handle API This patch implements basical feature for crash handle API. The feature like 'render processs keep crashing after crash is handled' hasn't implemented. BUG=666106 Review-Url: https://codereview.chromium.org/2630473004 Cr-Commit-Position: refs/heads/master@{#445256} Committed: https://chromium.googlesource.com/chromium/src/+/6e10ee5b759fbe79d5e24096a337... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/6e10ee5b759fbe79d5e24096a337... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
