Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1426403006:
Reland #1 of: Move WebUI ownership from the RenderFrameHostManager to the RenderFrameHost. (Closed)
|
Created:
5 years, 1 month ago by carlosk Modified:
5 years, 1 month ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReland #1 of: Move WebUI ownership from the RenderFrameHostManager to the RenderFrameHost.
(Revert: https://codereview.chromium.org/1422043007/)
This change refactors the current ownership of the WebUI instance,
moving it from the RenderFrameHostManager to the RenderFrameHost which
is directly associated with the WebUI.
The design document for this change is here:
https://docs.google.com/a/chromium.org/document/d/1jrge5ulZ1YtqSR3WkNXzrtxhd3JXcWm8_zWBhEjaJtI
BUG=508850, 552253
Committed: https://crrev.com/113503ad7f6957c1eb2bb12215c8d5fae3303ef6
Cr-Commit-Position: refs/heads/master@{#360807}
Patch Set 1 #
Total comments: 22
Patch Set 2 : Rebase. #Patch Set 3 : Removed RFHM WebUI getters (merged from http://crrev.com/1418853003). #Patch Set 4 : RFHM unittests improvements and new tests (merged from http://crrev.com/1417953002) #Patch Set 5 : WIP: finding tests that restora a WebUI #Patch Set 6 : Added a new test for restore navigations of WebUI. #Patch Set 7 : WebUIs destruction step in ~WebContents (crbug.com/552253, crbug.com/552394). #Patch Set 8 : Minor changes and comment updates. #Patch Set 9 : Rebase. #Patch Set 10 : Re-add the pending WebUI to RFHI. #
Total comments: 52
Patch Set 11 : Address CR comments. #
Total comments: 4
Patch Set 12 : Address CR comments. #Messages
Total messages: 28 (6 generated)
carlosk@chromium.org changed reviewers: + clamy@chromium.org, creis@chromium.org, dbeam@chromium.org, nasko@chromium.org
This is a reland of the WebUI refactor. I had to revert it as it became the #1 source of canary crashes since it landed yesterday. The reason is the security-driven CHECKs of same-site/RFH navigations with WebUI transitions: going from no WebUI to having a WebUI, vice-versa, or switching WebUI types. Not all cases that should be white-listed were implemented. I think CHECK-ing is not a good solution here because: - It's hard to map the full set of allowed cases as the SiteInstance selection logic is complex and depends on a lot of variables. - We are using these current RFH WebUI transitions as a reason to check the correctness of GetSiteInstanceForNavigation, which doesn't seem like the right approach for this as it's at the least very incomplete (when there's not WebUI transition there's no checks). - It's a loosing race: if the SiteInstance selection logic is updated there is no direct link with the need to update this rule set; we'll only find out about them once it crashes in the wild. And the opposite is also true as we might allow transitions that were not supposed to happen. - The crashes alone don't provide enough information to allow us to white-list their causes. I'm still considering how to deal with this and I need your input on this matter. On the bright side creis@ will finally be able to also give his LGTM now... :)
Beyond the WebUI transition crashes [1] I described above there is also another crash that happened mostly on Macs and also needs investigation [2]. [1] https://code.google.com/p/chromium/issues/detail?id=552295 [2] https://code.google.com/p/chromium/issues/detail?id=552253
And just making it clear that this first patch set is simply a copy of the final one from the original change (I figure I didn't make that clear before).
On 2015/11/06 21:56:37, carlosk wrote: > And just making it clear that this first patch set is simply a copy of the final > one from the original change (I figure I didn't make that clear before). Thanks. I'm looking through this but I'm not done just yet. I'll get my comments sent as soon as I can.
Most of my comments here are about the notion of changing the WebUI of a RFH, which seems like a case we can eliminate. That should hopefully avoid the CheckWebUITransition crashes and lead to a simpler implementation. (Apologies if I missed some of this discussion in later rounds of the previous CL.) https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.h:489: // setting it to either none, a new instance or simply reuses the currently This sounds like it's stale, or it should be. A RFH's WebUI should not change once it has been assigned. If that were the case, this should be called CreateWebUI and it would verify that it doesn't get called more than once. https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:418: GetNavigatingWebUI()->RenderViewCreated( I don't understand the 5 separate RenderViewCreated calls spread through this class. This used to be called in a single place (WebContentsImpl::RenderViewCreated), which was easy to reason about. Now it's not clear why they are where they are (which makes it hard to know if we've covered all the cases, or when future cases might matter). This one in particular confuses me, because the CreateRenderViewForRenderManager call on line 394 looks like it used to trigger a RenderViewCreated call but now it doesn't. Why does this post-InitRenderView case need it and the earlier case does not? Is there a simpler approach we can use? Is there a reason it can't stay in WebContentsImpl::RenderViewCreated? https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:1022: // Reuse the current RenderFrameHost if its SiteInstance matches the the nit: "the the" https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:1043: // SiteInstace differs from the one for the current URL, a new one needs nit: SiteInstance https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:2375: void RenderFrameHostManager::UpdateWebUIOnCurrentFrameHost(const GURL& dest_url, Nasko's https://codereview.chromium.org/1413863006/ was meant to eliminate this complexity, because a RenderFrameHost can't change its WebUI object anymore. See below for details. It seems like we should only need to take care when adding a WebUI object to an RFH in the first place. https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:2618: // There are a few navigation cases that allow for changes to the WebUI of I agree that this is not a sustainable approach, and I'm not sure what value it's adding. Wouldn't it be sufficient to say that an RFH can't change its WebUI object once it's added? (See below about renderer debug URLs, which only commit a page in rare cases like chrome://crashdump or chrome://shorthang.) https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:2643: // - Navigating back from a special renderer URL. This seems like an unnecessary case. Why not just leave the WebUI object in place for renderer debug URLs? Then the WebUI object of an RFH never changes once it's assigned, and we only have to be careful when first granting a WebUI object. I think the only renderer debug URLs that can commit in a WebUI are chrome://crashdump and chrome://shorthang (context: https://crbug.com/477606#c16), and those don't seem to pose a risk that would require dropping WebUI. Even if javascript: URLs were allowed on WebUI, they can run on the current page so dropping the WebUI if they commit doesn't seem helpful.
Only replying to the comments (no new patch set yet) as I'm still unsure about how to proceed. I'm working on some options but would like to have a VC with you and/or Nasko as I have doubts on the actual requirements and the right approach. On 2015/11/09 07:01:00, Charlie Reis (slow til 11-16) wrote: > Most of my comments here are about the notion of changing the WebUI of a RFH, > which seems like a case we can eliminate. That should hopefully avoid the > CheckWebUITransition crashes and lead to a simpler implementation. I also hope we can make that simplification actually happen! https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.h:489: // setting it to either none, a new instance or simply reuses the currently On 2015/11/09 07:01:00, Charlie Reis (slow til 11-16) wrote: > This sounds like it's stale, or it should be. A RFH's WebUI should not change > once it has been assigned. If that were the case, this should be called > CreateWebUI and it would verify that it doesn't get called more than once. Changing this method so that it works like that is not a problem. The question then is how handle the cases when that's not what should happen? Should we create a new UMA enumeration to report those cases so that we can track them? For the actual behavior you are suggesting I am in doubt between: 1) This method can only be ever called once and will simply decide to either create a WebUI or not. Future calls would CHECK/DCHECK. 2) This method could be called indefinitely until a WebUI is created. From that point on a call would CHECK/DCHECK. 3) Same as any of the above but instead of CHECK/DCHECK it would simply ignore future calls. Furthermore, if we do limit this method as you suggest will that be adequate for the non-default process models we support? If indeed they should behave differently where should the model-dependent logic exist: here or in RFHM? If the latter is preferable then maybe this method should stay as is. https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:418: GetNavigatingWebUI()->RenderViewCreated( On 2015/11/09 07:01:00, Charlie Reis (slow til 11-16) wrote: > I don't understand the 5 separate RenderViewCreated calls spread through this > class. This used to be called in a single place > (WebContentsImpl::RenderViewCreated), which was easy to reason about. Now it's > not clear why they are where they are (which makes it hard to know if we've > covered all the cases, or when future cases might matter). > > This one in particular confuses me, because the CreateRenderViewForRenderManager > call on line 394 looks like it used to trigger a RenderViewCreated call but now > it doesn't. Why does this post-InitRenderView case need it and the earlier case > does not? > > Is there a simpler approach we can use? Is there a reason it can't stay in > WebContentsImpl::RenderViewCreated? There are a few reasons for that, some discusses in previous comment rounds in the original change: - Creation order was changed and now the WebUI is created after the RFH is. Before WebContentsImpl::RenderViewCreated could call RFHM::pending_web_ui() and it would return a valid value but now it won't (when creating a fully new RFH/WebUI). - It was decided the RFHI constructor should not be changed to immediately initialize the WebUI. And also that we should not change all RFH-creation-related methods to pump down the needed arguments for that. That's the reason why this doesn't happen inside RFHM::InitRenderView either. All WebUI update happens at a more "shallow" method level in RFHM. - Before there was at least one missing call to RenderView(Created|Reused). There was one more reason during the development of the refactor that might now be deprecated: in some cases only RFHM had the information needed to decide which of RenderView(Created|Reused) should be called. As right now there's only one case of RenderViewReused. The reason for this is that *during this development* we arrived at the conclusion that RenderViewCreated should be called whenever either element in the WebUI/RVH pair is changed And for the future: - There's 2 "extra" calls right now because there's duplication between PlzNavigate and the current navigation implementation (two will go away - There's some discussion around eliminating RenderViewReused so that would simplify it even further. One point though that I think is better now is that all of these notification calls happen from one single class, RFHM, what makes them easier to track and understand. FYI, some related discussions I could dig from the previous change: [1] https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... [2] https://codereview.chromium.org/1352813006/diff/420001/content/browser/frame_... [3] https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... [4] https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_... https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:1022: // Reuse the current RenderFrameHost if its SiteInstance matches the the On 2015/11/09 07:01:00, Charlie Reis (slow til 11-16) wrote: > nit: "the the" Done. https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:1043: // SiteInstace differs from the one for the current URL, a new one needs On 2015/11/09 07:01:00, Charlie Reis (slow til 11-16) wrote: > nit: SiteInstance Done. https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:2375: void RenderFrameHostManager::UpdateWebUIOnCurrentFrameHost(const GURL& dest_url, On 2015/11/09 07:01:00, Charlie Reis (slow til 11-16) wrote: > Nasko's https://codereview.chromium.org/1413863006/ was meant to eliminate this > complexity, because a RenderFrameHost can't change its WebUI object anymore. > See below for details. > > It seems like we should only need to take care when adding a WebUI object to an > RFH in the first place. I'd love to get to that point! But right now that's not true yet. https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:2618: // There are a few navigation cases that allow for changes to the WebUI of On 2015/11/09 07:01:00, Charlie Reis (slow til 11-16) wrote: > I agree that this is not a sustainable approach, and I'm not sure what value > it's adding. Wouldn't it be sufficient to say that an RFH can't change its > WebUI object once it's added? (See below about renderer debug URLs, which only > commit a page in rare cases like chrome://crashdump or chrome://shorthang.) It doesn't seem like just that will be sufficient. Crash reports from this method [1] show that all 3 different CHECKs got triggered. So there are still unknown cases (not mapped by tests) where there was no change of SiteInstance (same-site navigation) but there was a WebUI transition: no WebUI to having a WebUI, vice-versa, or a change of WebUI type. To fix that it seems we'd have to further change SiteInstance selection logic and/or decide what should the WebUI selection logic do when a situation that is not allowed happens. [1] https://crash.corp.google.com/browse?stbtiq=content%3A%3ARenderFrameHostManag... https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:2643: // - Navigating back from a special renderer URL. On 2015/11/09 07:01:00, Charlie Reis (slow til 11-16) wrote: > This seems like an unnecessary case. Why not just leave the WebUI object in > place for renderer debug URLs? Then the WebUI object of an RFH never changes > once it's assigned, and we only have to be careful when first granting a WebUI > object. SGTM. But as for my previous questions should the logic to decide about that be kept here in RFHM or in RFHI? (I guess that also depends on the answers from my previous questions) > I think the only renderer debug URLs that can commit in a WebUI are > chrome://crashdump and chrome://shorthang (context: > https://crbug.com/477606#c16), and those don't seem to pose a risk that would > require dropping WebUI. Even if javascript: URLs were allowed on WebUI, they > can run on the current page so dropping the WebUI if they commit doesn't seem > helpful. Acknowledged.
Thanks. A few thoughts on your questions below. https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.h:489: // setting it to either none, a new instance or simply reuses the currently On 2015/11/10 15:02:30, carlosk wrote: > On 2015/11/09 07:01:00, Charlie Reis (slow til 11-16) wrote: > > This sounds like it's stale, or it should be. A RFH's WebUI should not change > > once it has been assigned. If that were the case, this should be called > > CreateWebUI and it would verify that it doesn't get called more than once. > > Changing this method so that it works like that is not a problem. The question > then is how handle the cases when that's not what should happen? Should we > create a new UMA enumeration to report those cases so that we can track them? > > For the actual behavior you are suggesting I am in doubt between: > 1) This method can only be ever called once and will simply decide to either > create a WebUI or not. Future calls would CHECK/DCHECK. > 2) This method could be called indefinitely until a WebUI is created. From that > point on a call would CHECK/DCHECK. > 3) Same as any of the above but instead of CHECK/DCHECK it would simply ignore > future calls. From the perspective of security, we shouldn't be loading non-WebUI URLs in a RFH and then later upgrade it to WebUI. Similarly, we shouldn't be loading non-WebUI-acceptable URLs in an RFH after it's been upgraded to WebUI. This means the first call seems like it should be the one that determines whether this RFH has a WebUI or not, and it shouldn't be called again (your option 1). > Furthermore, if we do limit this method as you suggest will that be adequate for > the non-default process models we support? If indeed they should behave > differently where should the model-dependent logic exist: here or in RFHM? If > the latter is preferable then maybe this method should stay as is. The other process models should be subject to the same rules. They all swap RFHs on WebUI-changing navigations, after Nasko's recent CL. https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:418: GetNavigatingWebUI()->RenderViewCreated( On 2015/11/10 15:02:30, carlosk wrote: > On 2015/11/09 07:01:00, Charlie Reis (slow til 11-16) wrote: > > I don't understand the 5 separate RenderViewCreated calls spread through this > > class. This used to be called in a single place > > (WebContentsImpl::RenderViewCreated), which was easy to reason about. Now > it's > > not clear why they are where they are (which makes it hard to know if we've > > covered all the cases, or when future cases might matter). > > > > This one in particular confuses me, because the > CreateRenderViewForRenderManager > > call on line 394 looks like it used to trigger a RenderViewCreated call but > now > > it doesn't. Why does this post-InitRenderView case need it and the earlier > case > > does not? > > > > Is there a simpler approach we can use? Is there a reason it can't stay in > > WebContentsImpl::RenderViewCreated? > > There are a few reasons for that, some discusses in previous comment rounds in > the original change: > - Creation order was changed and now the WebUI is created after the RFH is. > Before WebContentsImpl::RenderViewCreated could call RFHM::pending_web_ui() and > it would return a valid value but now it won't (when creating a fully new > RFH/WebUI). > - It was decided the RFHI constructor should not be changed to immediately > initialize the WebUI. And also that we should not change all > RFH-creation-related methods to pump down the needed arguments for that. That's > the reason why this doesn't happen inside RFHM::InitRenderView either. All WebUI > update happens at a more "shallow" method level in RFHM. > - Before there was at least one missing call to RenderView(Created|Reused). > > There was one more reason during the development of the refactor that might now > be deprecated: in some cases only RFHM had the information needed to decide > which of RenderView(Created|Reused) should be called. As right now there's only > one case of RenderViewReused. The reason for this is that *during this > development* we arrived at the conclusion that RenderViewCreated should be > called whenever either element in the WebUI/RVH pair is changed > > And for the future: > - There's 2 "extra" calls right now because there's duplication between > PlzNavigate and the current navigation implementation (two will go away > - There's some discussion around eliminating RenderViewReused so that would > simplify it even further. > > One point though that I think is better now is that all of these notification > calls happen from one single class, RFHM, what makes them easier to track and > understand. > > FYI, some related discussions I could dig from the previous change: > [1] > https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... > [2] > https://codereview.chromium.org/1352813006/diff/420001/content/browser/frame_... > [3] > https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... > [4] > https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_... Thanks for the context. It sounds like there's a lot of constraints on it that we may be stuck with for now. Would it be possible to add brief comments to the call sites about why it's necessary there, and update the declaration comment in web_ui_impl.h? That would make it easier to reason about whether the calls are correct and when they need changes or additions. https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:2618: // There are a few navigation cases that allow for changes to the WebUI of On 2015/11/10 15:02:30, carlosk wrote: > On 2015/11/09 07:01:00, Charlie Reis (slow til 11-16) wrote: > > I agree that this is not a sustainable approach, and I'm not sure what value > > it's adding. Wouldn't it be sufficient to say that an RFH can't change its > > WebUI object once it's added? (See below about renderer debug URLs, which > only > > commit a page in rare cases like chrome://crashdump or chrome://shorthang.) > > It doesn't seem like just that will be sufficient. Crash reports from this > method [1] show that all 3 different CHECKs got triggered. So there are still > unknown cases (not mapped by tests) where there was no change of SiteInstance > (same-site navigation) but there was a WebUI transition: no WebUI to having a > WebUI, vice-versa, or a change of WebUI type. > > To fix that it seems we'd have to further change SiteInstance selection logic > and/or decide what should the WebUI selection logic do when a situation that is > not allowed happens. > > [1] > https://crash.corp.google.com/browse?stbtiq=content%3A%3ARenderFrameHostManag... Interesting. This seems like something we should get to the bottom of, before re-landing this CL. Can we add some equivalent checks to trunk (without the rest of the changes in this CL) along with some crash keys to see what's causing us to fail these invariants? There's an example of this style of crash debugging here: https://codereview.chromium.org/1410543014/ I think this will be a worthwhile investigation (though I'm sure it feels like a distraction), since the code would really benefit if a RenderFrameHost can't change its WebUI object. https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:2643: // - Navigating back from a special renderer URL. On 2015/11/10 15:02:30, carlosk wrote: > On 2015/11/09 07:01:00, Charlie Reis (slow til 11-16) wrote: > > This seems like an unnecessary case. Why not just leave the WebUI object in > > place for renderer debug URLs? Then the WebUI object of an RFH never changes > > once it's assigned, and we only have to be careful when first granting a WebUI > > object. > > SGTM. But as for my previous questions should the logic to decide about that be > kept here in RFHM or in RFHI? (I guess that also depends on the answers from my > previous questions) Probably RFHI, but that's just guessing at this point. > > > I think the only renderer debug URLs that can commit in a WebUI are > > chrome://crashdump and chrome://shorthang (context: > > https://crbug.com/477606#c16), and those don't seem to pose a risk that would > > require dropping WebUI. Even if javascript: URLs were allowed on WebUI, they > > can run on the current page so dropping the WebUI if they commit doesn't seem > > helpful. > > Acknowledged.
Patchset #4 (id:60001) has been deleted
Considering we are hitting a lot of unknowns with this patch, and that it has been going on for a long time, can we consider landing a first version that still has the pending WebUI, but with the ownership of the WebUI in the RFH? Then, once enough data has been collected about the problematic cases we can remove the concept of the pending WebUI in a latter patch. This would unblock us in trying to set up a TBM benchmark for PlzNavigate.
Thanks. The investigation of the unknown cases of same-site navigations that are transitioning the WebUI will require more time investment and as clamy@ mentioned this is blocking our primary work in PlzNavigate. The initial design didn't include the elimination of the pending WebUI and we do not fully understand yet the consequences of this change. My suggestion is also to reintroduce the concept of the pending WebUI at the RFHI level to solve this problem for now. This seems to be the safest and more productive approach. https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.h:489: // setting it to either none, a new instance or simply reuses the currently On 2015/11/11 01:11:00, Charlie Reis (OOO till 11-16) wrote: > On 2015/11/10 15:02:30, carlosk wrote: > > On 2015/11/09 07:01:00, Charlie Reis (slow til 11-16) wrote: > > > This sounds like it's stale, or it should be. A RFH's WebUI should not > change > > > once it has been assigned. If that were the case, this should be called > > > CreateWebUI and it would verify that it doesn't get called more than once. > > > > Changing this method so that it works like that is not a problem. The question > > then is how handle the cases when that's not what should happen? Should we > > create a new UMA enumeration to report those cases so that we can track them? > > > > For the actual behavior you are suggesting I am in doubt between: > > 1) This method can only be ever called once and will simply decide to either > > create a WebUI or not. Future calls would CHECK/DCHECK. > > 2) This method could be called indefinitely until a WebUI is created. From > that > > point on a call would CHECK/DCHECK. > > 3) Same as any of the above but instead of CHECK/DCHECK it would simply ignore > > future calls. > > From the perspective of security, we shouldn't be loading non-WebUI URLs in a > RFH and then later upgrade it to WebUI. Similarly, we shouldn't be loading > non-WebUI-acceptable URLs in an RFH after it's been upgraded to WebUI. This > means the first call seems like it should be the one that determines whether > this RFH has a WebUI or not, and it shouldn't be called again (your option 1). > > > Furthermore, if we do limit this method as you suggest will that be adequate > for > > the non-default process models we support? If indeed they should behave > > differently where should the model-dependent logic exist: here or in RFHM? If > > the latter is preferable then maybe this method should stay as is. > > The other process models should be subject to the same rules. They all swap > RFHs on WebUI-changing navigations, after Nasko's recent CL. Acknowledged x2. https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:418: GetNavigatingWebUI()->RenderViewCreated( On 2015/11/11 01:11:00, Charlie Reis (OOO till 11-16) wrote: > On 2015/11/10 15:02:30, carlosk wrote: > > On 2015/11/09 07:01:00, Charlie Reis (slow til 11-16) wrote: > > > I don't understand the 5 separate RenderViewCreated calls spread through > this > > > class. This used to be called in a single place > > > (WebContentsImpl::RenderViewCreated), which was easy to reason about. Now > > it's > > > not clear why they are where they are (which makes it hard to know if we've > > > covered all the cases, or when future cases might matter). > > > > > > This one in particular confuses me, because the > > CreateRenderViewForRenderManager > > > call on line 394 looks like it used to trigger a RenderViewCreated call but > > now > > > it doesn't. Why does this post-InitRenderView case need it and the earlier > > case > > > does not? > > > > > > Is there a simpler approach we can use? Is there a reason it can't stay in > > > WebContentsImpl::RenderViewCreated? > > > > There are a few reasons for that, some discusses in previous comment rounds in > > the original change: > > - Creation order was changed and now the WebUI is created after the RFH is. > > Before WebContentsImpl::RenderViewCreated could call RFHM::pending_web_ui() > and > > it would return a valid value but now it won't (when creating a fully new > > RFH/WebUI). > > - It was decided the RFHI constructor should not be changed to immediately > > initialize the WebUI. And also that we should not change all > > RFH-creation-related methods to pump down the needed arguments for that. > That's > > the reason why this doesn't happen inside RFHM::InitRenderView either. All > WebUI > > update happens at a more "shallow" method level in RFHM. > > - Before there was at least one missing call to RenderView(Created|Reused). > > > > There was one more reason during the development of the refactor that might > now > > be deprecated: in some cases only RFHM had the information needed to decide > > which of RenderView(Created|Reused) should be called. As right now there's > only > > one case of RenderViewReused. The reason for this is that *during this > > development* we arrived at the conclusion that RenderViewCreated should be > > called whenever either element in the WebUI/RVH pair is changed > > > > And for the future: > > - There's 2 "extra" calls right now because there's duplication between > > PlzNavigate and the current navigation implementation (two will go away > > - There's some discussion around eliminating RenderViewReused so that would > > simplify it even further. > > > > One point though that I think is better now is that all of these notification > > calls happen from one single class, RFHM, what makes them easier to track and > > understand. > > > > FYI, some related discussions I could dig from the previous change: > > [1] > > > https://codereview.chromium.org/1352813006/diff/340001/content/browser/frame_... > > [2] > > > https://codereview.chromium.org/1352813006/diff/420001/content/browser/frame_... > > [3] > > > https://codereview.chromium.org/1352813006/diff/620001/content/browser/frame_... > > [4] > > > https://codereview.chromium.org/1352813006/diff/800001/content/browser/frame_... > > Thanks for the context. It sounds like there's a lot of constraints on it that > we may be stuck with for now. > > Would it be possible to add brief comments to the call sites about why it's > necessary there, and update the declaration comment in web_ui_impl.h? That > would make it easier to reason about whether the calls are correct and when they > need changes or additions. Done. https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:2618: // There are a few navigation cases that allow for changes to the WebUI of On 2015/11/11 01:11:00, Charlie Reis (OOO till 11-16) wrote: > On 2015/11/10 15:02:30, carlosk wrote: > > On 2015/11/09 07:01:00, Charlie Reis (slow til 11-16) wrote: > > > I agree that this is not a sustainable approach, and I'm not sure what value > > > it's adding. Wouldn't it be sufficient to say that an RFH can't change its > > > WebUI object once it's added? (See below about renderer debug URLs, which > > only > > > commit a page in rare cases like chrome://crashdump or chrome://shorthang.) > > > > It doesn't seem like just that will be sufficient. Crash reports from this > > method [1] show that all 3 different CHECKs got triggered. So there are still > > unknown cases (not mapped by tests) where there was no change of SiteInstance > > (same-site navigation) but there was a WebUI transition: no WebUI to having a > > WebUI, vice-versa, or a change of WebUI type. > > > > To fix that it seems we'd have to further change SiteInstance selection logic > > and/or decide what should the WebUI selection logic do when a situation that > is > > not allowed happens. > > > > [1] > > > https://crash.corp.google.com/browse?stbtiq=content%3A%3ARenderFrameHostManag... > > Interesting. This seems like something we should get to the bottom of, before > re-landing this CL. > > Can we add some equivalent checks to trunk (without the rest of the changes in > this CL) along with some crash keys to see what's causing us to fail these > invariants? There's an example of this style of crash debugging here: > https://codereview.chromium.org/1410543014/ > > I think this will be a worthwhile investigation (though I'm sure it feels like a > distraction), since the code would really benefit if a RenderFrameHost can't > change its WebUI object. This depends on our decision regarding the pending WebUI. https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:2643: // - Navigating back from a special renderer URL. On 2015/11/11 01:11:00, Charlie Reis (OOO till 11-16) wrote: > On 2015/11/10 15:02:30, carlosk wrote: > > On 2015/11/09 07:01:00, Charlie Reis (slow til 11-16) wrote: > > > This seems like an unnecessary case. Why not just leave the WebUI object in > > > place for renderer debug URLs? Then the WebUI object of an RFH never > changes > > > once it's assigned, and we only have to be careful when first granting a > WebUI > > > object. > > > > SGTM. But as for my previous questions should the logic to decide about that > be > > kept here in RFHM or in RFHI? (I guess that also depends on the answers from > my > > previous questions) > > Probably RFHI, but that's just guessing at this point. > > > > > > I think the only renderer debug URLs that can commit in a WebUI are > > > chrome://crashdump and chrome://shorthang (context: > > > https://crbug.com/477606#c16), and those don't seem to pose a risk that > would > > > require dropping WebUI. Even if javascript: URLs were allowed on WebUI, > they > > > can run on the current page so dropping the WebUI if they commit doesn't > seem > > > helpful. > > > > Acknowledged. > This depends on our decision regarding the pending WebUI.
Patchset #10 (id:200001) has been deleted
Description was changed from ========== Reland #1 of: Move WebUI ownership from the RenderFrameHostManager to the RenderFrameHost. (Revert: https://codereview.chromium.org/1422043007/) This change refactors the current ownership of the WebUI instance, moving it from the RenderFrameHostManager to the RenderFrameHost which is directly associated with the WebUI. The design document for this change is here: https://docs.google.com/a/chromium.org/document/d/1jrge5ulZ1YtqSR3WkNXzrtxhd3... BUG=508850 ========== to ========== Reland #1 of: Move WebUI ownership from the RenderFrameHostManager to the RenderFrameHost. (Revert: https://codereview.chromium.org/1422043007/) This change refactors the current ownership of the WebUI instance, moving it from the RenderFrameHostManager to the RenderFrameHost which is directly associated with the WebUI. The design document for this change is here: https://docs.google.com/a/chromium.org/document/d/1jrge5ulZ1YtqSR3WkNXzrtxhd3... BUG=508850, 552253 ==========
Pending WebUI is back and it should solve the same-site navigation problems we've been discussing. I had to reintroduce RFHM::GetNavigatingWebUI. nasko@ previously raised concerns with its naming and functionality but given this makes pending_web_ui and speculative_web_ui go away (it consolidates them) I think this change is for the better. See its doc comment in the header file. Some more work done in previously uploaded patch sets: - Added the special handling of destroying WebUIs as an initial step of destroying the WebContents that will hopefully stop the crashes caused by the WebUI trying to access a semi-destroyed frame tree. - Merged two patches I was working in parallel: the removal of the RFHM's WebUI getters (even though GetNavigatingWebUI was brought back) and the PlzNavigate specific fixes and tests. - Created a WebUI restore test (that right now might not be so important but it will be once we get back into eliminating the pending WebUI. I also filed a new issue to track the future work on removing the pending WebUI: https://code.google.com/p/chromium/issues/detail?id=557116
I'm sad the pending WebUI case proved to be a problem, but I'm ok with doing the move first. Thanks for the extra comments around RenderViewCreated, etc. I'll give my LGTM (assuming the linux_site_isolation try job I started passes), and please wait for Nasko's as well.
https://codereview.chromium.org/1426403006/diff/220001/content/browser/webui/... File content/browser/webui/web_ui_impl.h (right): https://codereview.chromium.org/1426403006/diff/220001/content/browser/webui/... content/browser/webui/web_ui_impl.h:30: // RenderView, vice-versa or if both have just been created. revert to old comment or explain to me why this is better https://codereview.chromium.org/1426403006/diff/220001/content/browser/webui/... content/browser/webui/web_ui_impl.h:34: // pair of instances. same
Thanks. https://codereview.chromium.org/1426403006/diff/220001/content/browser/webui/... File content/browser/webui/web_ui_impl.h (right): https://codereview.chromium.org/1426403006/diff/220001/content/browser/webui/... content/browser/webui/web_ui_impl.h:30: // RenderView, vice-versa or if both have just been created. On 2015/11/18 18:10:58, Dan Beam wrote: > revert to old comment or explain to me why this is better This change addresses a recent request by creis@ [1]. During the development of this refactor it seemed to me that RenderViewCreated should not only be called when a new RenderView is created but also when it remains the same and a new WebUI is created under it. This happens in a same-site navigation where the RenderFrameHost/RenderViewHost is kepts but the WebUI is changed to another type. Neither this method's name nor its comment expressed well that logic as they refer only to creations of the RenderView. ... (continues below) https://codereview.chromium.org/1426403006/diff/220001/content/browser/webui/... content/browser/webui/web_ui_impl.h:34: // pair of instances. On 2015/11/18 18:10:58, Dan Beam wrote: > same ... (continued from above) Following the same logic, RenderViewReused should work as the complement of the previous one: it is called in a navigation where none of them change. And again its name and comment are not clear and I would assume that if only the WebUI would have changed I should still call this method. In other words: if the navigating WebUI has never interacted with the RenderView before the former method should be called. And if it has already seen it then we should call the latter. [1] https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/...
Another round of comments. Count is high, but the CL is in very good shape. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_delegate.h:12: #include "content/browser/webui/web_ui_impl.h" Can we forward declare WebUIImpl here instead of including it? https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_delegate.h:176: // Creates a WebUI object for a frame navigating to the given URL. If no WebUI nit: s/to the given URL/to |url|/ https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2174: ClearPendingWebUI(); nit: I'd clear the pending first, in case some destruction related callbacks end up calling into RFH for some reason. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:508: void ClearAllWebUIs(); nit: drop the "s" at the end of the method name. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:864: // same-site navigation to a document requiring a new WebUI. Why do we need a pending WebUI for a same-site navigation? It makes sense to have it for none->WebUI needed one. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:688: // Note: if one tries to move same-site commit logic into RenderFrameHost nit: s/if/when/. Also start with capital letter as it is a sentence. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1043: // RenderFrameHost. What is the scenario when this happens? https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:183: // RenderFrameHosts. Used during destruction of WebContentsImpl. nit: Drop the second sentence. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:99: if (should_create_webui_ && HasWebUIScheme(url)) Uh, this is weird, but probably ok at the content/ layer. There are more schemes that get WebUI. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:109: return reinterpret_cast<WebUI::TypeID>(type_); Can we avoid adding this? Can't we just come up with some unique value based on the URL passed in? It will feel a bit more like the real implementation and it will avoid the complexity of calling set_webui_type. I'm fine with even having the same if statement structure from the real code and returning the same hardcoded small integers. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2697: const GURL init_kUrl("chrome://foo/"); This variable name doesn't really follow any single style and mixes them all. Be consistent with how the rest of the tests use this - kInitUrl. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2724: // Navigation request for a same-session restore navigation. What is "same-session" navigation? https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2736: TestRenderFrameHost* host = nit: If you are calling the first RFH init_host, this one should probably be current_host. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2765: RenderViewHostChangedObserver change_observer(contents()); Why do we care about RVH? I'd rather not add more usage of RVH as we are trying to remove it. Let's track RFH changes if you need to. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2805: // But there should be a pending WebUI set to re-use the current one. nit: reuse https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2806: EXPECT_EQ(web_ui, host->web_ui()); If there should be a pending WebUI, why aren't you checking the pending_web() of this host? Also, I don't see how we are testing "set to reuse the current one." https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2830: RenderViewHostChangedObserver change_observer(contents()); Same comment as above. If we care about such host changes, we should not be tracking RVH but RFH. https://codereview.chromium.org/1426403006/diff/220001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1426403006/diff/220001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:852: return GetCommittedWebUI() ? GetCommittedWebUI() nit: Why not assign it to WebUI* and avoid the two calls to GetCommittedWebUI? https://codereview.chromium.org/1426403006/diff/220001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:927: : GetRenderManager()->current_frame_host()->web_ui(); Same as above.
Thanks. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_delegate.h:12: #include "content/browser/webui/web_ui_impl.h" On 2015/11/18 23:48:03, nasko wrote: > Can we forward declare WebUIImpl here instead of including it? No because scoped_ptr requires the type to be defined. Curiously this is an almost 3 years old change: https://codereview.chromium.org/11149006 I updated the respective section of the Coding Style to clarify that: https://sites.google.com/a/chromium.org/dev/developers/coding-style?pli=1#TOC... https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_delegate.h:176: // Creates a WebUI object for a frame navigating to the given URL. If no WebUI On 2015/11/18 23:48:03, nasko wrote: > nit: s/to the given URL/to |url|/ Done. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2174: ClearPendingWebUI(); On 2015/11/18 23:48:03, nasko wrote: > nit: I'd clear the pending first, in case some destruction related callbacks end > up calling into RFH for some reason. Done. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:508: void ClearAllWebUIs(); On 2015/11/18 23:48:03, nasko wrote: > nit: drop the "s" at the end of the method name. Done. Is there a rule-of-thumb here? It seemed to make sense to use a plural form... https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:864: // same-site navigation to a document requiring a new WebUI. On 2015/11/18 23:48:03, nasko wrote: > Why do we need a pending WebUI for a same-site navigation? It makes sense to > have it for none->WebUI needed one. This is the main motivation of this revert to having the pending WebUI. It is used exclusively for same-site navigations, when the RFH is reused, and there is a WebUI transition. As shown by the many cases of triggered CHECKs when I landed the original change, this is still happening. In any cross-site navigation the pending WebUI is not used (more precisely, it is used momentarily only to update the active one). I also updated this comment because the word "set" was a tad misleading. These values being null/kNoWebUI are also a case of "usage" when transitioning from having to not having a WebUI. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:688: // Note: if one tries to move same-site commit logic into RenderFrameHost On 2015/11/18 23:48:03, nasko wrote: > nit: s/if/when/. Also start with capital letter as it is a sentence. Done. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1043: // RenderFrameHost. On 2015/11/18 23:48:03, nasko wrote: > What is the scenario when this happens? This function is always called twice for a regular navigation: - 1st at request time, from RenderFrameHostManager::DidCreateNavigationRequest above - 2nd when we are about to commit the navigation on the renderer, from NavigatorImpl::CommitNavigation In the 2nd call if the URL is still the same we don't want to recreate neither the speculative RFH nor its pending WebUI. This is what this comment is referring to. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:183: // RenderFrameHosts. Used during destruction of WebContentsImpl. On 2015/11/18 23:48:03, nasko wrote: > nit: Drop the second sentence. Done. Note that I was just following the model set above. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:99: if (should_create_webui_ && HasWebUIScheme(url)) On 2015/11/18 23:48:03, nasko wrote: > Uh, this is weird, but probably ok at the content/ layer. There are more schemes > that get WebUI. Acknowledged. Note this is not a regression and for this mock it is irrelevant as long as the developer of the test is aware of this. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:109: return reinterpret_cast<WebUI::TypeID>(type_); On 2015/11/18 23:48:04, nasko wrote: > Can we avoid adding this? Can't we just come up with some unique value based on > the URL passed in? It will feel a bit more like the real implementation and it > will avoid the complexity of calling set_webui_type. > I'm fine with even having the same if statement structure from the real code and > returning the same hardcoded small integers. A change like that will force tests to use actual URLs and would make so that they have less control over WebUI transitions. Also any change in the production code could break tests that shouldn't break (false positives) just because a type change (or lack of it) expected in the test stopped happening. As previously discussed [1] the problem here is that WebUIType is a very oddly defined as a void* and each factory makes sure internally that whatever value is passed is unique to itself. I tried to make the usage of this scheme clear in set_webui_type comments above and I think that with the usage examples in my tests it should be easy for others to use it properly. [1] https://codereview.chromium.org/1417953002/diff/40001/content/browser/frame_h... https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2697: const GURL init_kUrl("chrome://foo/"); On 2015/11/18 23:48:03, nasko wrote: > This variable name doesn't really follow any single style and mixes them all. Be > consistent with how the rest of the tests use this - kInitUrl. Done. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2724: // Navigation request for a same-session restore navigation. On 2015/11/18 23:48:03, nasko wrote: > What is "same-session" navigation? This comment was outdated. It meant this should be a tab restore under the same browsing session (when you press CTRL+SHIFT+T). But I in fact based this test on what happens in SessionRestoreTest::RestoreWebUI (session_restore_browsertest.cc) that closes the browser and reopens it from the previous browsing session. I updated the comment accordingly and tried to make it clearer. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2736: TestRenderFrameHost* host = On 2015/11/18 23:48:04, nasko wrote: > nit: If you are calling the first RFH init_host, this one should probably be > current_host. Done. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2765: RenderViewHostChangedObserver change_observer(contents()); On 2015/11/18 23:48:04, nasko wrote: > Why do we care about RVH? I'd rather not add more usage of RVH as we are trying > to remove it. Let's track RFH changes if you need to. I don't think it makes sense to track the RFH either so I removed the observer usage. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2805: // But there should be a pending WebUI set to re-use the current one. On 2015/11/18 23:48:03, nasko wrote: > nit: reuse Done. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2806: EXPECT_EQ(web_ui, host->web_ui()); On 2015/11/18 23:48:04, nasko wrote: > If there should be a pending WebUI, why aren't you checking the pending_web() of > this host? Also, I don't see how we are testing "set to reuse the current one." Yes, this was indeed missing here and all over. I had forgotten to re-add pending WebUI related checks in these 3 tests after I reintroduced them. Done now. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2830: RenderViewHostChangedObserver change_observer(contents()); On 2015/11/18 23:48:03, nasko wrote: > Same comment as above. If we care about such host changes, we should not be > tracking RVH but RFH. Done. https://codereview.chromium.org/1426403006/diff/220001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1426403006/diff/220001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:852: return GetCommittedWebUI() ? GetCommittedWebUI() On 2015/11/18 23:48:04, nasko wrote: > nit: Why not assign it to WebUI* and avoid the two calls to GetCommittedWebUI? Done. https://codereview.chromium.org/1426403006/diff/220001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:927: : GetRenderManager()->current_frame_host()->web_ui(); On 2015/11/18 23:48:04, nasko wrote: > Same as above. Done.
LGTM with a few nits. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:864: // same-site navigation to a document requiring a new WebUI. On 2015/11/19 16:49:14, carlosk wrote: > On 2015/11/18 23:48:03, nasko wrote: > > Why do we need a pending WebUI for a same-site navigation? It makes sense to > > have it for none->WebUI needed one. > > This is the main motivation of this revert to having the pending WebUI. It is > used exclusively for same-site navigations, when the RFH is reused, and there is > a WebUI transition. As shown by the many cases of triggered CHECKs when I landed > the original change, this is still happening. > > In any cross-site navigation the pending WebUI is not used (more precisely, it > is used momentarily only to update the active one). > > I also updated this comment because the word "set" was a tad misleading. These > values being null/kNoWebUI are also a case of "usage" when transitioning from > having to not having a WebUI. Transition from having a WebUI to not having WebUI should be a cross-process navigation. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1043: // RenderFrameHost. On 2015/11/19 16:49:14, carlosk wrote: > On 2015/11/18 23:48:03, nasko wrote: > > What is the scenario when this happens? > > This function is always called twice for a regular navigation: > - 1st at request time, from RenderFrameHostManager::DidCreateNavigationRequest > above > - 2nd when we are about to commit the navigation on the renderer, from > NavigatorImpl::CommitNavigation > > In the 2nd call if the URL is still the same we don't want to recreate neither > the speculative RFH nor its pending WebUI. This is what this comment is > referring to. I find this explanation of yours much more readable and understandable than the comment currently there. Can you add the clarifications of when/how the two calls occur, so it is easier for others to follow along? https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:109: return reinterpret_cast<WebUI::TypeID>(type_); On 2015/11/19 16:49:14, carlosk wrote: > On 2015/11/18 23:48:04, nasko wrote: > > Can we avoid adding this? Can't we just come up with some unique value based > on > > the URL passed in? It will feel a bit more like the real implementation and it > > will avoid the complexity of calling set_webui_type. > > I'm fine with even having the same if statement structure from the real code > and > > returning the same hardcoded small integers. > > A change like that will force tests to use actual URLs and would make so that > they have less control over WebUI transitions. Also any change in the production > code could break tests that shouldn't break (false positives) just because a > type change (or lack of it) expected in the test stopped happening. Why would we have tests that don't use actual URLs? > As previously discussed [1] the problem here is that WebUIType is a very oddly > defined as a void* and each factory makes sure internally that whatever value is > passed is unique to itself. I don't see how this is relevant to this discussion. > I tried to make the usage of this scheme clear in set_webui_type comments above > and I think that with the usage examples in my tests it should be easy for > others to use it properly. > > [1] > https://codereview.chromium.org/1417953002/diff/40001/content/browser/frame_h... It adds one more step for a developer to be aware of not to miss. I know I miss steps that aren't clear to me and this one doesn't seem very obvious. I'd leave it up to you, as I've laid out my arguments. https://codereview.chromium.org/1426403006/diff/240001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/1426403006/diff/240001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2789: // As the initial renderer was not live, the new RenderFrameHost should be nit: "renderer" should be either "renderer process" or "RenderFrame(Host)". Just on its own it is ambiguous which context is meant. https://codereview.chromium.org/1426403006/diff/240001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2889: // Set the WebUI controller to return a different WebUIType value. I'd suggest adding a bit more detail - This will cause the next navigation to "chrome://bar" to be treated as a different WebUI ...
https://codereview.chromium.org/1426403006/diff/220001/content/browser/webui/... File content/browser/webui/web_ui_impl.h (right): https://codereview.chromium.org/1426403006/diff/220001/content/browser/webui/... content/browser/webui/web_ui_impl.h:30: // RenderView, vice-versa or if both have just been created. On 2015/11/18 20:02:01, carlosk wrote: > On 2015/11/18 18:10:58, Dan Beam wrote: > > revert to old comment or explain to me why this is better > > This change addresses a recent request by creis@ [1]. > > During the development of this refactor it seemed to me that RenderViewCreated > should not only be called when a new RenderView is created but also when it > remains the same and a new WebUI is created under it. This happens in a > same-site navigation where the RenderFrameHost/RenderViewHost is kepts but the > WebUI is changed to another type. > > Neither this method's name nor its comment expressed well that logic as they > refer only to creations of the RenderView. > > ... (continues below) then how about: // Called when a RenderView is created for a WebUI (i.e. opening a new // tab) or a WebUI is created for an RenderView (i.e. navigating from // chrome://downloads to chrome://bookmarks). I'm still confused though: if both site instance and webui differ based on origin, how can a site instance be re-used between webui types? https://codereview.chromium.org/1426403006/diff/220001/content/browser/webui/... content/browser/webui/web_ui_impl.h:34: // pair of instances. On 2015/11/18 20:02:01, carlosk wrote: > On 2015/11/18 18:10:58, Dan Beam wrote: > > same > > ... (continued from above) > > Following the same logic, RenderViewReused should work as the complement of the > previous one: it is called in a navigation where none of them change. And again > its name and comment are not clear and I would assume that if only the WebUI > would have changed I should still call this method. > > In other words: if the navigating WebUI has never interacted with the RenderView > before the former method should be called. And if it has already seen it then we > should call the latter. > > [1] > https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... // Called when a renderer is reused for the same WebUI type (i.e. refresh).
Thanks. I'll hit the Commit check now. Fingers crossed! nasko@: If you think fell strongly about other model WebUI type in the controller mock, I'll land another CL with that change. dbeam@: I didn't get an explicit l-g-t-m from you but as I already had it from the previous CL and your comments here are in regards to method's comments, I'm assuming the previous one still stands. If you think my updated comments are still not good, I'll land another CL with the changes. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:864: // same-site navigation to a document requiring a new WebUI. On 2015/11/19 19:45:24, nasko wrote: > On 2015/11/19 16:49:14, carlosk wrote: > > On 2015/11/18 23:48:03, nasko wrote: > > > Why do we need a pending WebUI for a same-site navigation? It makes sense to > > > have it for none->WebUI needed one. > > > > This is the main motivation of this revert to having the pending WebUI. It is > > used exclusively for same-site navigations, when the RFH is reused, and there > is > > a WebUI transition. As shown by the many cases of triggered CHECKs when I > landed > > the original change, this is still happening. > > > > In any cross-site navigation the pending WebUI is not used (more precisely, it > > is used momentarily only to update the active one). > > > > I also updated this comment because the word "set" was a tad misleading. These > > values being null/kNoWebUI are also a case of "usage" when transitioning from > > having to not having a WebUI. > > Transition from having a WebUI to not having WebUI should be a cross-process > navigation. It should indeed and that's what we expected when the 1st CL landed. See http://crbug.com/557116#c1 (restricted) for links to crash reports of when it wasn't . https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:1043: // RenderFrameHost. On 2015/11/19 19:45:24, nasko wrote: > On 2015/11/19 16:49:14, carlosk wrote: > > On 2015/11/18 23:48:03, nasko wrote: > > > What is the scenario when this happens? > > > > This function is always called twice for a regular navigation: > > - 1st at request time, from RenderFrameHostManager::DidCreateNavigationRequest > > above > > - 2nd when we are about to commit the navigation on the renderer, from > > NavigatorImpl::CommitNavigation > > > > In the 2nd call if the URL is still the same we don't want to recreate neither > > the speculative RFH nor its pending WebUI. This is what this comment is > > referring to. > > I find this explanation of yours much more readable and understandable than the > comment currently there. Can you add the clarifications of when/how the two > calls occur, so it is easier for others to follow along? Done. I incorrectly referred to the speculative RFH in my previous reply. I fixed that in the added comments and also added a similar one for the cross-site case. https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/1426403006/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:109: return reinterpret_cast<WebUI::TypeID>(type_); On 2015/11/19 19:45:24, nasko wrote: > On 2015/11/19 16:49:14, carlosk wrote: > > On 2015/11/18 23:48:04, nasko wrote: > > > Can we avoid adding this? Can't we just come up with some unique value based > > on > > > the URL passed in? It will feel a bit more like the real implementation and > it > > > will avoid the complexity of calling set_webui_type. > > > I'm fine with even having the same if statement structure from the real code > > and > > > returning the same hardcoded small integers. > > > > A change like that will force tests to use actual URLs and would make so that > > they have less control over WebUI transitions. Also any change in the > production > > code could break tests that shouldn't break (false positives) just because a > > type change (or lack of it) expected in the test stopped happening. > > Why would we have tests that don't use actual URLs? Sorry: actual *WebUI* URLs (chrome://gpu, chrome://settings, ...). It wouldn't be possible to just use chrome://foo. > > As previously discussed [1] the problem here is that WebUIType is a very oddly > > defined as a void* and each factory makes sure internally that whatever value > is > > passed is unique to itself. > > I don't see how this is relevant to this discussion. This was to justify why this is indeed an odd solution: it mocks an odd choice of type value. > > I tried to make the usage of this scheme clear in set_webui_type comments > above > > and I think that with the usage examples in my tests it should be easy for > > others to use it properly. > > > > [1] > > > https://codereview.chromium.org/1417953002/diff/40001/content/browser/frame_h... > > It adds one more step for a developer to be aware of not to miss. I know I miss > steps that aren't clear to me and this one doesn't seem very obvious. I'd leave > it up to you, as I've laid out my arguments. The extra step is required if and only if the developer needs different WebUI types to be returned by the controller mock. Among existent and new WebUI tests, all but 1 didn't need it. Base the return value on the provided URL would require creating a "translation rule" from URL to some type value. This value would be arbitrary and would require the developer to understand the rule to be able to use it properly. When compared to this solution it would IMO make what's going on less explicit when reading the tests. Have an if-else table would require all tests to use and to conform to whatever URLs are defined in this table to have the returned types working as needed. This seems like a good alternative to me but not necessarily simpler or easier to follow. If this seems better to you I will implement it in a follow up CL. https://codereview.chromium.org/1426403006/diff/220001/content/browser/webui/... File content/browser/webui/web_ui_impl.h (right): https://codereview.chromium.org/1426403006/diff/220001/content/browser/webui/... content/browser/webui/web_ui_impl.h:30: // RenderView, vice-versa or if both have just been created. On 2015/11/20 00:07:59, Dan Beam wrote: > On 2015/11/18 20:02:01, carlosk wrote: > > On 2015/11/18 18:10:58, Dan Beam wrote: > > > revert to old comment or explain to me why this is better > > > > This change addresses a recent request by creis@ [1]. > > > > During the development of this refactor it seemed to me that RenderViewCreated > > should not only be called when a new RenderView is created but also when it > > remains the same and a new WebUI is created under it. This happens in a > > same-site navigation where the RenderFrameHost/RenderViewHost is kepts but the > > WebUI is changed to another type. > > > > Neither this method's name nor its comment expressed well that logic as they > > refer only to creations of the RenderView. > > > > ... (continues below) > > then how about: > > // Called when a RenderView is created for a WebUI (i.e. opening a new > // tab) or a WebUI is created for an RenderView (i.e. navigating from > // chrome://downloads to chrome://bookmarks). Done (with updates). > I'm still confused though: if both site instance and webui differ based on > origin, how can a site instance be re-used between webui types? We tried to avoid this kind of re-usage (see nasko@'s landed changes associated with the same issue, http://crbug.com/508850) but it wasn't enough. Some of the crashes we had were due to that still happening. See http://crbug.com/557116#c1 (restricted) for examples. https://codereview.chromium.org/1426403006/diff/220001/content/browser/webui/... content/browser/webui/web_ui_impl.h:34: // pair of instances. On 2015/11/20 00:07:59, Dan Beam wrote: > On 2015/11/18 20:02:01, carlosk wrote: > > On 2015/11/18 18:10:58, Dan Beam wrote: > > > same > > > > ... (continued from above) > > > > Following the same logic, RenderViewReused should work as the complement of > the > > previous one: it is called in a navigation where none of them change. And > again > > its name and comment are not clear and I would assume that if only the WebUI > > would have changed I should still call this method. > > > > In other words: if the navigating WebUI has never interacted with the > RenderView > > before the former method should be called. And if it has already seen it then > we > > should call the latter. > > > > [1] > > > https://codereview.chromium.org/1426403006/diff/1/content/browser/frame_host/... > > // Called when a renderer is reused for the same WebUI type (i.e. refresh). Done (with minor updates). https://codereview.chromium.org/1426403006/diff/240001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/1426403006/diff/240001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2789: // As the initial renderer was not live, the new RenderFrameHost should be On 2015/11/19 19:45:24, nasko wrote: > nit: "renderer" should be either "renderer process" or "RenderFrame(Host)". Just > on its own it is ambiguous which context is meant. Done. https://codereview.chromium.org/1426403006/diff/240001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:2889: // Set the WebUI controller to return a different WebUIType value. On 2015/11/19 19:45:24, nasko wrote: > I'd suggest adding a bit more detail - This will cause the next navigation to > "chrome://bar" to be treated as a different WebUI ... Done.
The CQ bit was checked by carlosk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/1426403006/#ps260001 (title: "Address CR comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1426403006/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1426403006/260001
Message was sent while issue was closed.
Committed patchset #12 (id:260001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/113503ad7f6957c1eb2bb12215c8d5fae3303ef6 Cr-Commit-Position: refs/heads/master@{#360807}
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:260001) has been created in https://codereview.chromium.org/1454883006/ by grt@chromium.org. The reason for reverting is: ExtensionApiTest.Debugger browser_tests failed on Win7 Tests (dbg)(1) Build #43339 with [3192:3164:1120/071447:FATAL:render_frame_host_manager.cc(2372)] Check failed: !render_frame_host_->pending_web_ui(). Which was introduced with this change.. |
