Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1036243003:
PlzNavigate: Improvements to RFHM commit logic. (Closed)
|
Created:
5 years, 9 months ago by carlosk Modified:
5 years, 8 months ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, creis+watch_chromium.org, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: Improvements to RFHM commit logic.
Updates RFHM::CommitPendingIfNecessary to actually commit in a few more
cases and not crash when navigations happen simultaneously.
Also updates RFHM::CommitPending to make some logic clearer.
BUG=376094
Committed: https://crrev.com/a83e4009197f14936d4b53a9e5702ff145f3a8c9
Cr-Commit-Position: refs/heads/master@{#324234}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Updates from CR comments. #
Total comments: 4
Patch Set 3 : Minor changes. #
Total comments: 2
Patch Set 4 : Comment change. #
Total comments: 10
Patch Set 5 : Rebase #Patch Set 6 : Address CR comments. #
Total comments: 2
Patch Set 7 : Rebase. #
Messages
Total messages: 22 (8 generated)
carlosk@chromium.org changed reviewers: + clamy@chromium.org, fdegans@chromium.org
clamy: PTAL. fdegans: FYI. This started as a WebUI fix but I finally figured out that the WebUI side I was investigating was surprisingly being handled correctly (the WebUI was being reset). But the fix finally fixes one test and moves a couple others forward. This is yet another spin-off from crrev.com/967383002. https://codereview.chromium.org/1036243003/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1036243003/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:465: // requests properly. Due to the possibility of having multiple ongoing navigations and the asynchronous nature of message passing there still are situations where the code below won't do the right thing. https://codereview.chromium.org/1036243003/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:1652: } This code does basically the same as the previous one but looks way clearer. I was really confused until I found out that speculative_web_ui_.release() won't raise an exception if it points to NULL (and correctly caused the reset of |web_ui_|).
Thanks! Mostly good apart from a few comments. https://codereview.chromium.org/1036243003/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1036243003/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:464: // TODO(carlosk): this code might not handle interwoven navigation Could you add a comment explaining that even if there is no speculative RFH there may be a speculative webui to commit? https://codereview.chromium.org/1036243003/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:465: // requests properly. On 2015/03/27 17:11:29, carlosk wrote: > Due to the possibility of having multiple ongoing navigations and the > asynchronous nature of message passing there still are situations where the code > below won't do the right thing. Yes we should maybe think about pausing a navigation request when we are in the process of committing something just to make things clearer. https://codereview.chromium.org/1036243003/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:1648: } else if (should_reuse_web_ui_) { At which point is should_reuse_web_ui_ reset? https://codereview.chromium.org/1036243003/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:1649: CHECK(web_ui_); I wonder if this should be a DCHECK seeing that all other webui related checks are DCHECKs instead of CHECKs.
Thanks. https://chromiumcodereview.appspot.com/1036243003/diff/1/content/browser/fram... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/1036243003/diff/1/content/browser/fram... content/browser/frame_host/render_frame_host_manager.cc:464: // TODO(carlosk): this code might not handle interwoven navigation On 2015/03/31 11:40:42, clamy wrote: > Could you add a comment explaining that even if there is no speculative RFH > there may be a speculative webui to commit? Done. https://chromiumcodereview.appspot.com/1036243003/diff/1/content/browser/fram... content/browser/frame_host/render_frame_host_manager.cc:465: // requests properly. On 2015/03/31 11:40:42, clamy wrote: > On 2015/03/27 17:11:29, carlosk wrote: > > Due to the possibility of having multiple ongoing navigations and the > > asynchronous nature of message passing there still are situations where the > code > > below won't do the right thing. > > Yes we should maybe think about pausing a navigation request when we are in the > process of committing something just to make things clearer. Let's talk offline about this as I don't understand what you meant by "pausing a navigation". https://chromiumcodereview.appspot.com/1036243003/diff/1/content/browser/fram... content/browser/frame_host/render_frame_host_manager.cc:1648: } else if (should_reuse_web_ui_) { On 2015/03/31 11:40:42, clamy wrote: > At which point is should_reuse_web_ui_ reset? You are right to bring this up: this is the place. https://chromiumcodereview.appspot.com/1036243003/diff/1/content/browser/fram... content/browser/frame_host/render_frame_host_manager.cc:1649: CHECK(web_ui_); On 2015/03/31 11:40:42, clamy wrote: > I wonder if this should be a DCHECK seeing that all other webui related checks > are DCHECKs instead of CHECKs. Done.
Thanks! There is still an issue with the speculative WebUI which should be fixed. https://codereview.chromium.org/1036243003/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1036243003/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:467: // WebUI was not meant for the current one. The last sentence is not true. The speculative web ui could be for the speculative RenderFrameHost, which I guess is a problem in the patch as well. This should be fixed before landing the patch. https://codereview.chromium.org/1036243003/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1658: DCHECK(!speculative_web_ui_); Could you also add a DCHECK(!should_reuse_web_ui_)?
Thanks. I think that issue should be dealt with later. https://chromiumcodereview.appspot.com/1036243003/diff/20001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/1036243003/diff/20001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:467: // WebUI was not meant for the current one. On 2015/03/31 15:54:10, clamy wrote: > The last sentence is not true. The speculative web ui could be for the > speculative RenderFrameHost, which I guess is a problem in the patch as well. > This should be fixed before landing the patch. If both the speculative RFH *and* WebUI are set then it's certain the two belong together. That's what I meant in the comment and that's what the if-check below intends to detect. What might still happen is that if two navigations, both to the current RFH, requested closely to each other, one requiring a new WebUI and the other requiring none, then we might end up having a WebUI set when we shouldn't or the other way around. I added that example to the existing TODO. The same problem can happen with the current navigation implementation so this patch does not regress on that point. And the TODO is there so that we don't forget to deal with it. https://chromiumcodereview.appspot.com/1036243003/diff/20001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:1658: DCHECK(!speculative_web_ui_); On 2015/03/31 15:54:10, clamy wrote: > Could you also add a DCHECK(!should_reuse_web_ui_)? Done.
Thanks! I'm fine now that the comment has been updated, lgtm. https://codereview.chromium.org/1036243003/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1036243003/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:469: // requests properly. For example if two navigations, both to the current nit: "if two navigations to the current RenderFrameHost are requested closely to each other,..."
carlosk@chromium.org changed reviewers: + creis@chromium.org, nasko@chromium.org
carlosk@chromium.org changed required reviewers: + creis@chromium.org
Thanks. cresi@: PTAL. nasko@: FYI. https://chromiumcodereview.appspot.com/1036243003/diff/40001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/1036243003/diff/40001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:469: // requests properly. For example if two navigations, both to the current On 2015/04/01 15:53:40, clamy wrote: > nit: "if two navigations to the current RenderFrameHost are requested closely to > each other,..." Done.
Is this what the test from https://codereview.chromium.org/1039023004 was for? It probably makes sense to put that test in this CL, to couple it with the fix. Otherwise the test would start failing if we landed both and then someone had to revert this one. https://codereview.chromium.org/1036243003/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1036243003/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:463: DCHECK(!should_reuse_web_ui_ || web_ui_); DCHECK_IMPLIES(should_reuse_web_ui_, web_ui_); (This is a new macro I just learned about, which makes the intention a bit clearer.) https://codereview.chromium.org/1036243003/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:471: // new WebUI and the other requiring none, it might happen that a WebUI is Can there actually be two navigations in the same RenderFrameHost where one has a WebUI and one doesn't? It sounds like a security bug to me to lose WebUI but stay in the same RenderFrameHost and process. Maybe it's possible to have a different WebUI object in the same RFH but not none? In the reverse direction, RenderViewHostImpl::AllowBindings prevents us from granting WebUI bindings to a process that has already been used for non-WebUI pages. https://codereview.chromium.org/1036243003/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:472: // set when it shouldn't or the other way around. If this is a real issue, what's the plan for fixing this TODO? Setting a WebUI when there shouldn't be one could be a critical severity security bug (i.e., potential sandbox escape), so we can't turn on PlzNavigate while this is possible. (If this is real issue, let's mention a bug number here so we can track the work to fix it.) https://codereview.chromium.org/1036243003/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:474: CommitPending(); I'm noticing that this ends up echoing a lot of the code below for the non-PlzNavigate case. For example, we call CommitPending() when there's a pending_web_ui() and no pending_render_frame_host_ on line 494. Is there a reasonable way to reuse the code rather than having two separate implementations each addressing subtle cases like this? I worry about them diverging if we fix a bug in one but not the other. (Or is there a good reason to keep the paths separate?) https://codereview.chromium.org/1036243003/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1652: if (speculative_web_ui_) { Again, these new cases look a lot like lines 1642-1649, with different names and slightly different guards. Is there a way to share the logic so that we don't diverge, or does that lead to problems?
Patchset #5 (id:80001) has been deleted
Patchset #6 (id:120001) has been deleted
Patchset #6 (id:120001) has been deleted
On 2015/04/02 03:18:36, Charlie Reis wrote: > Is this what the test from https://codereview.chromium.org/1039023004 was for? > It probably makes sense to put that test in this CL, to couple it with the fix. > Otherwise the test would start failing if we landed both and then someone had to > revert this one. I merged the tests into this change. I was in doubt about doing that as they are not actually testing this change. but instead it is this change that allows RenderFrameHostManagerTest.WebUIWasReused to fail if the bug fixed by http://crbug.com/1012863004 is reintroduced. https://chromiumcodereview.appspot.com/1036243003/diff/60001/content/browser/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/1036243003/diff/60001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:463: DCHECK(!should_reuse_web_ui_ || web_ui_); On 2015/04/02 03:18:36, Charlie Reis wrote: > DCHECK_IMPLIES(should_reuse_web_ui_, web_ui_); > > (This is a new macro I just learned about, which makes the intention a bit > clearer.) Done. https://chromiumcodereview.appspot.com/1036243003/diff/60001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:471: // new WebUI and the other requiring none, it might happen that a WebUI is On 2015/04/02 03:18:36, Charlie Reis wrote: > Can there actually be two navigations in the same RenderFrameHost where one has > a WebUI and one doesn't? It sounds like a security bug to me to lose WebUI but > stay in the same RenderFrameHost and process. Maybe it's possible to have a > different WebUI object in the same RFH but not none? > > In the reverse direction, RenderViewHostImpl::AllowBindings prevents us from > granting WebUI bindings to a process that has already been used for non-WebUI > pages. I looked through the code and had the impression from WebUIControllerFactoryRegistry::IsURLAcceptableForWebUI that it would be possible (as it for instance allows about:blank pages to reuse a RFH with a WebUI). But I just tried and was unable to create a test with that behavior (a speculative RFH was always created). So I removed that comment and following your suggestion below I merged the PlzNavigate and regular navigation cases. https://chromiumcodereview.appspot.com/1036243003/diff/60001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:472: // set when it shouldn't or the other way around. On 2015/04/02 03:18:36, Charlie Reis wrote: > If this is a real issue, what's the plan for fixing this TODO? Setting a WebUI > when there shouldn't be one could be a critical severity security bug (i.e., > potential sandbox escape), so we can't turn on PlzNavigate while this is > possible. > > (If this is real issue, let's mention a bug number here so we can track the work > to fix it.) My previous comment covers this. https://chromiumcodereview.appspot.com/1036243003/diff/60001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:474: CommitPending(); On 2015/04/02 03:18:36, Charlie Reis wrote: > I'm noticing that this ends up echoing a lot of the code below for the > non-PlzNavigate case. For example, we call CommitPending() when there's a > pending_web_ui() and no pending_render_frame_host_ on line 494. > > Is there a reasonable way to reuse the code rather than having two separate > implementations each addressing subtle cases like this? I worry about them > diverging if we fix a bug in one but not the other. > > (Or is there a good reason to keep the paths separate?) Done. As we plan to eliminate |cross_navigation_pending_| I added a note on that for the PlzNavigate case. I used to keep them separate to minimize the code paths that would touch independent members from either navigation implementation. So that when we would have to eliminate the current implementation we'd basically delete the other side and remove the overhanging if-PlzNavigate. But I agree your concern on code maintenance is more relevant. https://chromiumcodereview.appspot.com/1036243003/diff/60001/content/browser/... content/browser/frame_host/render_frame_host_manager.cc:1652: if (speculative_web_ui_) { On 2015/04/02 03:18:36, Charlie Reis wrote: > Again, these new cases look a lot like lines 1642-1649, with different names and > slightly different guards. Is there a way to share the logic so that we don't > diverge, or does that lead to problems? Done. No problems just the need for some extra PlzNavigate-enabled checks.
Great, this looks much better. LGTM with one question below. On 2015/04/07 14:42:32, carlosk wrote: > On 2015/04/02 03:18:36, Charlie Reis wrote: > > Is this what the test from https://codereview.chromium.org/1039023004 was for? > > > It probably makes sense to put that test in this CL, to couple it with the > fix. > > Otherwise the test would start failing if we landed both and then someone had > to > > revert this one. > > I merged the tests into this change. I was in doubt about doing that as they are > not actually testing this change. but instead it is this change that allows > RenderFrameHostManagerTest.WebUIWasReused to fail if the bug fixed by > http://crbug.com/1012863004 is reintroduced. Ah, I see. Yeah, that's a bit more complicated than I thought, but it sounds fine to have these in the same CL. https://codereview.chromium.org/1036243003/diff/140001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1036243003/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:483: if (was_caused_by_user_gesture) { Should the PlzNavigate CleanUpNavigation call be inside the was_caused_by_user_gesture check? Suppose the current frame does an in-page navigation (e.g., pushState) without a user gesture, which commits without going through the network stack. We don't want that to cancel the user's ongoing navigation. Does PlzNavigate handle that differently?
Thanks. https://chromiumcodereview.appspot.com/1036243003/diff/140001/content/browser... File content/browser/frame_host/render_frame_host_manager.cc (right): https://chromiumcodereview.appspot.com/1036243003/diff/140001/content/browser... content/browser/frame_host/render_frame_host_manager.cc:483: if (was_caused_by_user_gesture) { On 2015/04/07 23:15:05, Charlie Reis wrote: > Should the PlzNavigate CleanUpNavigation call be inside the > was_caused_by_user_gesture check? > > Suppose the current frame does an in-page navigation (e.g., pushState) without a > user gesture, which commits without going through the network stack. We don't > want that to cancel the user's ongoing navigation. > > Does PlzNavigate handle that differently? It is handled differently by PlzNavigate. The decision of canceling or not on navigation for that specific case is taken in NavigatorImpl::OnBeginNavigation (see line 652), which is the entry points for renderer initiated navigation requests. This cleanup here is intended to properly handle cases of interwoven navigation requests: the current RFH committed even though there's an ongoing cross-site navigation (started after the RFH was already committing). So we cancel the latter.
The CQ bit was checked by carlosk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org, creis@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1036243003/#ps160001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1036243003/160001
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/a83e4009197f14936d4b53a9e5702ff145f3a8c9 Cr-Commit-Position: refs/heads/master@{#324234} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
