Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7048015:
Fix app process initialization when opening an app in a new tab. (Closed)
|
Created:
9 years, 7 months ago by Charlie Reis Modified:
9 years, 7 months ago Reviewers:
Matt Perry CC:
chromium-reviews, Aaron Boodman, Erik does not do reviews, pam+watch_chromium.org, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix app process initialization when opening an app in a new tab.
Also test that hosted apps do not receive WebUI.
BUG=83289
TEST=AppApiTest.AppProcess
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86161
Patch Set 1 #
Total comments: 6
Patch Set 2 : Move to GetWebUIFactoryFunction. #Patch Set 3 : Don't use WebUI. #
Messages
Total messages: 8 (0 generated)
Please see the bug comments for why I think this is the right approach. Thanks, Charlie
http://codereview.chromium.org/7048015/diff/1/chrome/browser/ui/webui/chrome_... File chrome/browser/ui/webui/chrome_web_ui_factory.cc (right): http://codereview.chromium.org/7048015/diff/1/chrome/browser/ui/webui/chrome_... chrome/browser/ui/webui/chrome_web_ui_factory.cc:232: // Convert installed app URLs to extension URLs to classify them properly. Shouldn't this be done in GetWebUIType so that we use the right WebUI as well? Actually now that I think about it, do we want to use WebUI for hosted apps at all? I think hosted apps are unprivileged. Unfortunately I'm not very familiar with how hosted apps work. It might be safer just to keep the check local to BrowserNavigator's GetSiteInstance (which also seems weird to be basing its decision off of WebUI).
http://codereview.chromium.org/7048015/diff/1/chrome/browser/ui/webui/chrome_... File chrome/browser/ui/webui/chrome_web_ui_factory.cc (right): http://codereview.chromium.org/7048015/diff/1/chrome/browser/ui/webui/chrome_... chrome/browser/ui/webui/chrome_web_ui_factory.cc:232: // Convert installed app URLs to extension URLs to classify them properly. On 2011/05/19 22:15:11, Matt Perry wrote: > Shouldn't this be done in GetWebUIType so that we use the right WebUI as well? Ah, yes, good point. I think it actually belongs in GetUIFactoryFunction. > Actually now that I think about it, do we want to use WebUI for hosted apps at > all? I think hosted apps are unprivileged. Unfortunately I'm not very familiar > with how hosted apps work. We're already treating hosted apps as extension URLs in some places, which affects the process model we use for them (process-per-app, rather than process-per-app-instance) even if they're not privileged. It's also important that hosted apps get the proper type of RenderProcessHost, especially for isolated apps that have their own cookie store. > It might be safer just to keep the check local to BrowserNavigator's > GetSiteInstance (which also seems weird to be basing its decision off of WebUI). It looks like the other call sites of UseWebUIForURL in RenderViewHostManager face the same risk of getting this wrong. I think we want to be consistent about treating hosted apps as WebUI, but if you can think of a case where that's not true, let me know. (If we decide there's a reason to avoid this, we should add a test, because this version is passing the try bots, apart from the FavIconChange flake.)
http://codereview.chromium.org/7048015/diff/1/chrome/browser/ui/webui/chrome_... File chrome/browser/ui/webui/chrome_web_ui_factory.cc (right): http://codereview.chromium.org/7048015/diff/1/chrome/browser/ui/webui/chrome_... chrome/browser/ui/webui/chrome_web_ui_factory.cc:232: // Convert installed app URLs to extension URLs to classify them properly. On 2011/05/19 23:02:15, creis wrote: > On 2011/05/19 22:15:11, Matt Perry wrote: > > Shouldn't this be done in GetWebUIType so that we use the right WebUI as well? > > Ah, yes, good point. I think it actually belongs in GetUIFactoryFunction. > > > Actually now that I think about it, do we want to use WebUI for hosted apps at > > all? I think hosted apps are unprivileged. Unfortunately I'm not very familiar > > with how hosted apps work. > > We're already treating hosted apps as extension URLs in some places, which > affects the process model we use for them (process-per-app, rather than > process-per-app-instance) even if they're not privileged. It's also important > that hosted apps get the proper type of RenderProcessHost, especially for > isolated apps that have their own cookie store. Could you verify that we are using the same WebUI for hosted app URLs before and after this change? (i.e. if we weren't using a WebUI before, we still shouldn't.)
http://codereview.chromium.org/7048015/diff/1/chrome/browser/ui/webui/chrome_... File chrome/browser/ui/webui/chrome_web_ui_factory.cc (right): http://codereview.chromium.org/7048015/diff/1/chrome/browser/ui/webui/chrome_... chrome/browser/ui/webui/chrome_web_ui_factory.cc:232: // Convert installed app URLs to extension URLs to classify them properly. On 2011/05/19 23:04:43, Matt Perry wrote: > On 2011/05/19 23:02:15, creis wrote: > > On 2011/05/19 22:15:11, Matt Perry wrote: > > > Shouldn't this be done in GetWebUIType so that we use the right WebUI as > well? > > > > Ah, yes, good point. I think it actually belongs in GetUIFactoryFunction. > > > > > Actually now that I think about it, do we want to use WebUI for hosted apps > at > > > all? I think hosted apps are unprivileged. Unfortunately I'm not very > familiar > > > with how hosted apps work. > > > > We're already treating hosted apps as extension URLs in some places, which > > affects the process model we use for them (process-per-app, rather than > > process-per-app-instance) even if they're not privileged. It's also important > > that hosted apps get the proper type of RenderProcessHost, especially for > > isolated apps that have their own cookie store. > > Could you verify that we are using the same WebUI for hosted app URLs before and > after this change? (i.e. if we weren't using a WebUI before, we still > shouldn't.) Wow, the WebUI code is kind of hard to follow. I focused on what happened when CreateWebUIForURL was called. Before my change, we were not creating a WebUI for hosted apps, and RVHM::pending_web_ui_ was null. My change (once I fixed an inconsistency in which URL we were using) did cause it to create a WebUI. Sounds like my approach is wrong, then, but can you explain what's wrong about it? What are the implications for having a WebUI or not? Seems strange to me that we consider hosted app URLs to be extension URLs (and thus worthy of WebUI) in some places but not when we actually create the WebUI. I can move my change to BrowserNavigator, but I'd like to add a check (and explanation) in the AppProcess test if we expect a hosted app not to have WebUI.
http://codereview.chromium.org/7048015/diff/1/chrome/browser/ui/webui/chrome_... File chrome/browser/ui/webui/chrome_web_ui_factory.cc (right): http://codereview.chromium.org/7048015/diff/1/chrome/browser/ui/webui/chrome_... chrome/browser/ui/webui/chrome_web_ui_factory.cc:232: // Convert installed app URLs to extension URLs to classify them properly. On 2011/05/19 23:53:07, creis wrote: > On 2011/05/19 23:04:43, Matt Perry wrote: > > On 2011/05/19 23:02:15, creis wrote: > > > On 2011/05/19 22:15:11, Matt Perry wrote: > > > > Shouldn't this be done in GetWebUIType so that we use the right WebUI as > > well? > > > > > > Ah, yes, good point. I think it actually belongs in GetUIFactoryFunction. > > > > > > > Actually now that I think about it, do we want to use WebUI for hosted > apps > > at > > > > all? I think hosted apps are unprivileged. Unfortunately I'm not very > > familiar > > > > with how hosted apps work. > > > > > > We're already treating hosted apps as extension URLs in some places, which > > > affects the process model we use for them (process-per-app, rather than > > > process-per-app-instance) even if they're not privileged. It's also > important > > > that hosted apps get the proper type of RenderProcessHost, especially for > > > isolated apps that have their own cookie store. > > > > Could you verify that we are using the same WebUI for hosted app URLs before > and > > after this change? (i.e. if we weren't using a WebUI before, we still > > shouldn't.) > > Wow, the WebUI code is kind of hard to follow. I focused on what happened when > CreateWebUIForURL was called. > > Before my change, we were not creating a WebUI for hosted apps, and > RVHM::pending_web_ui_ was null. My change (once I fixed an inconsistency in > which URL we were using) did cause it to create a WebUI. > > Sounds like my approach is wrong, then, but can you explain what's wrong about > it? What are the implications for having a WebUI or not? Seems strange to me > that we consider hosted app URLs to be extension URLs (and thus worthy of WebUI) > in some places but not when we actually create the WebUI. > > I can move my change to BrowserNavigator, but I'd like to add a check (and > explanation) in the AppProcess test if we expect a hosted app not to have WebUI. WebUI is a way of exposing extra bindings to a tab. Extensions pages loaded in a tab are given an ExtensionWebUI, which gives that tab extension bindings. We want to do this for chrome-extension URLs, but not for hosted app URLs, because the latter should be unprivileged. For hosted app URLs, we want them to be grouped in a single process, so the app has script access to all its components (including backround windows), but we don't want to given them extension bindings.
http://codereview.chromium.org/7048015/diff/1/chrome/browser/ui/webui/chrome_... File chrome/browser/ui/webui/chrome_web_ui_factory.cc (right): http://codereview.chromium.org/7048015/diff/1/chrome/browser/ui/webui/chrome_... chrome/browser/ui/webui/chrome_web_ui_factory.cc:232: // Convert installed app URLs to extension URLs to classify them properly. On 2011/05/20 00:03:47, Matt Perry wrote: > WebUI is a way of exposing extra bindings to a tab. Extensions pages loaded in a > tab are given an ExtensionWebUI, which gives that tab extension bindings. We > want to do this for chrome-extension URLs, but not for hosted app URLs, because > the latter should be unprivileged. > > For hosted app URLs, we want them to be grouped in a single process, so the app > has script access to all its components (including backround windows), but we > don't want to given them extension bindings. Thanks-- that clarifies things. I've added a check and explanation to the browser test, and I changed BrowserNavigator to explicitly check for both WebUI and extensions (as the comment already claimed it did).
LGTM |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
