Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1315723003:
Make WebUIImpl a WebContentsObserver to re-use RenderViewCreated() (Closed)
|
Created:
5 years, 3 months ago by Dan Beam Modified:
5 years, 3 months ago Reviewers:
Charlie Reis CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, creis+watch_chromium.org, darin-cc_chromium.org, nasko Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake WebUIImpl a WebContentsObserver to re-use RenderViewCreated()
Reduces the amount of code in WebContentsImpl by re-using this class.
RenderViewCreated is dispatched right after the custom method is called.
R=creis@chromium.org
BUG=516690
Patch Set 1 #
Total comments: 5
Messages
Total messages: 9 (0 generated)
https://codereview.chromium.org/1315723003/diff/1/content/browser/webui/web_u... File content/browser/webui/web_ui_impl.h (right): https://codereview.chromium.org/1315723003/diff/1/content/browser/webui/web_u... content/browser/webui/web_ui_impl.h:104: WebContents* web_contents_; NOTE: I attempted to use WebContentsObserver::web_contents() and remove WebUIImpl::web_contents_, but WebContentsObserver::ResetWebContents() sets WebContentsObserver::web_contents_ = nullptr[1] before RenderFrameHostManager::web_ui_ dies[2]. Some webui handlers call WebUIImpl::GetWebContents() in their destructors to clean up state, so they need WebUIImpl::GetWebContents() to remain non-null.[3][4] I assume this is safe but subtle. [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... [2] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... [3] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... [4] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...
Yes, this seems like a better approach that decouples the WebUI code from WebContentsImpl a bit more. I like it. A few questions below. https://codereview.chromium.org/1315723003/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl.cc (left): https://codereview.chromium.org/1315723003/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:3777: if (GetRenderManager()->pending_web_ui()) I'm noticing that this old code only passes on the RenderViewCreated call if there's a pending or speculative WebUI object, and not for other RenderViewHosts. See my note in WebUIImpl::RenderViewCreated about the implications. https://codereview.chromium.org/1315723003/diff/1/content/browser/webui/web_u... File content/browser/webui/web_ui_impl.cc (right): https://codereview.chromium.org/1315723003/diff/1/content/browser/webui/web_u... content/browser/webui/web_ui_impl.cc:86: controller_->RenderViewCreated(render_view_host); Do we need to check whether this is our RVH before acting on it? For example, suppose we're on chrome://version and then navigate to chromium.org. WebContentsObserver::RenderViewCreated will get called for the pending, non-WebUI RVH, and the chrome://version WebUIImpl object will hear about it. Would checking whether the RVH's SiteInstance matches the target frame's SiteInstance might work? https://codereview.chromium.org/1315723003/diff/1/content/browser/webui/web_u... File content/browser/webui/web_ui_impl.h (right): https://codereview.chromium.org/1315723003/diff/1/content/browser/webui/web_u... content/browser/webui/web_ui_impl.h:104: WebContents* web_contents_; On 2015/08/28 01:50:53, Dan Beam wrote: > NOTE: I attempted to use WebContentsObserver::web_contents() and remove > WebUIImpl::web_contents_, but WebContentsObserver::ResetWebContents() sets > WebContentsObserver::web_contents_ = nullptr[1] before > RenderFrameHostManager::web_ui_ dies[2]. Some webui handlers call > WebUIImpl::GetWebContents() in their destructors to clean up state, so they need > WebUIImpl::GetWebContents() to remain non-null.[3][4] > > I assume this is safe but subtle. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... > [3] > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > [4] > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro... Thanks for looking into this. It seems a bit dangerous for WebUI objects to use a mostly destructed WebContents, so it's worth seeing if we can fix this and use WCO::web_contents(). That way we'll be consistent with other WebContents observers, which get notified at a reasonably safe point in time. Profile::FromWebUI just needs the BrowserContext from the WebContents. Maybe we could store that on WebUI instead of web_contents_, since it's guaranteed to outlive the RFHM that keeps most WebUI objects alive? (Not sure if there are other uses of GetWebContents in various destructors, though.) Alternatively, we could have ~WebContentsImpl tell the root RFHM to delete its web_ui_ before calling ResetWebContents. (The subframe RFHMs have already been deleted by that point, so we don't need to worry about them.) NOTE: This issue exists even before this CL lands, so I'm fine with addressing it in a followup CL. If you want to do that, just leave a TODO here.
https://codereview.chromium.org/1315723003/diff/1/content/browser/webui/web_u... File content/browser/webui/web_ui_impl.h (right): https://codereview.chromium.org/1315723003/diff/1/content/browser/webui/web_u... content/browser/webui/web_ui_impl.h:104: WebContents* web_contents_; On 2015/08/28 17:05:56, Charlie Reis wrote: > On 2015/08/28 01:50:53, Dan Beam wrote: > > NOTE: I attempted to use WebContentsObserver::web_contents() and remove > > WebUIImpl::web_contents_, but WebContentsObserver::ResetWebContents() sets > > WebContentsObserver::web_contents_ = nullptr[1] before > > RenderFrameHostManager::web_ui_ dies[2]. Some webui handlers call > > WebUIImpl::GetWebContents() in their destructors to clean up state, so they > need > > WebUIImpl::GetWebContents() to remain non-null.[3][4] > > > > I assume this is safe but subtle. > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... > > [2] > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... > > [3] > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > [4] > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro... > > Thanks for looking into this. It seems a bit dangerous for WebUI objects to use > a mostly destructed WebContents, so it's worth seeing if we can fix this and use > WCO::web_contents(). That way we'll be consistent with other WebContents > observers, which get notified at a reasonably safe point in time. > > Profile::FromWebUI just needs the BrowserContext from the WebContents. Maybe we > could store that on WebUI instead of web_contents_, since it's guaranteed to > outlive the RFHM that keeps most WebUI objects alive? (Not sure if there are > other uses of GetWebContents in various destructors, though.) > > Alternatively, we could have ~WebContentsImpl tell the root RFHM to delete its > web_ui_ before calling ResetWebContents. (The subframe RFHMs have already been > deleted by that point, so we don't need to worry about them.) > > NOTE: This issue exists even before this CL lands, so I'm fine with addressing > it in a followup CL. If you want to do that, just leave a TODO here. I'd rather nuke RenderFrameHostManager::web_ui_ earlier, yeah. I'll see how safe that is.
I think if WebUIs were owned by a RFH instead of RFHM, it'd fix some wonkiness (or maybe make other wonkiness). This probably wouldn't allow them to be re-used, though, as they currently are. By the way: deleting the WebUI earlier works, but I didn't notice the other issues you spotted with this patch (that RenderViewCreated() were only called for pending/speculative WebUIs) until now.
[+nasko to CC] On 2015/08/29 01:26:47, Dan Beam wrote: > I think if WebUIs were owned by a RFH instead of RFHM, it'd fix some wonkiness > (or maybe make other wonkiness). This probably wouldn't allow them to be > re-used, though, as they currently are. I would love to see that happen at some point. Nasko and I have chatted about it, and it seems like WebUI is much more tied to RFH and it's kind of awkward tracking its lifetime in RFHM. In which cases would the same WebUI object be used for two different RFHs? That seems odd from a security perspective. > By the way: deleting the WebUI earlier works, but I didn't notice the other > issues you spotted with this patch (that RenderViewCreated() were only called > for pending/speculative WebUIs) until now. Cool. Just ping me when it's ready for another round.
On 2015/08/31 19:07:18, Charlie Reis wrote: > [+nasko to CC] > > On 2015/08/29 01:26:47, Dan Beam wrote: > > I think if WebUIs were owned by a RFH instead of RFHM, it'd fix some wonkiness > > (or maybe make other wonkiness). This probably wouldn't allow them to be > > re-used, though, as they currently are. > > I would love to see that happen at some point. Nasko and I have chatted about > it, and it seems like WebUI is much more tied to RFH and it's kind of awkward > tracking its lifetime in RFHM. > > In which cases would the same WebUI object be used for two different RFHs? That > seems odd from a security perspective. Well, currently UberUI::sub_uis_ does this, but I'm pretty sure it was to fill a gap in the framework. > > > By the way: deleting the WebUI earlier works, but I didn't notice the other > > issues you spotted with this patch (that RenderViewCreated() were only called > > for pending/speculative WebUIs) until now. > > Cool. Just ping me when it's ready for another round. I'm not sure it's worth making this class a WebContentsObserver just to use web_contents()
On 2015/09/01 15:31:37, Dan Beam wrote: > On 2015/08/31 19:07:18, Charlie Reis wrote: > > [+nasko to CC] > > > > On 2015/08/29 01:26:47, Dan Beam wrote: > > > I think if WebUIs were owned by a RFH instead of RFHM, it'd fix some > wonkiness > > > (or maybe make other wonkiness). This probably wouldn't allow them to be > > > re-used, though, as they currently are. > > > > I would love to see that happen at some point. Nasko and I have chatted about > > it, and it seems like WebUI is much more tied to RFH and it's kind of awkward > > tracking its lifetime in RFHM. > > > > In which cases would the same WebUI object be used for two different RFHs? > That > > seems odd from a security perspective. > > Well, currently UberUI::sub_uis_ does this, but I'm pretty sure it was to fill a > gap in the framework. > > > > > By the way: deleting the WebUI earlier works, but I didn't notice the other > > > issues you spotted with this patch (that RenderViewCreated() were only > called > > > for pending/speculative WebUIs) until now. > > > > Cool. Just ping me when it's ready for another round. > > I'm not sure it's worth making this class a WebContentsObserver just to use > web_contents() I think the RenderViewCreated call can still work if you check the target frame's SiteInstance. (Plus the original reason for making this class a WebContentsObserver was to listen to navigation commits, but I suppose that's only useful depending on how the JS issue gets fixed.) Anyway, up to you if this is no longer a useful change.
On 2015/09/01 22:38:01, Charlie Reis (OOO till 9-8) wrote: > On 2015/09/01 15:31:37, Dan Beam wrote: > > On 2015/08/31 19:07:18, Charlie Reis wrote: > > > [+nasko to CC] > > > > > > On 2015/08/29 01:26:47, Dan Beam wrote: > > > > I think if WebUIs were owned by a RFH instead of RFHM, it'd fix some > > wonkiness > > > > (or maybe make other wonkiness). This probably wouldn't allow them to be > > > > re-used, though, as they currently are. > > > > > > I would love to see that happen at some point. Nasko and I have chatted > about > > > it, and it seems like WebUI is much more tied to RFH and it's kind of > awkward > > > tracking its lifetime in RFHM. > > > > > > In which cases would the same WebUI object be used for two different RFHs? > > That > > > seems odd from a security perspective. > > > > Well, currently UberUI::sub_uis_ does this, but I'm pretty sure it was to fill > a > > gap in the framework. > > > > > > > By the way: deleting the WebUI earlier works, but I didn't notice the > other > > > > issues you spotted with this patch (that RenderViewCreated() were only > > called > > > > for pending/speculative WebUIs) until now. > > > > > > Cool. Just ping me when it's ready for another round. > > > > I'm not sure it's worth making this class a WebContentsObserver just to use > > web_contents() > > I think the RenderViewCreated call can still work if you check the target > frame's SiteInstance. This isn't guaranteed, though, right? https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... > > (Plus the original reason for making this class a WebContentsObserver was to > listen to navigation commits, but I suppose that's only useful depending on how > the JS issue gets fixed.) Correct > > Anyway, up to you if this is no longer a useful change. If we really want to change how WebUIs are destructed, I guess we can, but I foresee subtle issues...
On 2015/09/04 00:06:10, Dan Beam wrote: > On 2015/09/01 22:38:01, Charlie Reis (OOO till 9-8) wrote: > > On 2015/09/01 15:31:37, Dan Beam wrote: > > > On 2015/08/31 19:07:18, Charlie Reis wrote: > > > > [+nasko to CC] > > > > > > > > On 2015/08/29 01:26:47, Dan Beam wrote: > > > > > I think if WebUIs were owned by a RFH instead of RFHM, it'd fix some > > > wonkiness > > > > > (or maybe make other wonkiness). This probably wouldn't allow them to > be > > > > > re-used, though, as they currently are. > > > > > > > > I would love to see that happen at some point. Nasko and I have chatted > > about > > > > it, and it seems like WebUI is much more tied to RFH and it's kind of > > awkward > > > > tracking its lifetime in RFHM. > > > > > > > > In which cases would the same WebUI object be used for two different RFHs? > > > > That > > > > seems odd from a security perspective. > > > > > > Well, currently UberUI::sub_uis_ does this, but I'm pretty sure it was to > fill > > a > > > gap in the framework. > > > > > > > > > By the way: deleting the WebUI earlier works, but I didn't notice the > > other > > > > > issues you spotted with this patch (that RenderViewCreated() were only > > > called > > > > > for pending/speculative WebUIs) until now. > > > > > > > > Cool. Just ping me when it's ready for another round. > > > > > > I'm not sure it's worth making this class a WebContentsObserver just to use > > > web_contents() > > > > I think the RenderViewCreated call can still work if you check the target > > frame's SiteInstance. > > This isn't guaranteed, though, right? > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... > > > > > (Plus the original reason for making this class a WebContentsObserver was to > > listen to navigation commits, but I suppose that's only useful depending on > how > > the JS issue gets fixed.) > > Correct > > > > > Anyway, up to you if this is no longer a useful change. > > If we really want to change how WebUIs are destructed, I guess we can, but I > foresee subtle issues... how WebUIs are destroyed** |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
