Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
146693011:
Terminate process in onDestroy. (Closed)
|
Created:
6 years, 10 months ago by Fredrik Öhrn Modified:
5 years, 4 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, ppi Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionTerminate process in onDestroy.
Normally this code is never reached, the child process terminates when the
IPC channel is closed. If onDestroy is called something abnormal happened
and there is not much point in doing a graceful shutdown.
BUG=338709
Patch Set 1 #Patch Set 2 : retry upload #
Total comments: 2
Patch Set 3 : Use System.exit #
Total comments: 5
Messages
Total messages: 36 (3 generated)
The current onDestroy code does a lot of work to initiate a graceful shutdown. I assume there is a good reason that I don't know about, so this patch is more about starting a discussion. As described in the bug, if something goes wrong we'll end up in onDestroy and it really needs a bigger hammer. The question is: How big?
When you have multiple reviewers, please explicitly specify who should be reviewing what files. Otherwise, it's easy to have everyone be an onlooker. FYI, "bulach_ooo_until_feb_2". I hope you're not expecting an answer soon. And this is a bit out of my expertise; backing out.
On 2014/01/28 17:04:54, Avi wrote: > When you have multiple reviewers, please explicitly specify who should be > reviewing what files. Otherwise, it's easy to have everyone be an onlooker. > > FYI, "bulach_ooo_until_feb_2". I hope you're not expecting an answer soon. > > And this is a bit out of my expertise; backing out. Sorry about that, the git cl upload hook throws a lot of people at you when submitting a patch and being an outsider it's hard to know who to involve. Adding michaelbai instead since it looks like you wrote most of the onDestroy function.
On 2014/01/28 17:16:03, Fredrik Öhrn wrote: > On 2014/01/28 17:04:54, Avi wrote: > > When you have multiple reviewers, please explicitly specify who should be > > reviewing what files. Otherwise, it's easy to have everyone be an onlooker. > > > > FYI, "bulach_ooo_until_feb_2". I hope you're not expecting an answer soon. > > > > And this is a bit out of my expertise; backing out. > > Sorry about that, the git cl upload hook throws a lot of people at you when > submitting a patch and being an outsider it's hard to know who to involve. > > Adding michaelbai instead since it looks like you wrote most of the onDestroy > function. Can you re-upload your patch. I'm getting errors trying to view the diff
On 2014/01/28 22:00:37, Yaron wrote: > > Can you re-upload your patch. I'm getting errors trying to view the diff Uploaded again, now the side-by-side view works.
+klobag who should also look https://codereview.chromium.org/146693011/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/app/ChildProcessService.java (left): https://codereview.chromium.org/146693011/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:194: // Avoid a potential race in calling through to native code before the library See this comment. Maybe rather than passing down to native, we can do this purely in java with a System.exit() ? It would mean we don't run the hooks registered on the AtExitManager but I can't see where we do that for child processes.
https://codereview.chromium.org/146693011/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/app/ChildProcessService.java (right): https://codereview.chromium.org/146693011/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:193: nativeExitChildProcess(); Besides Yaron's comment, this essentially reverted https://chromiumcodereview.appspot.com/11428056/. So we will hit the original crash, right?
On 2014/01/29 23:50:03, klobag.chromium wrote: > https://codereview.chromium.org/146693011/diff/20001/content/public/android/j... > File > content/public/android/java/src/org/chromium/content/app/ChildProcessService.java > (right): > > https://codereview.chromium.org/146693011/diff/20001/content/public/android/j... > content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:193: > nativeExitChildProcess(); > Besides Yaron's comment, this essentially reverted > https://chromiumcodereview.appspot.com/11428056/. So we will hit the original > crash, right? Ya, seems like it.. but doing it in Java would avoid the crash there
On 2014/01/30 00:12:50, Yaron wrote: > On 2014/01/29 23:50:03, klobag.chromium wrote: > > > https://codereview.chromium.org/146693011/diff/20001/content/public/android/j... > > File > > > content/public/android/java/src/org/chromium/content/app/ChildProcessService.java > > (right): > > > > > https://codereview.chromium.org/146693011/diff/20001/content/public/android/j... > > > content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:193: > > nativeExitChildProcess(); > > Besides Yaron's comment, this essentially reverted > > https://chromiumcodereview.appspot.com/11428056/. So we will hit the original > > crash, right? > > Ya, seems like it.. but doing it in Java would avoid the crash there Updated the patch to use System.exit. I actually wobbled a between System.exit and nativeExitChildProcess when writing the patch, I should have read the code comments more carefully.
I remembered we discussed do we really need to shutdown gracefully, if we decide we don't have to, it will make thing simple. BTW, if the IPC issue fixed, Is this patch still needed?
On 2014/01/30 21:26:50, michaelbai wrote: > I remembered we discussed do we really need to shutdown gracefully, if we decide > we don't have to, it will make thing simple. > If I understand the code correctly other platforms do not bother with a graceful shutdown? E.g. on plain Linux SIGTERM is sent to the render process. > BTW, if the IPC issue fixed, Is this patch still needed? Fixing the IPC issue alone will resolve the specific 338709 bug. Though I was under the impression that forcibly terminating the render process is an extra safeguard in case it is stuck in a hung/deadlocked/runaway state where closing the IPC channel is not enough. Just because one IPC bug is fixed doesn't mean there are no other bugs that may cause the renderer to hang around past it's expiry date... I may be totally wrong about this of course.
Unless we decide that Android chrome does NOT need to shutdown gracefully, I think fixing IPC is right fix for this issue.
https://codereview.chromium.org/146693011/diff/40001/content/public/android/j... File content/public/android/java/src/org/chromium/content/app/ChildProcessService.java (right): https://codereview.chromium.org/146693011/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:193: System.exit(0); I can try to dig further from the android side, but System.exit is extremely frowned upon... it messes up with the activity life cycle, and in some cases it may trigger a process restart.. don't quite know all the details, just saw some threads floating around. I can dig it up if you want?
On 2014/02/03 21:57:36, bulach wrote: > https://codereview.chromium.org/146693011/diff/40001/content/public/android/j... > File > content/public/android/java/src/org/chromium/content/app/ChildProcessService.java > (right): > > https://codereview.chromium.org/146693011/diff/40001/content/public/android/j... > content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:193: > System.exit(0); > I can try to dig further from the android side, but System.exit is extremely > frowned upon... > it messes up with the activity life cycle, and in some cases it may trigger a > process restart.. > don't quite know all the details, just saw some threads floating around. > I can dig it up if you want? I'm curious as to why it's important to do a clean shutdown here, while in the normal case SuicideOnChannelErrorFilter kills the process with _exit(0)?
On 2014/02/04 09:14:31, Fredrik Öhrn wrote: > On 2014/02/03 21:57:36, bulach wrote: > > > https://codereview.chromium.org/146693011/diff/40001/content/public/android/j... > > File > > > content/public/android/java/src/org/chromium/content/app/ChildProcessService.java > > (right): > > > > > https://codereview.chromium.org/146693011/diff/40001/content/public/android/j... > > > content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:193: > > System.exit(0); > > I can try to dig further from the android side, but System.exit is extremely > > frowned upon... > > it messes up with the activity life cycle, and in some cases it may trigger a > > process restart.. > > don't quite know all the details, just saw some threads floating around. > > I can dig it up if you want? > > I'm curious as to why it's important to do a clean shutdown here, while in the > normal case SuicideOnChannelErrorFilter kills the process with _exit(0)? To follow up on this, it seems to me a graceful shutdown is such an uncommon event the cleanup code is never tested and is prone to bitrot. I've just posted one example I've identified in crbug.com/342358 where the graceful shutdown ends up crashing. Again if exit(0) is such a bad thing it should be removed from SuicideOnChannelErrorFilter too, so that all the broken cleanup code will be noticed and fixed.
On 2014/02/10 13:38:06, Fredrik Öhrn wrote: > On 2014/02/04 09:14:31, Fredrik Öhrn wrote: > > On 2014/02/03 21:57:36, bulach wrote: > > > > > > https://codereview.chromium.org/146693011/diff/40001/content/public/android/j... > > > File > > > > > > content/public/android/java/src/org/chromium/content/app/ChildProcessService.java > > > (right): > > > > > > > > > https://codereview.chromium.org/146693011/diff/40001/content/public/android/j... > > > > > > content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:193: > > > System.exit(0); > > > I can try to dig further from the android side, but System.exit is extremely > > > frowned upon... > > > it messes up with the activity life cycle, and in some cases it may trigger > a > > > process restart.. > > > don't quite know all the details, just saw some threads floating around. > > > I can dig it up if you want? > > > > I'm curious as to why it's important to do a clean shutdown here, while in the > > normal case SuicideOnChannelErrorFilter kills the process with _exit(0)? > > To follow up on this, it seems to me a graceful shutdown is such an uncommon > event the cleanup code is never tested and is prone to bitrot. > > I've just posted one example I've identified in crbug.com/342358 where the > graceful shutdown ends up crashing. > > Again if exit(0) is such a bad thing it should be removed from > SuicideOnChannelErrorFilter too, so that all the broken cleanup code will be > noticed and fixed. fwiw, I agree but would prefer to hear from bulach if he can dig up further details.
ppi@, I think you dealt with the renderer termination in the past, right? would you mind taking a look at this please?
TL;DR: I like it but I don't feel competent to decide. While I second Fredrik's concerns for the clean shutdown bitrot and matching other platforms, I think we might want to check with the Android framework folks about the issues mentioned by Marcus. Fwiw, http://developer.android.com/reference/android/app/Service.html#onDestroy() states: "The service should clean up any resources it holds (threads, registered receivers, etc) at this point."
Ping?
ppi@chromium.org changed reviewers: - ppi@chromium.org
ppi@chromium.org changed reviewers: + ppi@chromium.org
Let's take this out of the limbo - Fredrik, ptal at a question below. https://codereview.chromium.org/146693011/diff/40001/content/public/android/j... File content/public/android/java/src/org/chromium/content/app/ChildProcessService.java (right): https://codereview.chromium.org/146693011/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:193: System.exit(0); On 2014/02/03 21:57:40, bulach wrote: > I can try to dig further from the android side, but System.exit is extremely > frowned upon... > it messes up with the activity life cycle, and in some cases it may trigger a > process restart.. > don't quite know all the details, just saw some threads floating around. > I can dig it up if you want? If System.exit(0) is frowned up... would this work with Process.killProcess(Process.myPid()); ?
In the context of this being the fix for the immediate process reuse problem, I'd be happy to try this in M41 (ie. soon), if that's fine with everyone.
https://codereview.chromium.org/146693011/diff/40001/content/public/android/j... File content/public/android/java/src/org/chromium/content/app/ChildProcessService.java (right): https://codereview.chromium.org/146693011/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:193: System.exit(0); On 2014/11/07 14:16:35, ppi wrote: > On 2014/02/03 21:57:40, bulach wrote: > > I can try to dig further from the android side, but System.exit is extremely > > frowned upon... > > it messes up with the activity life cycle, and in some cases it may trigger a > > process restart.. > > don't quite know all the details, just saw some threads floating around. > > I can dig it up if you want? > > If System.exit(0) is frowned up... would this work with > Process.killProcess(Process.myPid()); ? I'm not sure exactly what supposedly gets messed up in the activity manager when a service process exits. It does print a complaint in logcat and a notice about restarting the process (the same message as for a regular crash) however I've never seen it actually restart anything, presumably since the browser process will (or have already) drop all bindings. Digging into the Android OS code, System.exit will run exit hooks (both at the Java and C library levels) if there are any registered, while calling killProcess will send a SIGKILL which can't be caught so no hooks or other cleanup will run. I don't know if there are any activity manager related exit hooks, but if there are I would assume it's friendlier to let them run. On the other hand killProcess will be indistinguishable from a crash which the manager obviously must be able to cope with. In short I think in theory System.exit might be better, but in practice it probably doesn't matter, whatever goes "bad" when terminating it probably happens inside the activity manager, not in the service process. It would be great if you could reach out to the right Android OS people and ask them if there really is a problem with terminating the service process and if yes, exactly what that problem is. (It obviously can't be a big problem since we exit all the time from native code when the IPC pipe is closed.)
https://codereview.chromium.org/146693011/diff/40001/content/public/android/j... File content/public/android/java/src/org/chromium/content/app/ChildProcessService.java (right): https://codereview.chromium.org/146693011/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:193: System.exit(0); On 2014/11/07 15:00:43, Fredrik Öhrn wrote: > On 2014/11/07 14:16:35, ppi wrote: > > On 2014/02/03 21:57:40, bulach wrote: > > > I can try to dig further from the android side, but System.exit is extremely > > > frowned upon... > > > it messes up with the activity life cycle, and in some cases it may trigger > a > > > process restart.. > > > don't quite know all the details, just saw some threads floating around. > > > I can dig it up if you want? > > > > If System.exit(0) is frowned up... would this work with > > Process.killProcess(Process.myPid()); ? > > I'm not sure exactly what supposedly gets messed up in the activity manager when > a service process exits. It does print a complaint in logcat and a notice about > restarting the process (the same message as for a regular crash) however I've > never seen it actually restart anything, presumably since the browser process > will (or have already) drop all bindings. > > Digging into the Android OS code, System.exit will run exit hooks (both at the > Java and C library levels) if there are any registered, while calling > killProcess will send a SIGKILL which can't be caught so no hooks or other > cleanup will run. > > I don't know if there are any activity manager related exit hooks, but if there > are I would assume it's friendlier to let them run. On the other hand > killProcess will be indistinguishable from a crash which the manager obviously > must be able to cope with. In short I think in theory System.exit might be > better, but in practice it probably doesn't matter, whatever goes "bad" when > terminating it probably happens inside the activity manager, not in the service > process. > > It would be great if you could reach out to the right Android OS people and ask > them if there really is a problem with terminating the service process and if > yes, exactly what that problem is. > > (It obviously can't be a big problem since we exit all the time from native code > when the IPC pipe is closed.) Thanks for looking into it! Process.killProcess(Process.myPid()); seems a pretty safe bet to me, given its equivalence to a process crash. I relayed the discussion to android framework folks and pointed here, waiting for a reply.
ppi@chromium.org changed reviewers: + mariakhomenko@google.com
https://codereview.chromium.org/146693011/diff/40001/content/public/android/j... File content/public/android/java/src/org/chromium/content/app/ChildProcessService.java (right): https://codereview.chromium.org/146693011/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:193: System.exit(0); On 2014/11/14 10:29:58, ppi wrote: > On 2014/11/07 15:00:43, Fredrik Öhrn wrote: > > On 2014/11/07 14:16:35, ppi wrote: > > > On 2014/02/03 21:57:40, bulach wrote: > > > > I can try to dig further from the android side, but System.exit is > extremely > > > > frowned upon... > > > > it messes up with the activity life cycle, and in some cases it may > trigger > > a > > > > process restart.. > > > > don't quite know all the details, just saw some threads floating around. > > > > I can dig it up if you want? > > > > > > If System.exit(0) is frowned up... would this work with > > > Process.killProcess(Process.myPid()); ? > > > > I'm not sure exactly what supposedly gets messed up in the activity manager > when > > a service process exits. It does print a complaint in logcat and a notice > about > > restarting the process (the same message as for a regular crash) however I've > > never seen it actually restart anything, presumably since the browser process > > will (or have already) drop all bindings. > > > > Digging into the Android OS code, System.exit will run exit hooks (both at the > > Java and C library levels) if there are any registered, while calling > > killProcess will send a SIGKILL which can't be caught so no hooks or other > > cleanup will run. > > > > I don't know if there are any activity manager related exit hooks, but if > there > > are I would assume it's friendlier to let them run. On the other hand > > killProcess will be indistinguishable from a crash which the manager obviously > > must be able to cope with. In short I think in theory System.exit might be > > better, but in practice it probably doesn't matter, whatever goes "bad" when > > terminating it probably happens inside the activity manager, not in the > service > > process. > > > > It would be great if you could reach out to the right Android OS people and > ask > > them if there really is a problem with terminating the service process and if > > yes, exactly what that problem is. > > > > (It obviously can't be a big problem since we exit all the time from native > code > > when the IPC pipe is closed.) > > Thanks for looking into it! Process.killProcess(Process.myPid()); seems a pretty > safe bet to me, given its equivalence to a process crash. > > I relayed the discussion to android framework folks and pointed here, waiting > for a reply. We had a discussion with the framework folks. Please find some notes below: - processes for isolated services (like our renderers) are *not* reused for subsequent services after the service is destroyed - one hypothesis was that the problem we were seeing was caused by calling unbind and then binding the service again in quick succession; as destroying the service is asynchronous - yet it seems that even in this case we shouldn't be able to run into trouble - the activity manager service should always destroy the isolated process in response to the last binding being removed and not reuse it - it's hard to reason about the referenced crash - we were never able to reproduce the crash locally; we only had crash reports that ceased to come after we introduced the LRU ordering of services Finally, the consensus was that given that the system guarantees that processes for isolated services are not reused, killing the process ourselves would be a workaround for an issue that we don't understand; and we probably shouldn't do it. I feel on the fence on this matter - I think I'd err on keeping the status quo side until we hit the actual problem again. Wdyt? Also adding Maria, who is taking over process management on our side.
On 2014/12/12 19:23:06, ppi wrote: > > We had a discussion with the framework folks. Please find some notes below: > > - processes for isolated services (like our renderers) are *not* reused for > subsequent services after the service is destroyed > - one hypothesis was that the problem we were seeing was caused by calling > unbind and then binding the service again in quick succession; as destroying the > service is asynchronous > - yet it seems that even in this case we shouldn't be able to run into trouble - > the activity manager service should always destroy the isolated process in > response to the last binding being removed and not reuse it > - it's hard to reason about the referenced crash - we were never able to > reproduce the crash locally; we only had crash reports that ceased to come after > we introduced the LRU ordering of services > > Finally, the consensus was that given that the system guarantees that processes > for isolated services are not reused, killing the process ourselves would be a > workaround for an issue that we don't understand; and we probably shouldn't do > it. > Well, even if the process reuse issue is gone, there are still other edge cases where the attempt at doing a graceful shutdown will just crash. One example I identified is this bug: http://code.google.com/p/chromium/issues/detail?id=342358 Trying to fix it I concluded the shutdown code was beyond broken because it is never used, as all other platforms just take the exit(0) route. This was quite some time ago so that particular bug might be gone due to code churn by now, but as long as no-one tests graceful shutdown new crashes will certainly pop up here and there. > I feel on the fence on this matter - I think I'd err on keeping the status quo > side until we hit the actual problem again. Wdyt? > How about a compromise, no forced exit, but let's remove nativeShutdownMainThread()? > Also adding Maria, who is taking over process management on our side. Hi, and welcome to the jungle!
On 2014/12/15 16:55:58, Fredrik Öhrn wrote: > On 2014/12/12 19:23:06, ppi wrote: > > > > We had a discussion with the framework folks. Please find some notes below: > > > > - processes for isolated services (like our renderers) are *not* reused for > > subsequent services after the service is destroyed > > - one hypothesis was that the problem we were seeing was caused by calling > > unbind and then binding the service again in quick succession; as destroying > the > > service is asynchronous > > - yet it seems that even in this case we shouldn't be able to run into trouble > - > > the activity manager service should always destroy the isolated process in > > response to the last binding being removed and not reuse it > > - it's hard to reason about the referenced crash - we were never able to > > reproduce the crash locally; we only had crash reports that ceased to come > after > > we introduced the LRU ordering of services > > > > Finally, the consensus was that given that the system guarantees that > processes > > for isolated services are not reused, killing the process ourselves would be a > > workaround for an issue that we don't understand; and we probably shouldn't do > > it. > > > > Well, even if the process reuse issue is gone, there are still other edge cases > where the attempt at doing a graceful shutdown will just crash. > > One example I identified is this bug: > http://code.google.com/p/chromium/issues/detail?id=342358 > > Trying to fix it I concluded the shutdown code was beyond broken because it is > never used, as all other platforms just take the exit(0) route. This was quite > some time ago so that particular bug might be gone due to code churn by now, but > as long as no-one tests graceful shutdown new crashes will certainly pop up here > and there. > The point is after onDestroy is returned, framework will kill the process. There is no need to call exit(0) inside onDestroy, which may confuse framework. > > I feel on the fence on this matter - I think I'd err on keeping the status quo > > side until we hit the actual problem again. Wdyt? > > > > How about a compromise, no forced exit, but let's remove > nativeShutdownMainThread()? Then you will hit another crash, https://code.google.com/p/chromium/issues/detail?id=163017 > > > Also adding Maria, who is taking over process management on our side. > > Hi, and welcome to the jungle!
On 2014/12/16 00:31:55, klobag.chromium wrote: > On 2014/12/15 16:55:58, Fredrik Öhrn wrote: > > > > How about a compromise, no forced exit, but let's remove > > nativeShutdownMainThread()? > > Then you will hit another crash, > https://code.google.com/p/chromium/issues/detail?id=163017 > Wow, I'm surprised that there is so much exit/shutdown handling code all over the place when 99% of the time the plug is pulled in SuicideOnChannelErrorFilter with a rather abrupt _exit(0). I'm not going to press the issue, I think we'll just keep this patch in our own tree, it eliminates some crash reporting noise and has not had any ill effects so far.
Message was sent while issue was closed.
On 2014/12/16 10:02:05, Fredrik Öhrn wrote: > On 2014/12/16 00:31:55, klobag.chromium wrote: > > On 2014/12/15 16:55:58, Fredrik Öhrn wrote: > > > > > > How about a compromise, no forced exit, but let's remove > > > nativeShutdownMainThread()? > > > > Then you will hit another crash, > > https://code.google.com/p/chromium/issues/detail?id=163017 > > > > Wow, I'm surprised that there is so much exit/shutdown handling code all over > the place when 99% of the time the plug is pulled in SuicideOnChannelErrorFilter > with a rather abrupt _exit(0). > > I'm not going to press the issue, I think we'll just keep this patch in our own > tree, it eliminates some crash reporting noise and has not had any ill effects > so far. I found something that might be related and I'm wondering if it explains a difference between Chrome and Opera and why you may be hitting onDestroy more than Chrome. In the internal repo we have a ChromeTab which subclasses TabAndroid. I'm trying to upstream/remove it. In it, we have this tidbit: void ChromeTab::DestroyWebContents(JNIEnv* env, jobject obj, jboolean delete_native) { // Terminate the renderer process if this is the last tab. // If there's no unload listener, FastShutdownForPageCount kills the // renderer process. Otherwise, we go with the slow path where renderer // process shuts down itself when ref count becomes 0. // TODO(tedchoc): Clean up these deletion ordering dependencies and actually // fix the ownership issue. if (delete_native) { content::RenderProcessHost* process = web_contents()->GetRenderProcessHost(); if (process) process->FastShutdownForPageCount(1); } TabAndroid::DestroyWebContents(env, obj, delete_native); } It's not as aggressive as your patch, but it might explain why Chrome doesn't get to onDestroy as often. It seems like this would cause Chrome to reliably reach SuicideOnChannelErrorFilter and _exit(0). In the event of a beforeUnloadHandler, we could hit ChildProcessService.onDestroy but I assume that's fairly uncommon.
Message was sent while issue was closed.
On 2015/02/20 17:09:44, Yaron wrote:
>
> I found something that might be related and I'm wondering if it explains a
> difference between Chrome and Opera and why you may be hitting onDestroy more
> than Chrome.
>
> In the internal repo we have a ChromeTab which subclasses TabAndroid. I'm
trying
> to upstream/remove it. In it, we have this tidbit:
>
> void ChromeTab::DestroyWebContents(JNIEnv* env,
> jobject obj,
> jboolean delete_native) {
> // Terminate the renderer process if this is the last tab.
> // If there's no unload listener, FastShutdownForPageCount kills the
> // renderer process. Otherwise, we go with the slow path where renderer
> // process shuts down itself when ref count becomes 0.
>
> // TODO(tedchoc): Clean up these deletion ordering dependencies and actually
> // fix the ownership issue.
> if (delete_native) {
> content::RenderProcessHost* process =
> web_contents()->GetRenderProcessHost();
> if (process)
> process->FastShutdownForPageCount(1);
> }
>
> TabAndroid::DestroyWebContents(env,
> obj,
> delete_native);
> }
>
> It's not as aggressive as your patch, but it might explain why Chrome doesn't
> get to onDestroy as often. It seems like this would cause Chrome to reliably
> reach SuicideOnChannelErrorFilter and _exit(0). In the event of a
> beforeUnloadHandler, we could hit ChildProcessService.onDestroy but I assume
> that's fairly uncommon.
Thanks a lot! We didn't know about FastShutdownForPageCount. I'll look into
using it in Opera.
Message was sent while issue was closed.
On 2015/02/23 09:23:20, Fredrik Öhrn wrote:
> On 2015/02/20 17:09:44, Yaron wrote:
> >
> > I found something that might be related and I'm wondering if it explains a
> > difference between Chrome and Opera and why you may be hitting onDestroy
more
> > than Chrome.
> >
> > In the internal repo we have a ChromeTab which subclasses TabAndroid. I'm
> trying
> > to upstream/remove it. In it, we have this tidbit:
> >
> > void ChromeTab::DestroyWebContents(JNIEnv* env,
> > jobject obj,
> > jboolean delete_native) {
> > // Terminate the renderer process if this is the last tab.
> > // If there's no unload listener, FastShutdownForPageCount kills the
> > // renderer process. Otherwise, we go with the slow path where renderer
> > // process shuts down itself when ref count becomes 0.
> >
> > // TODO(tedchoc): Clean up these deletion ordering dependencies and
actually
> > // fix the ownership issue.
> > if (delete_native) {
> > content::RenderProcessHost* process =
> > web_contents()->GetRenderProcessHost();
> > if (process)
> > process->FastShutdownForPageCount(1);
> > }
> >
> > TabAndroid::DestroyWebContents(env,
> > obj,
> > delete_native);
> > }
> >
> > It's not as aggressive as your patch, but it might explain why Chrome
doesn't
> > get to onDestroy as often. It seems like this would cause Chrome to reliably
> > reach SuicideOnChannelErrorFilter and _exit(0). In the event of a
> > beforeUnloadHandler, we could hit ChildProcessService.onDestroy but I assume
> > that's fairly uncommon.
>
> Thanks a lot! We didn't know about FastShutdownForPageCount. I'll look into
> using it in Opera.
If you have a reproducible case, I would be interested to hear if this resolves
it for you. I can certainly upstream this snippet to chromium so you'll get it
for free in the next cycle
Message was sent while issue was closed.
On 2015/02/23 14:46:18, Yaron wrote: > > If you have a reproducible case, I would be interested to hear if this resolves > it for you. I can certainly upstream this snippet to chromium so you'll get it > for free in the next cycle There was a crash when closing pages with video I think was somewhat reproducible but mainly we had a big pile of breakpad crash reports from the field. So to be sure I'll have to try it out in a public beta release and see what shows up in the crash logs. I see that you've already prepared a review to include it upstream, unfortunately it just missed our beta for this cycle by a couple of days or I would have backported it right away. I'll get back to you during the next release cycle, by then we'll definitely have the patch in our tree.
Message was sent while issue was closed.
On 2015/02/23 14:46:18, Yaron wrote: > On 2015/02/23 09:23:20, Fredrik Öhrn wrote: > > > > Thanks a lot! We didn't know about FastShutdownForPageCount. I'll look into > > using it in Opera. > > If you have a reproducible case, I would be interested to hear if this resolves > it for you. I can certainly upstream this snippet to chromium so you'll get it > for free in the next cycle We've been using FastShutdownForPageCount instead of this patch for a while now and it seems to work fine. None of the old shutdown crashes have reappeared in our logs. Thanks!
Message was sent while issue was closed.
On 2015/08/07 10:01:05, Fredrik Öhrn wrote: > On 2015/02/23 14:46:18, Yaron wrote: > > On 2015/02/23 09:23:20, Fredrik Öhrn wrote: > > > > > > Thanks a lot! We didn't know about FastShutdownForPageCount. I'll look into > > > using it in Opera. > > > > If you have a reproducible case, I would be interested to hear if this > resolves > > it for you. I can certainly upstream this snippet to chromium so you'll get it > > for free in the next cycle > > We've been using FastShutdownForPageCount instead of this patch for a while now > and it seems to work fine. None of the old shutdown crashes have reappeared in > our logs. > > Thanks! Thanks for circling back. I'm glad it's working for you |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
