Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2457503002:
Send input message acks for swapped out renderers. (Closed)
|
Created:
4 years, 1 month ago by dtapuska Modified:
4 years, 1 month ago CC:
chromium-reviews, nasko+codewatch_chromium.org, mlamouri+watch-content_chromium.org, jam, darin-cc_chromium.org, creis+watch_chromium.org, site-isolation-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSend input message acks for swapped out renderers.
If we are discarding input events make sure that
we send acks for them indicating we aren't handling them.
Otherwise the input event count the browser sees will
get through off.
BUG=615090
Committed: https://crrev.com/1b0fcc3202979d4eededd36e6d3326769690696f
Cr-Commit-Position: refs/heads/master@{#428425}
Patch Set 1 #Patch Set 2 : Add histogram #
Total comments: 3
Patch Set 3 : Move histogram into conditional #
Total comments: 7
Patch Set 4 : Fix nits #
Messages
Total messages: 29 (10 generated)
dtapuska@chromium.org changed reviewers: + creis@chromium.org
Charlie, this is what we talked about before. Let me know if you are ok to review it or want someone a little more familiar this to.
Description was changed from ========== Send input message acks for sawpped out renderers. If we are discarding input events make sure that we send acks for them indicating we aren't handling them. Otherwise the input event count the browser sees will get through off. BUG=615090 ========== to ========== Send input message acks for swapped out renderers. If we are discarding input events make sure that we send acks for them indicating we aren't handling them. Otherwise the input event count the browser sees will get through off. BUG=615090 ==========
creis@chromium.org changed reviewers: + kenrb@chromium.org
[+kenrb] Thanks for putting this together. Maybe Ken can help me understand how input events for OOPIFs work in a little more detail, to see what we should do here? I've confirmed that RenderViewImpl::is_swapped_out_ is true for the RenderView of an OOPIF. That would mean that all input events sent to the RenderView would be dropped on the floor, without getting ack'd, potentially causing the hang monitor to be displayed. However, we don't have that problem with OOPIFs in general-- most input events seem to be processed just fine, and don't appear to go through here. Ken, do those get routed straight to the RenderWidget? Do any input events go through RenderView anymore, and would they cause a hang for OOPIFs? I ask because I'm wondering if we can just remove the "drop input events if swapped out" check entirely. (Is it dead code? Is it still used by main frames?) Then again, I don't know who would process and ack the input event if we did remove the check, possibly leaving us with the same hang problem. Anyway, if we are sending any input events to swapped out RenderViews (and dropping them without ack'ing them), then sending the ack will avoid the hang but possibly leave OOPIFs broken. That's probably a good thing to do for now to see if it fixes https://crbug.com/615090, but it's not clear to me yet if it's the right solution long term.
I am not aware of any way that an InputMsg can get sent to a swapped out RenderView, but I can't strictly rule it out, so this might be worth trying. It might be interesting to test with a DCHECK in there. On 2016/10/26 19:17:46, Charlie Reis wrote: > I ask because I'm wondering if we can just remove the "drop input events if > swapped out" check entirely. (Is it dead code? Is it still used by main > frames?) Then again, I don't know who would process and ack the input event if > we did remove the check, possibly leaving us with the same hang problem. > Until RenderView and RenderWidget get split, InputMsg events will still need to go through the main frame's RenderView (which isn't swapped out).
On 2016/10/26 20:38:24, kenrb wrote: > I am not aware of any way that an InputMsg can get sent to a swapped out > RenderView, but I can't strictly rule it out, so this might be worth trying. It > might be interesting to test with a DCHECK in there. Thanks. I like the idea of including a NOTREACHED and even a UMA stat to see if any tests (or users) hit it, since it sounds like we don't expect it to happen. > On 2016/10/26 19:17:46, Charlie Reis wrote: > > I ask because I'm wondering if we can just remove the "drop input events if > > swapped out" check entirely. (Is it dead code? Is it still used by main > > frames?) Then again, I don't know who would process and ack the input event > if > > we did remove the check, possibly leaving us with the same hang problem. > > > > Until RenderView and RenderWidget get split, InputMsg events will still need to > go through the main frame's RenderView (which isn't swapped out). I see. Keep in mind that we can have swapped out RenderViews even when there's no OOPIFs (e.g., if the main frame is a RenderFrameProxy, as you'd see with a popup window that gets navigated cross-process from its opener). Maybe that explains what we're seeing? Suppose we had these repro steps: 1) Page on a.com opens window 2 to a.com (same process). 2) Navigate window 2 to b.com (cross process). 3) Somehow an input event for window 2 is sent to the a.com process's RenderView after it swaps out? I don't really see how that's possible, since the commit would happen in the b.com process, then the browser process would hear about it and set b.com's RenderFrameHost to be active (and presumably stop sending input event IPCs to the a.com process in window 2), and *then* we would tell the a.com process to swap out its RenderFrame in the background. But maybe there's a race that makes it possible if you leave and come back quickly? Unclear at first glance. Anyway, I think I'm ok proceeding with this to see if it makes a difference. What do you think about the UMA stat idea, to verify that it's actually happening in practice?
On 2016/10/26 21:30:20, Charlie Reis wrote: > On 2016/10/26 20:38:24, kenrb wrote: > > I am not aware of any way that an InputMsg can get sent to a swapped out > > RenderView, but I can't strictly rule it out, so this might be worth trying. > It > > might be interesting to test with a DCHECK in there. > > Thanks. I like the idea of including a NOTREACHED and even a UMA stat to see if > any tests (or users) hit it, since it sounds like we don't expect it to happen. > > > On 2016/10/26 19:17:46, Charlie Reis wrote: > > > I ask because I'm wondering if we can just remove the "drop input events if > > > swapped out" check entirely. (Is it dead code? Is it still used by main > > > frames?) Then again, I don't know who would process and ack the input event > > if > > > we did remove the check, possibly leaving us with the same hang problem. > > > > > > > Until RenderView and RenderWidget get split, InputMsg events will still need > to > > go through the main frame's RenderView (which isn't swapped out). > > I see. Keep in mind that we can have swapped out RenderViews even when there's > no OOPIFs (e.g., if the main frame is a RenderFrameProxy, as you'd see with a > popup window that gets navigated cross-process from its opener). Maybe that > explains what we're seeing? > > Suppose we had these repro steps: > 1) Page on a.com opens window 2 to a.com (same process). > 2) Navigate window 2 to b.com (cross process). > 3) Somehow an input event for window 2 is sent to the a.com process's RenderView > after it swaps out? > > I don't really see how that's possible, since the commit would happen in the > b.com process, then the browser process would hear about it and set b.com's > RenderFrameHost to be active (and presumably stop sending input event IPCs to > the a.com process in window 2), and *then* we would tell the a.com process to > swap out its RenderFrame in the background. > > But maybe there's a race that makes it possible if you leave and come back > quickly? Unclear at first glance. > > Anyway, I think I'm ok proceeding with this to see if it makes a difference. > What do you think about the UMA stat idea, to verify that it's actually > happening in practice? Can someone show me the code that prevents input events being sent to a swapped out renderer? I think we will find adding a DCHECK is likely to fail here. I'll add a UMA stat to this tomorrow.
dtapuska@chromium.org changed reviewers: + isherman@chromium.org
On 2016/10/27 01:03:30, dtapuska wrote: > On 2016/10/26 21:30:20, Charlie Reis wrote: > > On 2016/10/26 20:38:24, kenrb wrote: > > > I am not aware of any way that an InputMsg can get sent to a swapped out > > > RenderView, but I can't strictly rule it out, so this might be worth trying. > > It > > > might be interesting to test with a DCHECK in there. > > > > Thanks. I like the idea of including a NOTREACHED and even a UMA stat to see > if > > any tests (or users) hit it, since it sounds like we don't expect it to > happen. > > > > > On 2016/10/26 19:17:46, Charlie Reis wrote: > > > > I ask because I'm wondering if we can just remove the "drop input events > if > > > > swapped out" check entirely. (Is it dead code? Is it still used by main > > > > frames?) Then again, I don't know who would process and ack the input > event > > > if > > > > we did remove the check, possibly leaving us with the same hang problem. > > > > > > > > > > Until RenderView and RenderWidget get split, InputMsg events will still need > > to > > > go through the main frame's RenderView (which isn't swapped out). > > > > I see. Keep in mind that we can have swapped out RenderViews even when > there's > > no OOPIFs (e.g., if the main frame is a RenderFrameProxy, as you'd see with a > > popup window that gets navigated cross-process from its opener). Maybe that > > explains what we're seeing? > > > > Suppose we had these repro steps: > > 1) Page on a.com opens window 2 to a.com (same process). > > 2) Navigate window 2 to b.com (cross process). > > 3) Somehow an input event for window 2 is sent to the a.com process's > RenderView > > after it swaps out? > > > > I don't really see how that's possible, since the commit would happen in the > > b.com process, then the browser process would hear about it and set b.com's > > RenderFrameHost to be active (and presumably stop sending input event IPCs to > > the a.com process in window 2), and *then* we would tell the a.com process to > > swap out its RenderFrame in the background. > > > > But maybe there's a race that makes it possible if you leave and come back > > quickly? Unclear at first glance. > > > > Anyway, I think I'm ok proceeding with this to see if it makes a difference. > > What do you think about the UMA stat idea, to verify that it's actually > > happening in practice? > > Can someone show me the code that prevents input events being sent to a swapped > out renderer? I think we will find adding a DCHECK is likely to fail here. I'll > add a UMA stat to this tomorrow. I've added the histogram. Please take another look.
I'm ok skipping the DCHECK/NOTREACHED if we have the UMA stat-- we can investigate separately if needed. LGTM with one change to where the histogram is logged. On 2016/10/27 01:03:30, dtapuska wrote: > Can someone show me the code that prevents input events being sent to a swapped > out renderer? I think we will find adding a DCHECK is likely to fail here. I'll > add a UMA stat to this tomorrow. Ken, where's the browser-side code that routes input events to their widget/frame instead of the view? https://codereview.chromium.org/2457503002/diff/20001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2457503002/diff/20001/content/renderer/render... content/renderer/render_view_impl.cc:1308: UMA_HISTOGRAM_BOOLEAN("Event.RenderView.DiscardInput", discard_input); I would recommend only sending this if discard_input is true. As Ken mentioned, this gets hit for all input events for the main frame, which would be a lot of unnecessary UMA activity. (If you do need to keep it as a bool even in that case, I've heard your supposed to add a more specific bool enum in histograms.xml for each case.)
https://codereview.chromium.org/2457503002/diff/20001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2457503002/diff/20001/content/renderer/render... content/renderer/render_view_impl.cc:1308: UMA_HISTOGRAM_BOOLEAN("Event.RenderView.DiscardInput", discard_input); On 2016/10/27 16:57:00, Charlie Reis wrote: > I would recommend only sending this if discard_input is true. As Ken mentioned, > this gets hit for all input events for the main frame, which would be a lot of > unnecessary UMA activity. > > (If you do need to keep it as a bool even in that case, I've heard your supposed > to add a more specific bool enum in histograms.xml for each case.) I fear shipping the dcheck/notreached because I think this truly does exist as a scenario whereby a swapped out renderer occurs but it is still getting input events before it responds with the swap ack where events are then not routed. Especially since the InputMsg channel is different than the ViewMsg channel. Can we go with the UMA metric for now and see if it gets increased at all? If we get no signal at all then we can dcheck or check?
https://codereview.chromium.org/2457503002/diff/20001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2457503002/diff/20001/content/renderer/render... content/renderer/render_view_impl.cc:1308: UMA_HISTOGRAM_BOOLEAN("Event.RenderView.DiscardInput", discard_input); On 2016/10/27 18:55:36, dtapuska wrote: > On 2016/10/27 16:57:00, Charlie Reis wrote: > > I would recommend only sending this if discard_input is true. As Ken > mentioned, > > this gets hit for all input events for the main frame, which would be a lot of > > unnecessary UMA activity. > > > > (If you do need to keep it as a bool even in that case, I've heard your > supposed > > to add a more specific bool enum in histograms.xml for each case.) > > I fear shipping the dcheck/notreached because I think this truly does exist as a > scenario whereby a swapped out renderer occurs but it is still getting input > events before it responds with the swap ack where events are then not routed. > > Especially since the InputMsg channel is different than the ViewMsg channel. > > Can we go with the UMA metric for now and see if it gets increased at all? If we > get no signal at all then we can dcheck or check? I already said yes to that in my previous reply. :) On 2016/10/27 16:57:00, Charlie Reis wrote: > I'm ok skipping the DCHECK/NOTREACHED if we have the UMA stat-- we can > investigate separately if needed. LGTM with one change to where the histogram > is logged. FWIW, DCHECKs and NOTREACHEDs are debug only and won't crash release builds, but I agree with your reasoning. My only remaining comment was about moving the histogram inside the discard_input conditional.
On 2016/10/27 19:28:59, Charlie Reis wrote: > https://codereview.chromium.org/2457503002/diff/20001/content/renderer/render... > File content/renderer/render_view_impl.cc (right): > > https://codereview.chromium.org/2457503002/diff/20001/content/renderer/render... > content/renderer/render_view_impl.cc:1308: > UMA_HISTOGRAM_BOOLEAN("Event.RenderView.DiscardInput", discard_input); > On 2016/10/27 18:55:36, dtapuska wrote: > > On 2016/10/27 16:57:00, Charlie Reis wrote: > > > I would recommend only sending this if discard_input is true. As Ken > > mentioned, > > > this gets hit for all input events for the main frame, which would be a lot > of > > > unnecessary UMA activity. > > > > > > (If you do need to keep it as a bool even in that case, I've heard your > > supposed > > > to add a more specific bool enum in histograms.xml for each case.) > > > > I fear shipping the dcheck/notreached because I think this truly does exist as > a > > scenario whereby a swapped out renderer occurs but it is still getting input > > events before it responds with the swap ack where events are then not routed. > > > > Especially since the InputMsg channel is different than the ViewMsg channel. > > > > Can we go with the UMA metric for now and see if it gets increased at all? If > we > > get no signal at all then we can dcheck or check? > > I already said yes to that in my previous reply. :) > > On 2016/10/27 16:57:00, Charlie Reis wrote: > > I'm ok skipping the DCHECK/NOTREACHED if we have the UMA stat-- we can > > investigate separately if needed. LGTM with one change to where the histogram > > is logged. > > FWIW, DCHECKs and NOTREACHEDs are debug only and won't crash release builds, but > I agree with your reasoning. > > My only remaining comment was about moving the histogram inside the > discard_input conditional. done
LGTM. Ilya, can you review for histograms approval? https://codereview.chromium.org/2457503002/diff/40001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2457503002/diff/40001/content/renderer/render... content/renderer/render_view_impl.cc:1304: // produces results true. See crbug.com/615090 nit: Move comment next to histogram. https://codereview.chromium.org/2457503002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2457503002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:14352: +<histogram name="Event.RenderView.DiscardInput" enum="Boolean"> isherman@ can confirm, but I think the practice is to add a custom enum for each boolean histogram. (At least, that's what was requested of me for my last one.) :)
metrics lgtm % nits: https://codereview.chromium.org/2457503002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2457503002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:14352: +<histogram name="Event.RenderView.DiscardInput" enum="Boolean"> On 2016/10/27 19:35:17, Charlie Reis wrote: > isherman@ can confirm, but I think the practice is to add a custom enum for each > boolean histogram. (At least, that's what was requested of me for my last one.) > :) Yes, please =) https://codereview.chromium.org/2457503002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:14355: + Whether the input IPC messages were discarded before fully processed in nit: Should this say "before *being* fully processed"?
https://codereview.chromium.org/2457503002/diff/40001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2457503002/diff/40001/content/renderer/render... content/renderer/render_view_impl.cc:1304: // produces results true. See crbug.com/615090 On 2016/10/27 19:35:17, Charlie Reis wrote: > nit: Move comment next to histogram. Done. https://codereview.chromium.org/2457503002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2457503002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:14352: +<histogram name="Event.RenderView.DiscardInput" enum="Boolean"> On 2016/10/28 02:10:06, Ilya Sherman wrote: > On 2016/10/27 19:35:17, Charlie Reis wrote: > > isherman@ can confirm, but I think the practice is to add a custom enum for > each > > boolean histogram. (At least, that's what was requested of me for my last > one.) > > :) > > Yes, please =) Done. https://codereview.chromium.org/2457503002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:14355: + Whether the input IPC messages were discarded before fully processed in On 2016/10/28 02:10:06, Ilya Sherman wrote: > nit: Should this say "before *being* fully processed"? Done.
The CQ bit was checked by dtapuska@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2457503002/#ps60001 (title: "Fix nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by dtapuska@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Send input message acks for swapped out renderers. If we are discarding input events make sure that we send acks for them indicating we aren't handling them. Otherwise the input event count the browser sees will get through off. BUG=615090 ========== to ========== Send input message acks for swapped out renderers. If we are discarding input events make sure that we send acks for them indicating we aren't handling them. Otherwise the input event count the browser sees will get through off. BUG=615090 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Send input message acks for swapped out renderers. If we are discarding input events make sure that we send acks for them indicating we aren't handling them. Otherwise the input event count the browser sees will get through off. BUG=615090 ========== to ========== Send input message acks for swapped out renderers. If we are discarding input events make sure that we send acks for them indicating we aren't handling them. Otherwise the input event count the browser sees will get through off. BUG=615090 Committed: https://crrev.com/1b0fcc3202979d4eededd36e6d3326769690696f Cr-Commit-Position: refs/heads/master@{#428425} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/1b0fcc3202979d4eededd36e6d3326769690696f Cr-Commit-Position: refs/heads/master@{#428425} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
