Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
88503002:
Have the unload event execute in background on cross-site navigations (Closed)
|
Created:
7 years ago by clamy Modified:
6 years, 10 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, ojan Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionHave the unload event execute in background on cross-site navigations
Cross-site navigations trigger renderer swap. This CL makes it so that the swap
does not need for the old renderer unload event to be fired. Instead, it will
be executed in the background, and the new renderer will be swapped in
immediately.
BUG=323528
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249676
Patch Set 1 #
Total comments: 2
Patch Set 2 : Keeping the old rvh alive for up to 1s #
Total comments: 10
Patch Set 3 : Removed SwapOutAck #Patch Set 4 : Removed booleans in the RVH #Patch Set 5 : Rebase + fixed bug in refcounting in FrameTree #Patch Set 6 : Added unit tests #
Total comments: 39
Patch Set 7 : Rebase + addressed some of Nasko's comments #
Total comments: 16
Patch Set 8 : Addressed Nasko's comments #
Total comments: 4
Patch Set 9 : Addressed Nasko's comments #
Total comments: 38
Patch Set 10 : Addressed Charlie's comments #Messages
Total messages: 36 (0 generated)
@creis: Please take a look. @pasko: FYI
[+ojan in CC] https://chromiumcodereview.appspot.com/88503002/diff/1/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://chromiumcodereview.appspot.com/88503002/diff/1/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:106: // swapping renderers. The old renderer will allways be sent to the background nit: always As Ojan wondered, have you checked whether a long-running unload handler actually finishes running in the background, or do we kill the process right away? https://chromiumcodereview.appspot.com/88503002/diff/1/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:711: StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kUnloadSwapTimeoutMS)); Moving these calls inside the if seems like a problem. For example, if the render view isn't live, we'll still call decrement_in_flight_event_count() in OnSwappedOut. Also, using a timeout of 0 ms seems like it's keeping a lot of complexity around unnecessarily. It's great for the measurements to confirm that this will help, but if we know we're going down this path, can we remove the use of the timer instead?
On 2013/11/26 19:35:00, creis wrote: > [+ojan in CC] > > https://chromiumcodereview.appspot.com/88503002/diff/1/content/browser/render... > File content/browser/renderer_host/render_view_host_impl.cc (right): > > https://chromiumcodereview.appspot.com/88503002/diff/1/content/browser/render... > content/browser/renderer_host/render_view_host_impl.cc:106: // swapping > renderers. The old renderer will allways be sent to the background > nit: always > > As Ojan wondered, have you checked whether a long-running unload handler > actually finishes running in the background, or do we kill the process right > away? Since you seemed quite sure, I did not test it. On Android, it seems that it gets to execute... sometimes (for example when I tested it 60% of the time). The old renderer seems to get killed when we call WasSwappedOut on the old RenderViewHostImpl, which happens in RenderViewHostManager::CommitPending. This method is at least called by RenderViewHostManager::DidNavigateMainFrame, so I guess that depending on how long the new page takes to load, we may or may not execute it. > > https://chromiumcodereview.appspot.com/88503002/diff/1/content/browser/render... > content/browser/renderer_host/render_view_host_impl.cc:711: > StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kUnloadSwapTimeoutMS)); > Moving these calls inside the if seems like a problem. For example, if the > render view isn't live, we'll still call decrement_in_flight_event_count() in > OnSwappedOut. > > Also, using a timeout of 0 ms seems like it's keeping a lot of complexity around > unnecessarily. It's great for the measurements to confirm that this will help, > but if we know we're going down this path, can we remove the use of the timer > instead? Yes, probably we should just call SwapOut(true), which is what would get called eventually. Based on this two items, I am working on completely redoing the CL, to ensure the old RenderViewHostImpl, and the old renderer will stay around long enough. I think a 1s grace period should be enough to match the current guarantees on the execution of the unload handler. Wdyt?
On 2013/11/27 18:25:39, clamy wrote: > On 2013/11/26 19:35:00, creis wrote: > > [+ojan in CC] > > > > > https://chromiumcodereview.appspot.com/88503002/diff/1/content/browser/render... > > File content/browser/renderer_host/render_view_host_impl.cc (right): > > > > > https://chromiumcodereview.appspot.com/88503002/diff/1/content/browser/render... > > content/browser/renderer_host/render_view_host_impl.cc:106: // swapping > > renderers. The old renderer will allways be sent to the background > > nit: always > > > > As Ojan wondered, have you checked whether a long-running unload handler > > actually finishes running in the background, or do we kill the process right > > away? > > Since you seemed quite sure, I did not test it. On Android, it seems that it > gets to execute... sometimes (for example when I tested it 60% of the time). The > old renderer seems to get killed when we call WasSwappedOut on the old > RenderViewHostImpl, which happens in RenderViewHostManager::CommitPending. This > method is at least called by RenderViewHostManager::DidNavigateMainFrame, so I > guess that depending on how long the new page takes to load, we may or may not > execute it. I'm not sure I follow why it's failing to run in 40% of the cases. If we send the SwapOut message before the WasSwappedOut message, then the renderer process won't get to WasSwappedOut until it finishes running the unload handler. In theory, if you have a while (true) {} in the unload handler, I expect the old renderer process to never exit. There's a bug for that: http://crbug.com/124663, motivated by http://crbug.com/104346. > > > > > https://chromiumcodereview.appspot.com/88503002/diff/1/content/browser/render... > > content/browser/renderer_host/render_view_host_impl.cc:711: > > StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kUnloadSwapTimeoutMS)); > > Moving these calls inside the if seems like a problem. For example, if the > > render view isn't live, we'll still call decrement_in_flight_event_count() in > > OnSwappedOut. > > > > Also, using a timeout of 0 ms seems like it's keeping a lot of complexity > around > > unnecessarily. It's great for the measurements to confirm that this will > help, > > but if we know we're going down this path, can we remove the use of the timer > > instead? > > Yes, probably we should just call SwapOut(true), which is what would get called > eventually. I agree that skipping the timer logic for unload makes sense. This might include some larger changes to how we use the timer. > Based on this two items, I am working on completely redoing the CL, to ensure > the old RenderViewHostImpl, and the old renderer will stay around long enough. I > think a 1s grace period should be enough to match the current guarantees on the > execution of the unload handler. Wdyt? I'm not sure I understand the 1s grace period yet. That makes sense if we're killing the renderer process if it takes too long, but I don't know how that's happening. (We tried to do it in 104346 and gave up.)
On 2013/11/27 20:54:26, creis wrote: > On 2013/11/27 18:25:39, clamy wrote: > > On 2013/11/26 19:35:00, creis wrote: > > > [+ojan in CC] > > > > > > > > > https://chromiumcodereview.appspot.com/88503002/diff/1/content/browser/render... > > > File content/browser/renderer_host/render_view_host_impl.cc (right): > > > > > > > > > https://chromiumcodereview.appspot.com/88503002/diff/1/content/browser/render... > > > content/browser/renderer_host/render_view_host_impl.cc:106: // swapping > > > renderers. The old renderer will allways be sent to the background > > > nit: always > > > > > > As Ojan wondered, have you checked whether a long-running unload handler > > > actually finishes running in the background, or do we kill the process right > > > away? > > > > Since you seemed quite sure, I did not test it. On Android, it seems that it > > gets to execute... sometimes (for example when I tested it 60% of the time). > The > > old renderer seems to get killed when we call WasSwappedOut on the old > > RenderViewHostImpl, which happens in RenderViewHostManager::CommitPending. > This > > method is at least called by RenderViewHostManager::DidNavigateMainFrame, so I > > guess that depending on how long the new page takes to load, we may or may not > > execute it. > > I'm not sure I follow why it's failing to run in 40% of the cases. If we send > the SwapOut message before the WasSwappedOut message, then the renderer process > won't get to WasSwappedOut until it finishes running the unload handler. > > In theory, if you have a while (true) {} in the unload handler, I expect the old > renderer process to never exit. There's a bug for that: > http://crbug.com/124663, motivated by http://crbug.com/104346. > > > > > > > > > > https://chromiumcodereview.appspot.com/88503002/diff/1/content/browser/render... > > > content/browser/renderer_host/render_view_host_impl.cc:711: > > > StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kUnloadSwapTimeoutMS)); > > > Moving these calls inside the if seems like a problem. For example, if the > > > render view isn't live, we'll still call decrement_in_flight_event_count() > in > > > OnSwappedOut. > > > > > > Also, using a timeout of 0 ms seems like it's keeping a lot of complexity > > around > > > unnecessarily. It's great for the measurements to confirm that this will > > help, > > > but if we know we're going down this path, can we remove the use of the > timer > > > instead? > > > > Yes, probably we should just call SwapOut(true), which is what would get > called > > eventually. > > I agree that skipping the timer logic for unload makes sense. This might > include some larger changes to how we use the timer. > > > Based on this two items, I am working on completely redoing the CL, to ensure > > the old RenderViewHostImpl, and the old renderer will stay around long enough. > I > > think a 1s grace period should be enough to match the current guarantees on > the > > execution of the unload handler. Wdyt? > > I'm not sure I understand the 1s grace period yet. That makes sense if we're > killing the renderer process if it takes too long, but I don't know how that's > happening. (We tried to do it in 104346 and gave up.) Well the idea is that we need to keep the old rvh alive for a certain amount of time. After all, the renderer needs to communicate with the browser during the unload event (for network requests/local storage access, which is probably what is done in the unload event). But if it takes too long, we should kill it (to free memory). This is what I tried to implement in the latest patch set.
On 2013/12/06 17:33:50, clamy wrote: > Well the idea is that we need to keep the old rvh alive for a certain amount of > time. After all, the renderer needs to communicate with the browser during the > unload event (for network requests/local storage access, which is probably what > is done in the unload event). But if it takes too long, we should kill it (to > free memory). This is what I tried to implement in the latest patch set. Hmm. I'm part-way through, but I need more time to understand this. I feel like we should be able to simplify this. I'll try finish the review next week.
This doesn't yet match what I was picturing. I'll try to describe what I was thinking, and perhaps we can discuss if that has issues. Currently, SwapOut sends an IPC to the renderer and waits for an ack, giving up after a one-second timer. Once we get to OnSwappedOut, the navigation resumes in the new process and eventually commits. The commit ends up calling WasSwappedOut, which notifies the renderer to decrement its refcount and exit if no one else is using it. By switching to a timeout of zero, we don't need the SwapOut ACK anymore. We can just tell the renderer to SwapOut and skip the timer entirely, proceeding with delegate_->SwappedOut. The new navigation will eventually commit and we'll send a WasSwappedOut message to the old renderer, which can't be processed until it finishes the SwapOut message anyway. That brings us to parity with where we are today, though it leaves the risk that the old process will not exit for a long time. Killing the old process turns out to be extremely hard to do without causing other issues, as Nasko and I learned in http://crbug.com/104346. I don't know what else would cause the old renderer to exit without finishing the unload handler, as you mentioned in an earlier comment. It shouldn't exit on its own until we get to the ReleaseProcess() call in RenderWidget::WasSwappedOut, which happens after unload is finished. Hopefully we can get to a point where the unload handler always finishes running, and we can decide whether to tackle 104346 separately. https://chromiumcodereview.appspot.com/88503002/diff/290001/chrome/browser/re... File chrome/browser/renderer_host/web_cache_manager_browsertest.cc (right): https://chromiumcodereview.appspot.com/88503002/diff/290001/chrome/browser/re... chrome/browser/renderer_host/web_cache_manager_browsertest.cc:60: // We may have two renderers if one is still executing the unload event in the Slight concern: Are we likely to have many flaky tests if we don't know when a renderer is going to go away? https://chromiumcodereview.appspot.com/88503002/diff/290001/content/browser/f... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/88503002/diff/290001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:893: ->active_view_count() && nit: Please keep the arrow at the end of the line, to be consistent with other files in frame_host/. https://chromiumcodereview.appspot.com/88503002/diff/290001/content/browser/f... content/browser/frame_host/render_frame_host_manager.cc:894: !old_render_view_host->should_be_preserved_on_swap_out()) { I'd like to avoid adding this here if possible. https://chromiumcodereview.appspot.com/88503002/diff/290001/content/browser/f... File content/browser/frame_host/render_frame_host_manager.h (right): https://chromiumcodereview.appspot.com/88503002/diff/290001/content/browser/f... content/browser/frame_host/render_frame_host_manager.h:15: #include "content/public/browser/global_request_id.h" nit: Looks unneeded? https://chromiumcodereview.appspot.com/88503002/diff/290001/content/browser/r... File content/browser/renderer_host/render_view_host_impl.cc (right): https://chromiumcodereview.appspot.com/88503002/diff/290001/content/browser/r... content/browser/renderer_host/render_view_host_impl.cc:699: void RenderViewHostImpl::SwapOut() { I don't think we need most of this method, and we can get rid of OnSwapOutACK. Seems like this method should just send the ViewMsg_SwapOut message if IsRenderViewLive(), then call delegate_->SwappedOut. All the timer stuff can go away. https://chromiumcodereview.appspot.com/88503002/diff/290001/content/browser/r... content/browser/renderer_host/render_view_host_impl.cc:749: // The RenderView should not exit yet. It still has to execute the unload This doesn't make sense to me. The renderer won't try to process the WasSwappedOut message until it finishes processing the SwapOut message anyway. There's no reason to avoid sending this. https://chromiumcodereview.appspot.com/88503002/diff/290001/content/browser/r... File content/browser/renderer_host/render_view_host_impl.h (right): https://chromiumcodereview.appspot.com/88503002/diff/290001/content/browser/r... content/browser/renderer_host/render_view_host_impl.h:301: bool should_be_preserved_on_swap_out() const { I don't think we need these. https://chromiumcodereview.appspot.com/88503002/diff/290001/content/browser/r... content/browser/renderer_host/render_view_host_impl.h:341: void OnSwappedOut(bool timed_out, bool preserve_render_view_host); I think we can get rid of this whole method. https://chromiumcodereview.appspot.com/88503002/diff/290001/content/public/br... File content/public/browser/render_process_host.h (right): https://chromiumcodereview.appspot.com/88503002/diff/290001/content/public/br... content/public/browser/render_process_host.h:216: virtual void ResumeDeferredNavigation(const GlobalRequestID& request_id) = 0; I don't think this should be public.
Fly-by comments. I am not expert in this area, so take my comments as just 2c. On 2013/12/09 20:12:29, creis wrote: > This doesn't yet match what I was picturing. I'll try to describe what I was > thinking, and perhaps we can discuss if that has issues. > > Currently, SwapOut sends an IPC to the renderer and waits for an ack, giving up > after a one-second timer. Once we get to OnSwappedOut, the navigation resumes > in the new process and eventually commits. The commit ends up calling > WasSwappedOut, which notifies the renderer to decrement its refcount and exit if > no one else is using it. > > By switching to a timeout of zero, we don't need the SwapOut ACK anymore. We > can just tell the renderer to SwapOut and skip the timer entirely, proceeding > with delegate_->SwappedOut. The new navigation will eventually commit and we'll > send a WasSwappedOut message to the old renderer, which can't be processed until > it finishes the SwapOut message anyway. > > That brings us to parity with where we are today, though it leaves the risk that > the old process will not exit for a long time. Killing the old process turns > out to be extremely hard to do without causing other issues, as Nasko and I > learned in http://crbug.com/104346. The bug looks scary :( Though it seems like the swapout <-> ack ping pong is not adding anything significantly new to interactions. Are you saying that the interactions are now not very well understood? Did the UMA datapoint in comment #45 help reveal the reasons of leftover processes or just the issue disappeared earlier? I like the simplicity of your sugestion: make all swapout actions immediately and forget about the timer. However, a compromised renderer can be pretty DoSy by just ignoring the swappedout instructions and consuming CPU. How serious is it?
https://chromiumcodereview.appspot.com/88503002/diff/290001/content/browser/r... File content/browser/renderer_host/render_view_host_impl.cc (right): https://chromiumcodereview.appspot.com/88503002/diff/290001/content/browser/r... content/browser/renderer_host/render_view_host_impl.cc:755: need_to_perform_clean_up_on_swapped_out_ = false; when is it the case that (should_be_preserved_on_swap_out_ != need_to_perform_clean_up_on_swapped_out_)?
On 2013/12/10 19:31:47, pasko wrote: > Fly-by comments. I am not expert in this area, so take my comments as just 2c. > > On 2013/12/09 20:12:29, creis wrote: > > This doesn't yet match what I was picturing. I'll try to describe what I was > > thinking, and perhaps we can discuss if that has issues. > > > > Currently, SwapOut sends an IPC to the renderer and waits for an ack, giving > up > > after a one-second timer. Once we get to OnSwappedOut, the navigation resumes > > in the new process and eventually commits. The commit ends up calling > > WasSwappedOut, which notifies the renderer to decrement its refcount and exit > if > > no one else is using it. > > > > By switching to a timeout of zero, we don't need the SwapOut ACK anymore. We > > can just tell the renderer to SwapOut and skip the timer entirely, proceeding > > with delegate_->SwappedOut. The new navigation will eventually commit and > we'll > > send a WasSwappedOut message to the old renderer, which can't be processed > until > > it finishes the SwapOut message anyway. > > > > That brings us to parity with where we are today, though it leaves the risk > that > > the old process will not exit for a long time. Killing the old process turns > > out to be extremely hard to do without causing other issues, as Nasko and I > > learned in http://crbug.com/104346. > > The bug looks scary :( > > Though it seems like the swapout <-> ack ping pong is not adding anything > significantly > new to interactions. Are you saying that the interactions are now not very well > understood? Did the UMA datapoint in comment #45 help reveal the reasons of > leftover > processes or just the issue disappeared earlier? I'm not sure I understand the question, and unfortunately the UMA dashboard is down at the moment, so I can't check for the BrowserRenderProcessHost.ChildKillsUnresponsive results. I seem to remember that the problems we ran into involved tests and unexplained crash statistics. At least on desktop, we decided to let the user discover and kill runaway processes instead in http://crbug.com/124663, but we haven't gotten around to implementing it. > > I like the simplicity of your sugestion: make all swapout actions immediately > and forget about the timer. However, a compromised renderer can be pretty DoSy > by just > ignoring the swappedout instructions and consuming CPU. How serious is it? I'm not concerned about that. A compromised renderer can do much worse things than use CPU. In fact, it doesn't even need to be compromised. Just put while (true) {} in an unload handler and it will eat up CPU. That will go away when the tab goes away, though.
On 2013/12/10 20:47:51, creis wrote:
> > I like the simplicity of your sugestion: make all swapout actions
immediately
> > and forget about the timer. However, a compromised renderer can be pretty
DoSy
> > by just
> > ignoring the swappedout instructions and consuming CPU. How serious is it?
>
> I'm not concerned about that. A compromised renderer can do much worse things
> than use CPU. In fact, it doesn't even need to be compromised. Just put
while
> (true) {} in an unload handler and it will eat up CPU. That will go away when
> the tab goes away, though.
The while(true) is reported in task manager, that's different.
I would argue that those much worse things are limited in time (1 sec). Though
it can DoS Chrome with RPCs and with that prevent itself from being killed,
right? Here it would invisibly occupy one core forever, that's different, but
maybe not so much. Just curious, in your proposal will the infinite unload
renderer remain alive when a user exits the browser?
Anyway, sounds like a narrow scenario not very interesting to attackers. I'm
happy, the code would look simpler.
On 2013/12/10 22:46:42, pasko wrote:
> On 2013/12/10 20:47:51, creis wrote:
> > > I like the simplicity of your sugestion: make all swapout actions
> immediately
> > > and forget about the timer. However, a compromised renderer can be pretty
> DoSy
> > > by just
> > > ignoring the swappedout instructions and consuming CPU. How serious is it?
> >
> > I'm not concerned about that. A compromised renderer can do much worse
things
> > than use CPU. In fact, it doesn't even need to be compromised. Just put
> while
> > (true) {} in an unload handler and it will eat up CPU. That will go away
when
> > the tab goes away, though.
>
> The while(true) is reported in task manager, that's different.
>
> I would argue that those much worse things are limited in time (1 sec).
Sorry, I don't understand this statement. Nothing kills the renderer process
after 1 second if it continues to run, whether it's because of a busy loop or an
IPC flood. That's what bug 104346 was about, and why we want to show these
runaway processes in bug 124663.
In other words, while (true) {} in an unload handler will cause the process to
use 100% CPU forever, until the tab is closed or Chrome exits. You can verify
this on desktop using the OS process manager, since the runaway process ceases
to show up in Chrome's task manager after you navigate away.
> Though
> it can DoS Chrome with RPCs and with that prevent itself from being killed,
> right?
There's two cases I think you're conflating here. The renderer process may
never choose to exit on its own, but the browser process can always kill it
(e.g., task manager, frozen renderer dialog, or upon receiving an illegally
formatted IPC message). The renderer process will also reliably be killed if
the user closes all tabs using it or exits the browser.
> Here it would invisibly occupy one core forever, that's different, but
> maybe not so much. Just curious, in your proposal will the infinite unload
> renderer remain alive when a user exits the browser?
No. If all tabs using the process are closed or Chrome exits, the renderer
process will go away.
> Anyway, sounds like a narrow scenario not very interesting to attackers. I'm
> happy, the code would look simpler.
Great. I think that's the right plan to proceed with, and we can pursue the
runaway process problem if it later proves to be necessary.
I have uploaded an initial version of the CL, which should be quite close to what you are proposing. While the design is much simpler, I am afraid it broke some of the requirements put on the execution of the unload event. Namely, we end up deleting the RenderViewHost immediately after sending the SwappedOut IPC to the renderer, which causes two problems. Problem 1: the renderer seems to be unable to send IPCs to the browser anymore. But at that time the renderer is still busy trying to execute the unload event. Which means that the unload event cannot make a network request, or use local storage. This makes the unload event equivalent to a noop since it cannot possibly have any noticeable side effect. Problem 2: on Android, deleting the rvh kills the renderer process. I don't know why, and based on the bug you've linked, it seems to be different on desktop. Renderer processes management is quite different on Android, and killing renderers is not exactly a problem (but keeping them alive is :) ). So let me expand on the considerations that led to Patch Set 2: Based on the two aforementioned items, I concluded that we needed to keep the old rvh alive, but switched to background. Since we can't keep it forever, we probably wanted to be notified when it was safe to delete it, that is when the unload event finished executing in the renderer. Which is basically the SwapOutAck. Also, looking from the Android side, we don't want to keep two renderers for too long, since memory consumption is critical. Therefore, I thought keeping the timeout would be good, since we appear to kill renderers easily on Android. In the end, the logic pretty much looked like what exists nowadays, with the exception that we want to switch rvh immediately, and store the old one for a little while. If you think that patchset 3 is simpler and that this is better, we can go ahead with it. I am just worried that it is breaking the requirements of the unload event. I.e. we likely have to cope with some additional complexity.
On 2013/12/11 16:21:14, clamy wrote: > I have uploaded an initial version of the CL, which should be quite close to > what you are proposing. While the design is much simpler, I am afraid it broke > some of the requirements put on the execution of the unload event. Namely, we > end up deleting the RenderViewHost immediately after sending the SwappedOut IPC > to the renderer, which causes two problems. > > Problem 1: the renderer seems to be unable to send IPCs to the browser anymore. > But at that time the renderer is still busy trying to execute the unload event. > Which means that the unload event cannot make a network request, or use local > storage. This makes the unload event equivalent to a noop since it cannot > possibly have any noticeable side effect. I see. Even though it's running in the background, we still have to keep its RenderViewHost around until it's done (or a timeout), to service things like network requests. Ok, patchset 3 probably won't work. > Problem 2: on Android, deleting the rvh kills the renderer process. I don't know > why, and based on the bug you've linked, it seems to be different on desktop. > Renderer processes management is quite different on Android, and killing > renderers is not exactly a problem (but keeping them alive is :) ). > > So let me expand on the considerations that led to Patch Set 2: > > Based on the two aforementioned items, I concluded that we needed to keep the > old rvh alive, but switched to background. Since we can't keep it forever, we > probably wanted to be notified when it was safe to delete it, that is when the > unload event finished executing in the renderer. Which is basically the > SwapOutAck. Also, looking from the Android side, we don't want to keep two > renderers for too long, since memory consumption is critical. Therefore, I > thought keeping the timeout would be good, since we appear to kill renderers > easily on Android. In the end, the logic pretty much looked like what exists > nowadays, with the exception that we want to switch rvh immediately, and store > the old one for a little while. I think we're going to need something more explicit and specific to this case than simply reusing the old hang monitor timeout. For example, it wouldn't make sense to show a hang dialog for the process in the background running an unload handler. Maybe we can do something with the same intention as patch set 2 but cleaner, without the artifacts of the old approach. I'm sorry that I don't have specific design suggestions at the moment. > > If you think that patchset 3 is simpler and that this is better, we can go ahead > with it. I am just worried that it is breaking the requirements of the unload > event. I.e. we likely have to cope with some additional complexity.
+nasko@ and ppi@
On 2013/12/16 22:21:12, clamy wrote: > +nasko@ and ppi@ @nasko: I have uploaded my latest version, which is still in development.
@nasko: PTAL I have cleaned up the development version, and the patch should now be reviewable. There is one remaining issue on a browser test that I intend to look at tomorrow, as well as start writing additional tests.
I'm sending you some of the comments, as I need to review the test files as well. I'll send those later. https://chromiumcodereview.appspot.com/88503002/diff/1180001/chrome/test/base... File chrome/test/base/browser_with_test_window_test.cc (right): https://chromiumcodereview.appspot.com/88503002/diff/1180001/chrome/test/base... chrome/test/base/browser_with_test_window_test.cc:153: // Simulate the SwapOut_ACK from the renderer. Why did this move? https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... File content/browser/frame_host/frame_tree.h (right): https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/frame_host/frame_tree.h:153: RenderViewHostMultiMap render_view_host_pending_shutdown_map_; Why is this a multimap? There should be only one RVH per SiteInstance in a FrameTree. https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:242: current_host()->SwapOut(); Calling SwapOut() will cause us to run the unload handler again. Is this desired? Also, why are we calling OnSwappedOut right after that with false? The parameter is "timed out", which is consistent with the previous version. https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:585: pending_delete_iter->second.get() != iter->second) { What happens if there is already an entry for this site_instance_id and we overwrite its entry in the map? https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:1083: // If the RenderViewHost backing the RenderFrameHost is pending shutdown, nit: You are mixing terminology - pending shutdown and pending_delete_hosts. Is it pure delete or do we need to call shutdown on those? https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... File content/browser/renderer_host/render_view_host_impl.cc (left): https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/renderer_host/render_view_host_impl.cc:727: StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kUnloadTimeoutMS)); Why is this removed? It was balanced by StopMonitorTimeout in OnSwappedOut, which is not removed. https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... File content/browser/renderer_host/render_view_host_impl.cc (right): https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/renderer_host/render_view_host_impl.cc:819: (rvh_state_ == RVH_STATE_LIVE && !IsRenderViewLive())) { I'm not sure I understand what the STATE_LIVE check does. https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/renderer_host/render_view_host_impl.cc:1489: if (IsWaitingForUnloadACK()) { nit: No need for {}. https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... File content/browser/renderer_host/render_view_host_impl.h (left): https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:287: bool is_swapped_out() const { return is_swapped_out_; } nit: This might be useful method to keep around as an easy way to check for swapped out state. https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... File content/browser/renderer_host/render_view_host_impl.h (right): https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:104: enum RenderViewHostImplState { Having a good comment for each of those states will be awesome. Since this is hard to understand area, anything we can do to help our future selves will be great! https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:105: RVH_STATE_LIVE = 0, nit: Drop the RVH_ prefix, as this is already accessed through RenderViewHostImpl:: prefix. https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:631: void SetRVHState(RenderViewHostImplState rvh_state); nit: Why not just SetState? The method is already called on RVH object, so that part is redundant. https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:635: // The number of RenderFrameHost which have a pointer to this RVH. nit: s/pointer/reference/ https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:636: int ref_count_; The name implies generic ref count, which this isn't. Maybe call it frames_ref_count_? https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/public/b... File content/public/browser/render_process_host.h (right): https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/public/b... content/public/browser/render_process_host.h:223: virtual void ResumeDeferredNavigation(const GlobalRequestID& request_id) = 0; Why is this a public method now? I don't see anyone outside of content using it in your patch.
Thanks for your comments :) ! https://chromiumcodereview.appspot.com/88503002/diff/1180001/chrome/test/base... File chrome/test/base/browser_with_test_window_test.cc (right): https://chromiumcodereview.appspot.com/88503002/diff/1180001/chrome/test/base... chrome/test/base/browser_with_test_window_test.cc:153: // Simulate the SwapOut_ACK from the renderer. On 2014/01/23 23:34:58, nasko wrote: > Why did this move? It was causing a problem with some tests in unit_tests (the RVH would not end up in the right state and would not be deleted). However, it must have come from another underlying issue that was fixed, since removing this change does not seem to affect tests at all. I undid the move. https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... File content/browser/frame_host/frame_tree.h (right): https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/frame_host/frame_tree.h:153: RenderViewHostMultiMap render_view_host_pending_shutdown_map_; On 2014/01/23 23:34:58, nasko wrote: > Why is this a multimap? There should be only one RVH per SiteInstance in a > FrameTree. It seems that we can end up reusing the same site instanec id at least, which can cause us to have at least one active RVH and one RVH pending shutdown for the same site instance id (that happens in one of the browser tests). Therefore, I was worried about corner cases where we would navigate fast enough so that we end up with two RVH pending shutdown and sharing the same site instance id. https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:242: current_host()->SwapOut(); On 2014/01/23 23:34:58, nasko wrote: > Calling SwapOut() will cause us to run the unload handler again. Is this > desired? > Also, why are we calling OnSwappedOut right after that with false? The parameter > is "timed out", which is consistent with the previous version. Done. https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:585: pending_delete_iter->second.get() != iter->second) { On 2014/01/23 23:34:58, nasko wrote: > What happens if there is already an entry for this site_instance_id and we > overwrite its entry in the map? Since it is stored in a linked_ptr, the only ref on the linked_ptr goes away and the RFH is detroyed. If it was the last RFH using the RVH, then Shutdown is called on the RVH. https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:1083: // If the RenderViewHost backing the RenderFrameHost is pending shutdown, On 2014/01/23 23:34:58, nasko wrote: > nit: You are mixing terminology - pending shutdown and pending_delete_hosts. Is > it pure delete or do we need to call shutdown on those? We call delete on the RenderFrameHost, which will eventually lead Shutdown to be called on the RenderViewHost. Shutdwon needs to be called on the RVH, but we are keeping RFH in the map, which are only deleted. Should it be called pending_shutdown_hosts_? https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... File content/browser/renderer_host/render_view_host_impl.cc (left): https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/renderer_host/render_view_host_impl.cc:727: StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kUnloadTimeoutMS)); On 2014/01/23 23:34:58, nasko wrote: > Why is this removed? It was balanced by StopMonitorTimeout in OnSwappedOut, > which is not removed. creis expressed a preference towards not using the hang monitor for the timeout, which is why I introduced a simple timeout monitor to keep track of the time taken by the unload event. I kept the StopHangMonitorTimeout in OnSwappedOut, because I thought that the hang monitor needed to be stopped at that point (we don't really care that the renderer is hung anymore). Was it stopped before the cross-site transition? If not, should the increment_in_flight_event_count() be kept? https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... File content/browser/renderer_host/render_view_host_impl.cc (right): https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/renderer_host/render_view_host_impl.cc:819: (rvh_state_ == RVH_STATE_LIVE && !IsRenderViewLive())) { On 2014/01/23 23:34:58, nasko wrote: > I'm not sure I understand what the STATE_LIVE check does. When the RenderView is not live, the RenderFrameHostManager will call commit directly on the live RVH. Therefore WasSwappedOut is called immediately. I put the test in as a test for correctness. Alternatively, I could do: } else if (rvh_state_ == RVH_STATE_WAITING_FOR_COMMIT) { SetRVHState(RVH_STATE_SWAPPED_OUT); } else if (rvh_state_ == RVH_STATE_LIVE) { DCHECK(!IsRenderViewLive()); SetRVHState(RVH_STATE_SWAPPED_OUT); } https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/renderer_host/render_view_host_impl.cc:1489: if (IsWaitingForUnloadACK()) { On 2014/01/23 23:34:58, nasko wrote: > nit: No need for {}. Done. https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... File content/browser/renderer_host/render_view_host_impl.h (left): https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:287: bool is_swapped_out() const { return is_swapped_out_; } On 2014/01/23 23:34:58, nasko wrote: > nit: This might be useful method to keep around as an easy way to check for > swapped out state. Do we want it to return true only for STATE_SWAPPED_OUT, or include STATE_PENDING_SWAP_OUT as well? When the RVH is in STATE_PENDING_SWAP_OUT, it is stored in the map of swapped_out_hosts in the RenderFrameHostManager (though it does not filter all IPCs as in STATE_SWAPPED_OUT). https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... File content/browser/renderer_host/render_view_host_impl.h (right): https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:104: enum RenderViewHostImplState { On 2014/01/23 23:34:58, nasko wrote: > Having a good comment for each of those states will be awesome. Since this is > hard to understand area, anything we can do to help our future selves will be > great! Done. https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:105: RVH_STATE_LIVE = 0, On 2014/01/23 23:34:58, nasko wrote: > nit: Drop the RVH_ prefix, as this is already accessed through > RenderViewHostImpl:: prefix. Done. https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:631: void SetRVHState(RenderViewHostImplState rvh_state); On 2014/01/23 23:34:58, nasko wrote: > nit: Why not just SetState? The method is already called on RVH object, so that > part is redundant. Done. https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:635: // The number of RenderFrameHost which have a pointer to this RVH. On 2014/01/23 23:34:58, nasko wrote: > nit: s/pointer/reference/ Done. https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:636: int ref_count_; On 2014/01/23 23:34:58, nasko wrote: > The name implies generic ref count, which this isn't. Maybe call it > frames_ref_count_? Done. https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/public/b... File content/public/browser/render_process_host.h (right): https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/public/b... content/public/browser/render_process_host.h:223: virtual void ResumeDeferredNavigation(const GlobalRequestID& request_id) = 0; On 2014/01/23 23:34:58, nasko wrote: > Why is this a public method now? I don't see anyone outside of content using it > in your patch. The method used to be in RenderProcessHostImpl, where it was public. It is called in RenderFrameHostManager::SwappedOut where we static_cast a RenderProcessHost into a RenderProcessHostImpl. In one of the unit_tests, we use MockRenderProcessHost and happen, because of the patch, to hit this particular cast when we previously did not (because the call to delegate_->SwappedOut(this) in the RVH moved from OnSwappedOut to SwapOut). Since MockRenderProcessHost is not derived from RenderProcessHostImpl, this was causing problems, and I had to move this specific method to the parent class of RenderProcessHostImpl and MockRenderProcessHost, that is here, in RenderProcessHost.
Replied to some of your comments and reviewed the tests. I think it is shaping up well. Considering Charlie is coming back on Thursday, it will be best for him to do the content/ OWNERS review. https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:1083: // If the RenderViewHost backing the RenderFrameHost is pending shutdown, On 2014/01/24 17:01:54, clamy wrote: > On 2014/01/23 23:34:58, nasko wrote: > > nit: You are mixing terminology - pending shutdown and pending_delete_hosts. > Is > > it pure delete or do we need to call shutdown on those? > > We call delete on the RenderFrameHost, which will eventually lead Shutdown to be > called on the RenderViewHost. Shutdwon needs to be called on the RVH, but we are > keeping RFH in the map, which are only deleted. Should it be called > pending_shutdown_hosts_? I think it is best we keep it using "delete", since RVH will eventually disappear and currently RFH uses delete, not shutdown. https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... File content/browser/renderer_host/render_view_host_impl.cc (left): https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/renderer_host/render_view_host_impl.cc:727: StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kUnloadTimeoutMS)); On 2014/01/24 17:01:54, clamy wrote: > On 2014/01/23 23:34:58, nasko wrote: > > Why is this removed? It was balanced by StopMonitorTimeout in OnSwappedOut, > > which is not removed. > > creis expressed a preference towards not using the hang monitor for the timeout, > which is why I introduced a simple timeout monitor to keep track of the time > taken by the unload event. > > I kept the StopHangMonitorTimeout in OnSwappedOut, because I thought that the > hang monitor needed to be stopped at that point (we don't really care that the > renderer is hung anymore). Was it stopped before the cross-site transition? If > not, should the increment_in_flight_event_count() be kept? If we aren't using the hang monitor anymore, then we shouldn't be stopping it in OnSwappedOut, should we? If we don't start it, I don't think we should be stopping it. Since we are going with a separate timeout monitor, we shouldn't be touching the in-flight even count either. https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... File content/browser/renderer_host/render_view_host_impl.cc (right): https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/renderer_host/render_view_host_impl.cc:819: (rvh_state_ == RVH_STATE_LIVE && !IsRenderViewLive())) { On 2014/01/24 17:01:54, clamy wrote: > On 2014/01/23 23:34:58, nasko wrote: > > I'm not sure I understand what the STATE_LIVE check does. > > When the RenderView is not live, the RenderFrameHostManager will call commit > directly on the live RVH. Therefore WasSwappedOut is called immediately. I put > the test in as a test for correctness. Alternatively, I could do: > > } else if (rvh_state_ == RVH_STATE_WAITING_FOR_COMMIT) { > SetRVHState(RVH_STATE_SWAPPED_OUT); > } else if (rvh_state_ == RVH_STATE_LIVE) { > DCHECK(!IsRenderViewLive()); > SetRVHState(RVH_STATE_SWAPPED_OUT); > } It will be better to have them separate. Also, it will be great to have this comment in the code, so it is easier for people who didn't write the code to understand the case under which this occurs. https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... File content/browser/renderer_host/render_view_host_impl.h (left): https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:287: bool is_swapped_out() const { return is_swapped_out_; } On 2014/01/24 17:01:54, clamy wrote: > On 2014/01/23 23:34:58, nasko wrote: > > nit: This might be useful method to keep around as an easy way to check for > > swapped out state. > > Do we want it to return true only for STATE_SWAPPED_OUT, or include > STATE_PENDING_SWAP_OUT as well? When the RVH is in STATE_PENDING_SWAP_OUT, it is > stored in the map of swapped_out_hosts in the RenderFrameHostManager (though it > does not filter all IPCs as in STATE_SWAPPED_OUT). Based on the replaced usages, it looks that returning only true for STATE_SWAPPED_OUT is the best thing. Also, it will match the name and be conceptually simple. https://chromiumcodereview.appspot.com/88503002/diff/1270001/content/browser/... File content/browser/frame_host/render_frame_host_manager_unittest.cc (left): https://chromiumcodereview.appspot.com/88503002/diff/1270001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1035: EXPECT_FALSE(rvh2->is_waiting_for_unload_ack()); Why is this expectation dropped? https://chromiumcodereview.appspot.com/88503002/diff/1270001/content/browser/... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/88503002/diff/1270001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:299: EXPECT_NE(RenderViewHostImpl::STATE_LIVE, ntp_rvh->rvh_state()); Shouldn't this be test for states where we filter IPCs? https://chromiumcodereview.appspot.com/88503002/diff/1270001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1042: EXPECT_EQ(RenderViewHostImpl::STATE_PENDING_SWAP_OUT, rvh1->rvh_state()); Why not add a call to OnSwapOut() and then check that we have moved rvh1 to swapped out state? It will make the test match a bit closer what it used to test. https://chromiumcodereview.appspot.com/88503002/diff/1270001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1344: class RenderViewHostDestructionChecker : public WebContentsObserver { nit: s/Checker/Observer/ https://chromiumcodereview.appspot.com/88503002/diff/1270001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1390: rvh2, nit: you can put multiple arguments on one line to save some vertical space https://chromiumcodereview.appspot.com/88503002/diff/1270001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1417: // Increment the number of active views in SiteInstanceImpl so that rvh2 is The code following seems like a separate test case. Since this is unit tests and it is cheap to start new test, might as well make it separate. https://chromiumcodereview.appspot.com/88503002/diff/1270001/content/browser/... File content/browser/renderer_host/render_view_host_impl.h (right): https://chromiumcodereview.appspot.com/88503002/diff/1270001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:121: // zero. Upon reeception of the SwapOutACK, the RVH will be swapped out. nit: "reeception" < extra 'e'? https://chromiumcodereview.appspot.com/88503002/diff/1270001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:125: // zero. Upon reeception of the SwapOutACK, the RVH will be shutdown. nit: same as above, extra 'e'
Thanks for the review :) - I tried to address all of your comments with the new patchset. https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... File content/browser/renderer_host/render_view_host_impl.cc (left): https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/renderer_host/render_view_host_impl.cc:727: StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kUnloadTimeoutMS)); On 2014/01/27 19:09:51, nasko wrote: > On 2014/01/24 17:01:54, clamy wrote: > > On 2014/01/23 23:34:58, nasko wrote: > > > Why is this removed? It was balanced by StopMonitorTimeout in OnSwappedOut, > > > which is not removed. > > > > creis expressed a preference towards not using the hang monitor for the > timeout, > > which is why I introduced a simple timeout monitor to keep track of the time > > taken by the unload event. > > > > I kept the StopHangMonitorTimeout in OnSwappedOut, because I thought that the > > hang monitor needed to be stopped at that point (we don't really care that the > > renderer is hung anymore). Was it stopped before the cross-site transition? If > > not, should the increment_in_flight_event_count() be kept? > > If we aren't using the hang monitor anymore, then we shouldn't be stopping it in > OnSwappedOut, should we? If we don't start it, I don't think we should be > stopping it. Since we are going with a separate timeout monitor, we shouldn't be > touching the in-flight even count either. Done. https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... File content/browser/renderer_host/render_view_host_impl.cc (right): https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/renderer_host/render_view_host_impl.cc:819: (rvh_state_ == RVH_STATE_LIVE && !IsRenderViewLive())) { On 2014/01/27 19:09:51, nasko wrote: > On 2014/01/24 17:01:54, clamy wrote: > > On 2014/01/23 23:34:58, nasko wrote: > > > I'm not sure I understand what the STATE_LIVE check does. > > > > When the RenderView is not live, the RenderFrameHostManager will call commit > > directly on the live RVH. Therefore WasSwappedOut is called immediately. I put > > the test in as a test for correctness. Alternatively, I could do: > > > > } else if (rvh_state_ == RVH_STATE_WAITING_FOR_COMMIT) { > > SetRVHState(RVH_STATE_SWAPPED_OUT); > > } else if (rvh_state_ == RVH_STATE_LIVE) { > > DCHECK(!IsRenderViewLive()); > > SetRVHState(RVH_STATE_SWAPPED_OUT); > > } > > It will be better to have them separate. Also, it will be great to have this > comment in the code, so it is easier for people who didn't write the code to > understand the case under which this occurs. Done. https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... File content/browser/renderer_host/render_view_host_impl.h (left): https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:287: bool is_swapped_out() const { return is_swapped_out_; } On 2014/01/27 19:09:51, nasko wrote: > On 2014/01/24 17:01:54, clamy wrote: > > On 2014/01/23 23:34:58, nasko wrote: > > > nit: This might be useful method to keep around as an easy way to check for > > > swapped out state. > > > > Do we want it to return true only for STATE_SWAPPED_OUT, or include > > STATE_PENDING_SWAP_OUT as well? When the RVH is in STATE_PENDING_SWAP_OUT, it > is > > stored in the map of swapped_out_hosts in the RenderFrameHostManager (though > it > > does not filter all IPCs as in STATE_SWAPPED_OUT). > > Based on the replaced usages, it looks that returning only true for > STATE_SWAPPED_OUT is the best thing. Also, it will match the name and be > conceptually simple. Done. https://chromiumcodereview.appspot.com/88503002/diff/1270001/content/browser/... File content/browser/frame_host/render_frame_host_manager_unittest.cc (left): https://chromiumcodereview.appspot.com/88503002/diff/1270001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1035: EXPECT_FALSE(rvh2->is_waiting_for_unload_ack()); On 2014/01/27 19:09:51, nasko wrote: > Why is this expectation dropped? If the RVH is in STATE_PENDING_SWAP_OUT then IsWaitingForUnloadAck is true. Since I dropped the boolean is_waiting_for_unload_ack_, IsWaitingForUnloadAck returns true when the RVH is in STATE_WAITING_FOR_UNLOAD_ACK, STATE_WAITING_FOR_CLOSE, STATE_PENDING_SWAP_OUT, STATE_PENDING_SHUTDOWN. https://chromiumcodereview.appspot.com/88503002/diff/1270001/content/browser/... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/88503002/diff/1270001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:299: EXPECT_NE(RenderViewHostImpl::STATE_LIVE, ntp_rvh->rvh_state()); On 2014/01/27 19:09:51, nasko wrote: > Shouldn't this be test for states where we filter IPCs? Done. https://chromiumcodereview.appspot.com/88503002/diff/1270001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1042: EXPECT_EQ(RenderViewHostImpl::STATE_PENDING_SWAP_OUT, rvh1->rvh_state()); On 2014/01/27 19:09:51, nasko wrote: > Why not add a call to OnSwapOut() and then check that we have moved rvh1 to > swapped out state? It will make the test match a bit closer what it used to > test. Done. https://chromiumcodereview.appspot.com/88503002/diff/1270001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1344: class RenderViewHostDestructionChecker : public WebContentsObserver { On 2014/01/27 19:09:51, nasko wrote: > nit: s/Checker/Observer/ Done. https://chromiumcodereview.appspot.com/88503002/diff/1270001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1390: rvh2, On 2014/01/27 19:09:51, nasko wrote: > nit: you can put multiple arguments on one line to save some vertical space Done. https://chromiumcodereview.appspot.com/88503002/diff/1270001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1417: // Increment the number of active views in SiteInstanceImpl so that rvh2 is On 2014/01/27 19:09:51, nasko wrote: > The code following seems like a separate test case. Since this is unit tests and > it is cheap to start new test, might as well make it separate. Done. https://chromiumcodereview.appspot.com/88503002/diff/1270001/content/browser/... File content/browser/renderer_host/render_view_host_impl.h (right): https://chromiumcodereview.appspot.com/88503002/diff/1270001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:121: // zero. Upon reeception of the SwapOutACK, the RVH will be swapped out. On 2014/01/27 19:09:51, nasko wrote: > nit: "reeception" < extra 'e'? Done. https://chromiumcodereview.appspot.com/88503002/diff/1270001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:125: // zero. Upon reeception of the SwapOutACK, the RVH will be shutdown. On 2014/01/27 19:09:51, nasko wrote: > nit: same as above, extra 'e' Done.
Just a couple of minor comments and I think it is ready for Charlie to poke at it. Thanks for your hard work on this! https://chromiumcodereview.appspot.com/88503002/diff/1610001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (left): https://chromiumcodereview.appspot.com/88503002/diff/1610001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:947: if (!rvh_impl->is_swapped_out()) { Since we brought back IsSwappedOut, why not use it here and through the rest of the CL? https://chromiumcodereview.appspot.com/88503002/diff/1610001/content/browser/... File content/browser/renderer_host/render_view_host_impl.cc (right): https://chromiumcodereview.appspot.com/88503002/diff/1610001/content/browser/... content/browser/renderer_host/render_view_host_impl.cc:821: // Commit directly, without calling SwapOut on the old RVH. This will cause nit: Did you mean "call CommitPending"? Or something else?
Thanks! We're hoping to land this ASAP as we expect some nice performance improvements :) https://chromiumcodereview.appspot.com/88503002/diff/1610001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (left): https://chromiumcodereview.appspot.com/88503002/diff/1610001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:947: if (!rvh_impl->is_swapped_out()) { On 2014/01/29 22:49:53, nasko wrote: > Since we brought back IsSwappedOut, why not use it here and through the rest of > the CL? Done. https://chromiumcodereview.appspot.com/88503002/diff/1610001/content/browser/... File content/browser/renderer_host/render_view_host_impl.cc (right): https://chromiumcodereview.appspot.com/88503002/diff/1610001/content/browser/... content/browser/renderer_host/render_view_host_impl.cc:821: // Commit directly, without calling SwapOut on the old RVH. This will cause On 2014/01/29 22:49:53, nasko wrote: > nit: Did you mean "call CommitPending"? Or something else? Done.
Glad to see you've all been making progress on this! My first day back from leave was a bit crazy, but I've set aside tomorrow morning to go through it. I'll get you comments as soon as I can.
This looks like a great overhaul. It's a bit complex in places (like the multimap and callbacks), but I think I follow the reasoning for it. I especially appreciate the thorough unit tests. Most of the comments below are nits to improve clarity. Unless Nasko has further questions, LGTM. https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... File content/browser/frame_host/frame_tree.cc (right): https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/frame_host/frame_tree.cc:183: // in the map of RenderViewHost pending shutdown. Otherwise there should not nit: RenderViewHosts https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/frame_host/frame_tree.cc:184: // be a RenderViewHost for the site instance. nit: SiteInstance https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/frame_host/frame_tree.cc:253: if (multi_iter->second != render_frame_host->render_view_host()) nit: Let's go ahead and put multi_iter->second into a rvh local variable, which will make this code a bit easier to read. https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... File content/browser/frame_host/frame_tree.h (right): https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/frame_host/frame_tree.h:145: // it allows us to call Shutdown on the RenderViewHost and remove it from the This second part about "allows us to call Shutdown" is a little unclear now, since the refcount has been moved to RVH itself. Can you rephrase it to "Combined with the refcount on RenderViewHost, this also allows us to call Shutdown on..."? https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/frame_host/frame_tree.h:152: // Map of SiteInstance ID to RenderViewHosts that are pending shutdown. Yes, I can see how you might need this. Can you add more explanation of why it's needed in the comment, though? It will help others who haven't read your overview document. https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... File content/browser/frame_host/render_frame_host_impl.h (right): https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/frame_host/render_frame_host_impl.h:83: // when the swap out ack is received. nit: SwapOutACK https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:546: base::hash_map<int32, linked_ptr<RenderFrameHostImpl> >::iterator iter = RFHPendingDeleteMap::iterator https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:1116: // the RenderFrameHost should be put in the map of RenderFrameHost pending map of RenderFrameHosts https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:1120: RenderViewHostImpl::STATE_PENDING_SHUTDOWN) { nit: 4 more spaces https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... File content/browser/renderer_host/render_view_host_impl.h (right): https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:104: // Keeps track of the state of the RenderViewHostImpl, more particularly nit: more particularly during -> particularly with respect to https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:109: STATE_LIVE = 0, This name is a bit confusing, since we'll be in this state for crashed RenderViewHosts (in which case IsRenderViewLive() will return false). Maybe it should be something like STATE_DEFAULT? https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:120: // committed. The number of active views of the RVH SiteInstanceImpl is not new page has committed in a different RVH. https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:124: // committed. The number of active views of the RVH SiteInstanceImpl is new page has committed in a different RVH. https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:127: // The RVH is swapped out, it is being used as a placeholder to allow for nit: swapped out, and it is https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:363: // when the swap out ack is received, or when the unload timer times out. nit: SwapOutACK https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:657: // The number of RenderFrameHost which have a reference to this RVH. nit: RenderFrameHosts https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:717: bool is_waiting_for_beforeunload_ack_; Should we consider moving this into the state machine in a future CL? (No need to do that here, since this CL is already a large enough change. Might be worth a TODO if so, though.) https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:756: // execute. Can you mention whether this is used in the case that the RVH is going to be swapped out rather than shut down? https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:759: // Called after receiving the swap out ack when the RVH is in state pending nit: SwapOutACK
I don't have anything else other than a great job! LGTM
@creis, creis: Thank you very much for your detailed and speedy review! It has been a pleasure to work with both of you :)! I was traveling on Monday and today, so I could not get much done, but I will upload a new patchset tomorrow to take into account your comments. @jam: PTAL at the changes in chrome/browser/renderer_host/webcache_manager_browsertest.cc. This CL is making renderers execute the unload event in background, which means that we can now have two active renderers (with one executing the unload event) when there previously was only one. Some expectations of the test had to be changed because of that.
lgtm, nice https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/public/b... File content/public/browser/render_process_host.h (right): https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/public/b... content/public/browser/render_process_host.h:223: virtual void ResumeDeferredNavigation(const GlobalRequestID& request_id) = 0; On 2014/01/24 17:01:54, clamy wrote: > On 2014/01/23 23:34:58, nasko wrote: > > Why is this a public method now? I don't see anyone outside of content using > it > > in your patch. > > The method used to be in RenderProcessHostImpl, where it was public. It is > called in RenderFrameHostManager::SwappedOut where we static_cast a > RenderProcessHost into a RenderProcessHostImpl. In one of the unit_tests, we use > MockRenderProcessHost and happen, because of the patch, to hit this particular > cast when we previously did not (because the call to delegate_->SwappedOut(this) > in the RVH moved from OnSwappedOut to SwapOut). Since MockRenderProcessHost is > not derived from RenderProcessHostImpl, this was causing problems, and I had to > move this specific method to the parent class of RenderProcessHostImpl and > MockRenderProcessHost, that is here, in RenderProcessHost. This is an unfortunate side effect of having the public interface of RenderProcessHostImpl be the same base class for the mock version. I did this because I didn't want to create yet another base class for RPH when we added the content api. if it's just one test, and you can find a way to rewrite it as a browser test (where all implementations will be RenderProcessHostImpl), that would be great. if not, ok.
Thanks for the review! https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/public/b... File content/public/browser/render_process_host.h (right): https://chromiumcodereview.appspot.com/88503002/diff/1180001/content/public/b... content/public/browser/render_process_host.h:223: virtual void ResumeDeferredNavigation(const GlobalRequestID& request_id) = 0; On 2014/02/04 23:42:37, jam wrote: > On 2014/01/24 17:01:54, clamy wrote: > > On 2014/01/23 23:34:58, nasko wrote: > > > Why is this a public method now? I don't see anyone outside of content using > > it > > > in your patch. > > > > The method used to be in RenderProcessHostImpl, where it was public. It is > > called in RenderFrameHostManager::SwappedOut where we static_cast a > > RenderProcessHost into a RenderProcessHostImpl. In one of the unit_tests, we > use > > MockRenderProcessHost and happen, because of the patch, to hit this particular > > cast when we previously did not (because the call to > delegate_->SwappedOut(this) > > in the RVH moved from OnSwappedOut to SwapOut). Since MockRenderProcessHost is > > not derived from RenderProcessHostImpl, this was causing problems, and I had > to > > move this specific method to the parent class of RenderProcessHostImpl and > > MockRenderProcessHost, that is here, in RenderProcessHost. > > This is an unfortunate side effect of having the public interface of > RenderProcessHostImpl be the same base class for the mock version. I did this > because I didn't want to create yet another base class for RPH when we added the > content api. if it's just one test, and you can find a way to rewrite it as a > browser test (where all implementations will be RenderProcessHostImpl), that > would be great. if not, ok. I think I will leave it as it is now, as this is already quite a big change, and am not really sure about rewriting the unit test into a browser test. https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... File content/browser/frame_host/frame_tree.cc (right): https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/frame_host/frame_tree.cc:183: // in the map of RenderViewHost pending shutdown. Otherwise there should not On 2014/01/31 19:11:55, Charlie Reis wrote: > nit: RenderViewHosts Done. https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/frame_host/frame_tree.cc:184: // be a RenderViewHost for the site instance. On 2014/01/31 19:11:55, Charlie Reis wrote: > nit: SiteInstance Done. https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/frame_host/frame_tree.cc:253: if (multi_iter->second != render_frame_host->render_view_host()) On 2014/01/31 19:11:55, Charlie Reis wrote: > nit: Let's go ahead and put multi_iter->second into a rvh local variable, which > will make this code a bit easier to read. Done. https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... File content/browser/frame_host/frame_tree.h (right): https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/frame_host/frame_tree.h:145: // it allows us to call Shutdown on the RenderViewHost and remove it from the On 2014/01/31 19:11:55, Charlie Reis wrote: > This second part about "allows us to call Shutdown" is a little unclear now, > since the refcount has been moved to RVH itself. Can you rephrase it to > "Combined with the refcount on RenderViewHost, this also allows us to call > Shutdown on..."? Done. https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/frame_host/frame_tree.h:152: // Map of SiteInstance ID to RenderViewHosts that are pending shutdown. On 2014/01/31 19:11:55, Charlie Reis wrote: > Yes, I can see how you might need this. Can you add more explanation of why > it's needed in the comment, though? It will help others who haven't read your > overview document. Done. https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... File content/browser/frame_host/render_frame_host_impl.h (right): https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/frame_host/render_frame_host_impl.h:83: // when the swap out ack is received. On 2014/01/31 19:11:55, Charlie Reis wrote: > nit: SwapOutACK Done. https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:546: base::hash_map<int32, linked_ptr<RenderFrameHostImpl> >::iterator iter = On 2014/01/31 19:11:55, Charlie Reis wrote: > RFHPendingDeleteMap::iterator Done. https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:1116: // the RenderFrameHost should be put in the map of RenderFrameHost pending On 2014/01/31 19:11:55, Charlie Reis wrote: > map of RenderFrameHosts Done. https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:1120: RenderViewHostImpl::STATE_PENDING_SHUTDOWN) { On 2014/01/31 19:11:55, Charlie Reis wrote: > nit: 4 more spaces Done. https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... File content/browser/renderer_host/render_view_host_impl.h (right): https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:104: // Keeps track of the state of the RenderViewHostImpl, more particularly On 2014/01/31 19:11:55, Charlie Reis wrote: > nit: more particularly during -> particularly with respect to Done. https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:109: STATE_LIVE = 0, On 2014/01/31 19:11:55, Charlie Reis wrote: > This name is a bit confusing, since we'll be in this state for crashed > RenderViewHosts (in which case IsRenderViewLive() will return false). Maybe it > should be something like STATE_DEFAULT? Done. https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:120: // committed. The number of active views of the RVH SiteInstanceImpl is not On 2014/01/31 19:11:55, Charlie Reis wrote: > new page has committed in a different RVH. Done. https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:124: // committed. The number of active views of the RVH SiteInstanceImpl is On 2014/01/31 19:11:55, Charlie Reis wrote: > new page has committed in a different RVH. Done. https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:127: // The RVH is swapped out, it is being used as a placeholder to allow for On 2014/01/31 19:11:55, Charlie Reis wrote: > nit: swapped out, and it is Done. https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:363: // when the swap out ack is received, or when the unload timer times out. On 2014/01/31 19:11:55, Charlie Reis wrote: > nit: SwapOutACK Done. https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:657: // The number of RenderFrameHost which have a reference to this RVH. On 2014/01/31 19:11:55, Charlie Reis wrote: > nit: RenderFrameHosts Done. https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:717: bool is_waiting_for_beforeunload_ack_; On 2014/01/31 19:11:55, Charlie Reis wrote: > Should we consider moving this into the state machine in a future CL? (No need > to do that here, since this CL is already a large enough change. Might be worth > a TODO if so, though.) I think it would make the state of RenderViewHost easier to understand. I added a TODO. https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:756: // execute. On 2014/01/31 19:11:55, Charlie Reis wrote: > Can you mention whether this is used in the case that the RVH is going to be > swapped out rather than shut down? Done. https://chromiumcodereview.appspot.com/88503002/diff/1630001/content/browser/... content/browser/renderer_host/render_view_host_impl.h:759: // Called after receiving the swap out ack when the RVH is in state pending On 2014/01/31 19:11:55, Charlie Reis wrote: > nit: SwapOutACK Done.
The CQ bit was checked by clamy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clamy@chromium.org/88503002/1740001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by clamy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clamy@chromium.org/88503002/1740001
Message was sent while issue was closed.
Change committed as 249676 |
