Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23530029:
Support webview tag when the container extension is embedded in a webUI (Closed)
|
Created:
7 years, 3 months ago by guohui Modified:
7 years, 3 months ago Reviewers:
Fady Samuel, Charlie Reis, jochen (gone - plz use gerrit), Roger Tawa OOO till Jul 10th, Cris Neckar CC:
chromium-reviews, joi+watch-content_chromium.org, chromium-apps-reviews_chromium.org, darin-cc_chromium.org, jam, extensions-reviews_chromium.org, Roger Tawa OOO till Jul 10th Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSupport webview tag when the container ext/app is embedded in a webUI
Design doc at https://docs.google.com/a/google.com/document/d/1LcriMmLY76IUhFO5rWh3Tz6xDFkGPNnL4MYSIBD4Jbc/edit#
BUG=285151
R=cdn@chromium.org, creis@chromium.org, fsamuel@chromium.org, jochen@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=225014
Patch Set 1 : initial #Patch Set 2 : Charlie's comments addressed #Patch Set 3 : guest site instance fixed #
Total comments: 4
Patch Set 4 : fady's comment addressed #
Total comments: 10
Patch Set 5 : Removed unnecessary lines from document_custom_bindings #
Total comments: 34
Patch Set 6 : added URL filtering #Patch Set 7 : Rename ChromeContentRendererClient::GetExtension #Patch Set 8 : remove changes for unblessed API #Patch Set 9 : rebased #Patch Set 10 : fixed init order #Patch Set 11 : merged #Messages
Total messages: 36 (0 generated)
Hello guys, The CL is ready for initial review. Thanks, Hui
On 2013/09/04 16:10:42, guohui wrote: > Hello guys, > > The CL is ready for initial review. > > Thanks, > > Hui @charlie, i uploaded a new patch based on your comments in the doc. To summarize, 1. it no longer exposes extension privileges to WebUI process. Instead it marks WebView API as unprivileged so that a WebView could be loaded in an iframed extension. 2. instead of associating an "active" extension ID with a WebUI URL at constructor time, it passes the parent frame URL when attaching a WebView guest, and extracts the extension ID from the parent frame URL.
On 2013/09/12 19:34:05, guohui wrote: > On 2013/09/04 16:10:42, guohui wrote: > > Hello guys, > > > > The CL is ready for initial review. > > > > Thanks, > > > > Hui > > @charlie, i uploaded a new patch based on your comments in the doc. To > summarize, > > 1. it no longer exposes extension privileges to WebUI process. Instead it marks > WebView API as unprivileged so that a WebView could be loaded in an iframed > extension. > 2. instead of associating an "active" extension ID with a WebUI URL at > constructor time, it passes the parent frame URL when attaching a WebView guest, > and extracts the extension ID from the parent frame URL. Uploaded another patch that generates the guest site instance based on the embedder frame URL instead of the top frame URL. The new patch fixed the issue that when the embedder extension is iframed in a WebUI, the storage partition is isolated per WebUI instead of per extension.
https://codereview.chromium.org/23530029/diff/23001/content/browser/browser_p... File content/browser/browser_plugin/browser_plugin_guest_manager.cc (right): https://codereview.chromium.org/23530029/diff/23001/content/browser/browser_p... content/browser/browser_plugin/browser_plugin_guest_manager.cc:73: const std::string& host = GURL(params.embedder_frame_url).host(); Wow, this is a bit scary. You should only be doing this if the embedder is a WebUI because we trust WebUI processes. We don't trust embedders to report their iframes correctly, in general.
https://codereview.chromium.org/23530029/diff/23001/content/browser/browser_p... File content/browser/browser_plugin/browser_plugin_guest_manager.cc (right): https://codereview.chromium.org/23530029/diff/23001/content/browser/browser_p... content/browser/browser_plugin/browser_plugin_guest_manager.cc:73: const std::string& host = GURL(params.embedder_frame_url).host(); On 2013/09/12 21:54:38, Fady Samuel wrote: > Wow, this is a bit scary. You should only be doing this if the embedder is a > WebUI because we trust WebUI processes. We don't trust embedders to report their > iframes correctly, in general. We got the frame URL you mean the frame URL we got when creating BrowserPlugin cannot be trusted in general? hmm, so we don't trust them to report iframe URL correctly but we do trust them to report the top frame URL correctly? Could you please explain a bit more?
https://codereview.chromium.org/23530029/diff/23001/content/browser/browser_p... File content/browser/browser_plugin/browser_plugin_guest_manager.cc (right): https://codereview.chromium.org/23530029/diff/23001/content/browser/browser_p... content/browser/browser_plugin/browser_plugin_guest_manager.cc:73: const std::string& host = GURL(params.embedder_frame_url).host(); On 2013/09/12 22:06:08, guohui wrote: > On 2013/09/12 21:54:38, Fady Samuel wrote: > > Wow, this is a bit scary. You should only be doing this if the embedder is a > > WebUI because we trust WebUI processes. We don't trust embedders to report > their > > iframes correctly, in general. > We got the frame URL > you mean the frame URL we got when creating BrowserPlugin cannot be trusted in > general? > hmm, so we don't trust them to report iframe URL correctly but we do trust them > to report the top frame URL correctly? Could you please explain a bit more? The embedder's site instance is a URL generated by the browser process (which is trusted), the embedder_frame_url is coming from the embedder render process (which is less trusted).
https://codereview.chromium.org/23530029/diff/23001/content/browser/browser_p... File content/browser/browser_plugin/browser_plugin_guest_manager.cc (right): https://codereview.chromium.org/23530029/diff/23001/content/browser/browser_p... content/browser/browser_plugin/browser_plugin_guest_manager.cc:73: const std::string& host = GURL(params.embedder_frame_url).host(); On 2013/09/12 22:11:21, Fady Samuel wrote: > On 2013/09/12 22:06:08, guohui wrote: > > On 2013/09/12 21:54:38, Fady Samuel wrote: > > > Wow, this is a bit scary. You should only be doing this if the embedder is a > > > WebUI because we trust WebUI processes. We don't trust embedders to report > > their > > > iframes correctly, in general. > > We got the frame URL > > you mean the frame URL we got when creating BrowserPlugin cannot be trusted in > > general? > > hmm, so we don't trust them to report iframe URL correctly but we do trust > them > > to report the top frame URL correctly? Could you please explain a bit more? > > The embedder's site instance is a URL generated by the browser process (which is > trusted), the embedder_frame_url is coming from the embedder render process > (which is less trusted). that makes sense, thanks! fixed as suggested.
https://codereview.chromium.org/23530029/diff/37001/chrome/browser/chrome_con... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/23530029/diff/37001/chrome/browser/chrome_con... chrome/browser/chrome_content_browser_client.cc:2351: if (content::HasWebUIScheme(site_url)) We should probably do the same check as above here rather than simply always allowing BrowserPlugin in WebUI. https://codereview.chromium.org/23530029/diff/37001/chrome/renderer/extension... File chrome/renderer/extensions/document_custom_bindings.cc (right): https://codereview.chromium.org/23530029/diff/37001/chrome/renderer/extension... chrome/renderer/extensions/document_custom_bindings.cc:11: #include "content/public/renderer/render_view.h" This probably not necessary. https://codereview.chromium.org/23530029/diff/37001/chrome/renderer/extension... chrome/renderer/extensions/document_custom_bindings.cc:14: #include "third_party/WebKit/public/web/WebView.h" This line might not be necessary. https://codereview.chromium.org/23530029/diff/37001/chrome/renderer/extension... chrome/renderer/extensions/document_custom_bindings.cc:30: content::RenderView* render_view = GetRenderView(); These three lines are no longer necessary. https://codereview.chromium.org/23530029/diff/37001/chrome/renderer/extension... chrome/renderer/extensions/document_custom_bindings.cc:35: WebKit::WebView* web_view = render_view->GetWebView(); These 4 lines are no longer necessary. All the changes in this file can probably be done in a separate CL. Nothing here is controversial.
https://codereview.chromium.org/23530029/diff/37001/chrome/browser/chrome_con... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/23530029/diff/37001/chrome/browser/chrome_con... chrome/browser/chrome_content_browser_client.cc:2351: if (content::HasWebUIScheme(site_url)) On 2013/09/12 22:46:46, Fady Samuel wrote: > We should probably do the same check as above here rather than simply always > allowing BrowserPlugin in WebUI. SupportsBrowserPlugin is called before RenderViewHost is even created, so there is no way we could check the extension ID at this time point. Also, this method is only called to determine whether the RenderViewHost should monitor messages from guests, https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... we have other checks in places that prevents webview from creation outside of the context of a component extension with webview permission declared. https://codereview.chromium.org/23530029/diff/37001/chrome/renderer/extension... File chrome/renderer/extensions/document_custom_bindings.cc (right): https://codereview.chromium.org/23530029/diff/37001/chrome/renderer/extension... chrome/renderer/extensions/document_custom_bindings.cc:11: #include "content/public/renderer/render_view.h" On 2013/09/12 22:46:46, Fady Samuel wrote: > This probably not necessary. Thanks for spotting this! Done. https://codereview.chromium.org/23530029/diff/37001/chrome/renderer/extension... chrome/renderer/extensions/document_custom_bindings.cc:14: #include "third_party/WebKit/public/web/WebView.h" On 2013/09/12 22:46:46, Fady Samuel wrote: > This line might not be necessary. Done. https://codereview.chromium.org/23530029/diff/37001/chrome/renderer/extension... chrome/renderer/extensions/document_custom_bindings.cc:30: content::RenderView* render_view = GetRenderView(); On 2013/09/12 22:46:46, Fady Samuel wrote: > These three lines are no longer necessary. https://codereview.chromium.org/23530029/diff/37001/chrome/renderer/extension... chrome/renderer/extensions/document_custom_bindings.cc:35: WebKit::WebView* web_view = render_view->GetWebView(); On 2013/09/12 22:46:46, Fady Samuel wrote: > These 4 lines are no longer necessary. > > All the changes in this file can probably be done in a separate CL. Nothing here > is controversial. Changes in this file are an integral part of the whole plan to support webview in an iframed extension. Without other changes in this CL, they make no difference, since otherwise a webview embedder must always be the main frame.
On 2013/09/13 00:04:02, guohui wrote: > https://codereview.chromium.org/23530029/diff/37001/chrome/browser/chrome_con... > File chrome/browser/chrome_content_browser_client.cc (right): > > https://codereview.chromium.org/23530029/diff/37001/chrome/browser/chrome_con... > chrome/browser/chrome_content_browser_client.cc:2351: if > (content::HasWebUIScheme(site_url)) > On 2013/09/12 22:46:46, Fady Samuel wrote: > > We should probably do the same check as above here rather than simply always > > allowing BrowserPlugin in WebUI. > SupportsBrowserPlugin is called before RenderViewHost is even created, so there > is no way we could check the extension ID at this time point. > > Also, this method is only called to determine whether the RenderViewHost should > monitor messages from guests, > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > > we have other checks in places that prevents webview from creation outside of > the context of a component extension with webview permission declared. > > https://codereview.chromium.org/23530029/diff/37001/chrome/renderer/extension... > File chrome/renderer/extensions/document_custom_bindings.cc (right): > > https://codereview.chromium.org/23530029/diff/37001/chrome/renderer/extension... > chrome/renderer/extensions/document_custom_bindings.cc:11: #include > "content/public/renderer/render_view.h" > On 2013/09/12 22:46:46, Fady Samuel wrote: > > This probably not necessary. > Thanks for spotting this! > > Done. > > https://codereview.chromium.org/23530029/diff/37001/chrome/renderer/extension... > chrome/renderer/extensions/document_custom_bindings.cc:14: #include > "third_party/WebKit/public/web/WebView.h" > On 2013/09/12 22:46:46, Fady Samuel wrote: > > This line might not be necessary. > > Done. > > https://codereview.chromium.org/23530029/diff/37001/chrome/renderer/extension... > chrome/renderer/extensions/document_custom_bindings.cc:30: content::RenderView* > render_view = GetRenderView(); > On 2013/09/12 22:46:46, Fady Samuel wrote: > > These three lines are no longer necessary. > > https://codereview.chromium.org/23530029/diff/37001/chrome/renderer/extension... > chrome/renderer/extensions/document_custom_bindings.cc:35: WebKit::WebView* > web_view = render_view->GetWebView(); > On 2013/09/12 22:46:46, Fady Samuel wrote: > > These 4 lines are no longer necessary. > > > > All the changes in this file can probably be done in a separate CL. Nothing > here > > is controversial. > > Changes in this file are an integral part of the whole plan to support webview > in an iframed extension. Without other changes in this CL, they make no > difference, since otherwise a webview embedder must always be the main frame. ping?
I'm happy with the parts I've reviewed and understand. This CL needs more scrutiny from Charlie though. LGTM.
Thanks, this looks like it's on the right track. A few questions for how we might lock some aspects of it down further. https://codereview.chromium.org/23530029/diff/42001/chrome/browser/chrome_con... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/23530029/diff/42001/chrome/browser/chrome_con... chrome/browser/chrome_content_browser_client.cc:818: // Only trust |embedder_frame_url| reported by a WebUI renderer. Let's be a little more explicit here. Something like: We usually require BrowserPlugins to be hosted by a storage isolated extension. We treat WebUI pages as a special case if they host the BrowserPlugin in a component extension iframe. In that case, we use the iframe's URL to determine the extension. https://codereview.chromium.org/23530029/diff/42001/chrome/browser/chrome_con... chrome/browser/chrome_content_browser_client.cc:824: if (!extension) { Fady, would it make sense to add || !extension->is_storage_isolated() to this check? (Or perhaps it belongs in SupportsBrowserPlugin?) If not, we might try to create a guest partition inside the default profile directory, which would be very bad. https://codereview.chromium.org/23530029/diff/42001/chrome/browser/chrome_con... chrome/browser/chrome_content_browser_client.cc:2351: if (content::HasWebUIScheme(site_url)) Do we need all WebUI pages to be able to do this, or just particular ones that we might be able to whitelist here? There are enough requirements (e.g., must host in an component extension iframe, extension must be storage isolated) that I'm hesitant to open it up to all WebUI pages. I'm also wondering if it's easy to check the embedder_frame_url here as you've done above, or if that's difficult to pass in here. https://codereview.chromium.org/23530029/diff/42001/chrome/common/extensions/... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/23530029/diff/42001/chrome/common/extensions/... chrome/common/extensions/api/_api_features.json:617: "contexts": ["blessed_extension", "unblessed_extension"] What does unblessed_extension mean, and why do we need to add it? https://codereview.chromium.org/23530029/diff/42001/chrome/renderer/chrome_co... File chrome/renderer/chrome_content_renderer_client.cc (left): https://codereview.chromium.org/23530029/diff/42001/chrome/renderer/chrome_co... chrome/renderer/chrome_content_renderer_client.cc:418: if (!extension_dispatcher_->IsExtensionActive(extension_id)) Why is this being removed? https://codereview.chromium.org/23530029/diff/42001/content/browser/browser_p... File content/browser/browser_plugin/browser_plugin_embedder.cc (right): https://codereview.chromium.org/23530029/diff/42001/content/browser/browser_p... content/browser/browser_plugin/browser_plugin_embedder.cc:190: GURL(params.embedder_frame_url), URLs received from the renderer should be passed through RenderViewHost::FilterURL, to prevent the renderer process from lying to gain more privileges. |empty_allowed| should be false for this context. Look around for examples of validated_params and FilterURL in other files. (Fady, it looks like we may need to do this to BrowserPlugin messages more generally.) https://codereview.chromium.org/23530029/diff/42001/content/browser/web_conte... File content/browser/web_contents/render_view_host_manager.cc (right): https://codereview.chromium.org/23530029/diff/42001/content/browser/web_conte... content/browser/web_contents/render_view_host_manager.cc:703: if (pending_web_ui() && !render_view_host->GetProcess()->IsGuest()) Is this independent of the rest of the CL? Maybe we can land this part first, since it seems like a good check to have. https://codereview.chromium.org/23530029/diff/42001/content/common/browser_pl... File content/common/browser_plugin/browser_plugin_messages.h (right): https://codereview.chromium.org/23530029/diff/42001/content/common/browser_pl... content/common/browser_plugin/browser_plugin_messages.h:72: IPC_STRUCT_MEMBER(std::string, embedder_frame_url) Please use GURL for urls. (I'm not certain, but I think they used string for src because it comes from an attribute on the tag.)
https://codereview.chromium.org/23530029/diff/42001/chrome/browser/chrome_con... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/23530029/diff/42001/chrome/browser/chrome_con... chrome/browser/chrome_content_browser_client.cc:818: // Only trust |embedder_frame_url| reported by a WebUI renderer. On 2013/09/13 21:17:24, creis wrote: > Let's be a little more explicit here. Something like: > > We usually require BrowserPlugins to be hosted by a storage isolated extension. > We treat WebUI pages as a special case if they host the BrowserPlugin in a > component extension iframe. In that case, we use the iframe's URL to determine > the extension. Done. https://codereview.chromium.org/23530029/diff/42001/chrome/browser/chrome_con... chrome/browser/chrome_content_browser_client.cc:824: if (!extension) { On 2013/09/13 21:17:24, creis wrote: > Fady, would it make sense to add || !extension->is_storage_isolated() to this > check? (Or perhaps it belongs in SupportsBrowserPlugin?) > > If not, we might try to create a guest partition inside the default profile > directory, which would be very bad. A webview (browser plugin) always has an isolated storage, regardless of the isolation setting of the embedder ext/app. The partition is put under [profile name]/storage/ext/[ext-id]/[partition-id]. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... Actually, storage isolation setting is only supported for apps, but not for extensions (in the strict sense), so the check for is_stoarge_isolated does not make sense for component extensions. https://codereview.chromium.org/23530029/diff/42001/chrome/browser/chrome_con... chrome/browser/chrome_content_browser_client.cc:2351: if (content::HasWebUIScheme(site_url)) On 2013/09/13 21:17:24, creis wrote: > Do we need all WebUI pages to be able to do this, or just particular ones that > we might be able to whitelist here? There are enough requirements (e.g., must > host in an component extension iframe, extension must be storage isolated) that > I'm hesitant to open it up to all WebUI pages. > > I'm also wondering if it's easy to check the embedder_frame_url here as you've > done above, or if that's difficult to pass in here. As i replied to Fady, SupportsBrowserPlugin is called before RenderViewHost is even created, so there is no way we could get the frame URL at this time point. Also, this method is only called to determine whether the RenderViewHost should monitor messages from guests, we have other checks in places that prevents webview from creation outside of the context of a component extension with webview permission declared. https://codereview.chromium.org/23530029/diff/42001/chrome/common/extensions/... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/23530029/diff/42001/chrome/common/extensions/... chrome/common/extensions/api/_api_features.json:617: "contexts": ["blessed_extension", "unblessed_extension"] On 2013/09/13 21:17:24, creis wrote: > What does unblessed_extension mean, and why do we need to add it? It means a JavaScript context for an inactive extension, that is for an extension loaded through iframe. We need this to support webview in an iframed component extension. https://codereview.chromium.org/23530029/diff/42001/chrome/renderer/chrome_co... File chrome/renderer/chrome_content_renderer_client.cc (left): https://codereview.chromium.org/23530029/diff/42001/chrome/renderer/chrome_co... chrome/renderer/chrome_content_renderer_client.cc:418: if (!extension_dispatcher_->IsExtensionActive(extension_id)) On 2013/09/13 21:17:24, creis wrote: > Why is this being removed? This check requires the extension to be active in the current process, meaning that the extension must be loaded in the top window. We need to remove this check to support webview in an iframed extension. Also this method is only called to check for browser plugin support, thus it should have no side effect. https://codereview.chromium.org/23530029/diff/42001/content/browser/browser_p... File content/browser/browser_plugin/browser_plugin_embedder.cc (right): https://codereview.chromium.org/23530029/diff/42001/content/browser/browser_p... content/browser/browser_plugin/browser_plugin_embedder.cc:190: GURL(params.embedder_frame_url), On 2013/09/13 21:17:24, creis wrote: > URLs received from the renderer should be passed through > RenderViewHost::FilterURL, to prevent the renderer process from lying to gain > more privileges. |empty_allowed| should be false for this context. > > Look around for examples of validated_params and FilterURL in other files. > (Fady, it looks like we may need to do this to BrowserPlugin messages more > generally.) Done. https://codereview.chromium.org/23530029/diff/42001/content/browser/web_conte... File content/browser/web_contents/render_view_host_manager.cc (right): https://codereview.chromium.org/23530029/diff/42001/content/browser/web_conte... content/browser/web_contents/render_view_host_manager.cc:703: if (pending_web_ui() && !render_view_host->GetProcess()->IsGuest()) On 2013/09/13 21:17:24, creis wrote: > Is this independent of the rest of the CL? Maybe we can land this part first, > since it seems like a good check to have. yes it is, though without other changes in this CL, the check is never hit. https://codereview.chromium.org/23530029/diff/42001/content/common/browser_pl... File content/common/browser_plugin/browser_plugin_messages.h (right): https://codereview.chromium.org/23530029/diff/42001/content/common/browser_pl... content/common/browser_plugin/browser_plugin_messages.h:72: IPC_STRUCT_MEMBER(std::string, embedder_frame_url) On 2013/09/13 21:17:24, creis wrote: > Please use GURL for urls. (I'm not certain, but I think they used string for > src because it comes from an attribute on the tag.) Done.
Thanks for the updates! Fady and I met yesterday to think carefully about what needs to be done to support this, and you've already handled a lot of it. Much of the remaining work is making sure that this new support won't be misused or vulnerable if it's used in other ways in the future. Specific feedback below. https://codereview.chromium.org/23530029/diff/42001/chrome/browser/chrome_con... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/23530029/diff/42001/chrome/browser/chrome_con... chrome/browser/chrome_content_browser_client.cc:824: if (!extension) { On 2013/09/16 22:27:02, guohui wrote: > On 2013/09/13 21:17:24, creis wrote: > > Fady, would it make sense to add || !extension->is_storage_isolated() to this > > check? (Or perhaps it belongs in SupportsBrowserPlugin?) > > > > If not, we might try to create a guest partition inside the default profile > > directory, which would be very bad. > A webview (browser plugin) always has an isolated storage, regardless of the > isolation setting of the embedder ext/app. The partition is put under [profile > name]/storage/ext/[ext-id]/[partition-id]. > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... > > Actually, storage isolation setting is only supported for apps, but not for > extensions (in the strict sense), so the check for is_stoarge_isolated does not > make sense for component extensions. Ok. Fady and I talked about this and skipping this check should be ok as long as we explicitly support creating storage/ext/[ext-id] directories for non-storage-isolated extensions when they can host guests. This requires tests and making sure that the directory won't be deleted on cleanup. See http://crbug.com/293722. https://codereview.chromium.org/23530029/diff/42001/chrome/browser/chrome_con... chrome/browser/chrome_content_browser_client.cc:2351: if (content::HasWebUIScheme(site_url)) On 2013/09/16 22:27:02, guohui wrote: > On 2013/09/13 21:17:24, creis wrote: > > Do we need all WebUI pages to be able to do this, or just particular ones that > > we might be able to whitelist here? There are enough requirements (e.g., must > > host in an component extension iframe, extension must be storage isolated) > that > > I'm hesitant to open it up to all WebUI pages. > > > > I'm also wondering if it's easy to check the embedder_frame_url here as you've > > done above, or if that's difficult to pass in here. > > As i replied to Fady, SupportsBrowserPlugin is called before RenderViewHost is > even created, so there is no way we could get the frame URL at this time point. Ok, let's skip the frame URL check. > Also, this method is only called to determine whether the RenderViewHost should > monitor messages from guests, we have other checks in places that prevents > webview from creation outside of the context of a component extension with > webview permission declared. Where are those checks? Can we limit the support to particular WebUI URLs, or is it necessary to support webviews in all WebUI processes? I'd prefer to lock it down to a whitelist if possible, since not all WebUI pages will have the constraints you're relying on. More general support will be possible with out-of-process iframes. https://codereview.chromium.org/23530029/diff/42001/chrome/common/extensions/... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/23530029/diff/42001/chrome/common/extensions/... chrome/common/extensions/api/_api_features.json:617: "contexts": ["blessed_extension", "unblessed_extension"] On 2013/09/16 22:27:02, guohui wrote: > On 2013/09/13 21:17:24, creis wrote: > > What does unblessed_extension mean, and why do we need to add it? > > It means a JavaScript context for an inactive extension, that is for an > extension loaded through iframe. We need this to support webview in an iframed > component extension. Ok. Fady and I talked about this and we think it's safe because the API calls involve a check to see if the embedder has access to the guest it's trying to access. Still, this requires a security audit to make sure that normal web renderer processes can't abuse this API to cause harm, as well as some safeguards to ensure that new APIs won't pose a risk in the future. See http://crbug.com/293724. https://codereview.chromium.org/23530029/diff/42001/chrome/renderer/chrome_co... File chrome/renderer/chrome_content_renderer_client.cc (left): https://codereview.chromium.org/23530029/diff/42001/chrome/renderer/chrome_co... chrome/renderer/chrome_content_renderer_client.cc:418: if (!extension_dispatcher_->IsExtensionActive(extension_id)) On 2013/09/16 22:27:02, guohui wrote: > On 2013/09/13 21:17:24, creis wrote: > > Why is this being removed? > > This check requires the extension to be active in the current process, meaning > that the extension must be loaded in the top window. We need to remove this > check to support webview in an iframed extension. > > Also this method is only called to check for browser plugin support, thus it > should have no side effect. This is a very general sounding method (GetExtension), so it has implications for future callers, not just the current ones. To remove any ambiguity about what it does, let's rename it to GetExtensionByOrigin, and let's add a comment to the declaration saying that it doesn't require the current process to be an extension process. https://codereview.chromium.org/23530029/diff/42001/content/browser/browser_p... File content/browser/browser_plugin/browser_plugin_embedder.cc (right): https://codereview.chromium.org/23530029/diff/42001/content/browser/browser_p... content/browser/browser_plugin/browser_plugin_embedder.cc:190: GURL(params.embedder_frame_url), On 2013/09/16 22:27:02, guohui wrote: > On 2013/09/13 21:17:24, creis wrote: > > URLs received from the renderer should be passed through > > RenderViewHost::FilterURL, to prevent the renderer process from lying to gain > > more privileges. |empty_allowed| should be false for this context. > > > > Look around for examples of validated_params and FilterURL in other files. > > (Fady, it looks like we may need to do this to BrowserPlugin messages more > > generally.) > > Done. Great, thanks! https://codereview.chromium.org/23530029/diff/42001/content/browser/web_conte... File content/browser/web_contents/render_view_host_manager.cc (right): https://codereview.chromium.org/23530029/diff/42001/content/browser/web_conte... content/browser/web_contents/render_view_host_manager.cc:703: if (pending_web_ui() && !render_view_host->GetProcess()->IsGuest()) On 2013/09/16 22:27:02, guohui wrote: > On 2013/09/13 21:17:24, creis wrote: > > Is this independent of the rest of the CL? Maybe we can land this part first, > > since it seems like a good check to have. > > yes it is, though without other changes in this CL, the check is never hit. It's still a useful defense-in-depth check, so I think it would be better to have it landed separately from the rest in case we need to revert some other part of the CL. I'm happy to quickly approve this part in its own CL.
https://codereview.chromium.org/23530029/diff/42001/chrome/browser/chrome_con... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/23530029/diff/42001/chrome/browser/chrome_con... chrome/browser/chrome_content_browser_client.cc:824: if (!extension) { On 2013/09/18 17:42:23, creis wrote: > On 2013/09/16 22:27:02, guohui wrote: > > On 2013/09/13 21:17:24, creis wrote: > > > Fady, would it make sense to add || !extension->is_storage_isolated() to > this > > > check? (Or perhaps it belongs in SupportsBrowserPlugin?) > > > > > > If not, we might try to create a guest partition inside the default profile > > > directory, which would be very bad. > > A webview (browser plugin) always has an isolated storage, regardless of the > > isolation setting of the embedder ext/app. The partition is put under [profile > > name]/storage/ext/[ext-id]/[partition-id]. > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... > > > > Actually, storage isolation setting is only supported for apps, but not for > > extensions (in the strict sense), so the check for is_stoarge_isolated does > not > > make sense for component extensions. > > Ok. Fady and I talked about this and skipping this check should be ok as long > as we explicitly support creating storage/ext/[ext-id] directories for > non-storage-isolated extensions when they can host guests. This requires tests > and making sure that the directory won't be deleted on cleanup. See > http://crbug.com/293722. Can we address this issue in a separate CL? https://codereview.chromium.org/23530029/diff/42001/chrome/browser/chrome_con... chrome/browser/chrome_content_browser_client.cc:2351: if (content::HasWebUIScheme(site_url)) On 2013/09/18 17:42:23, creis wrote: > On 2013/09/16 22:27:02, guohui wrote: > > On 2013/09/13 21:17:24, creis wrote: > > > Do we need all WebUI pages to be able to do this, or just particular ones > that > > > we might be able to whitelist here? There are enough requirements (e.g., > must > > > host in an component extension iframe, extension must be storage isolated) > > that > > > I'm hesitant to open it up to all WebUI pages. > > > > > > I'm also wondering if it's easy to check the embedder_frame_url here as > you've > > > done above, or if that's difficult to pass in here. > > > > As i replied to Fady, SupportsBrowserPlugin is called before RenderViewHost is > > even created, so there is no way we could get the frame URL at this time > point. > > Ok, let's skip the frame URL check. > > > Also, this method is only called to determine whether the RenderViewHost > should > > monitor messages from guests, we have other checks in places that prevents > > webview from creation outside of the context of a component extension with > > webview permission declared. > > > Where are those checks? Can we limit the support to particular WebUI URLs, or > is it necessary to support webviews in all WebUI processes? I'd prefer to lock > it down to a whitelist if possible, since not all WebUI pages will have the > constraints you're relying on. More general support will be possible with > out-of-process iframes. I prefer to allows it for all WebUI URLs if possible. The reason is we want to embed the gaia auth extension in any WebUI that needs to trigger inline signin flow. To avoid unwanted use until general support is available, i limited the feature to whitelisted component extensions only in the CL 23530029. https://codereview.chromium.org/23530029/diff/42001/chrome/common/extensions/... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/23530029/diff/42001/chrome/common/extensions/... chrome/common/extensions/api/_api_features.json:617: "contexts": ["blessed_extension", "unblessed_extension"] On 2013/09/18 17:42:23, creis wrote: > On 2013/09/16 22:27:02, guohui wrote: > > On 2013/09/13 21:17:24, creis wrote: > > > What does unblessed_extension mean, and why do we need to add it? > > > > It means a JavaScript context for an inactive extension, that is for an > > extension loaded through iframe. We need this to support webview in an iframed > > component extension. > > Ok. Fady and I talked about this and we think it's safe because the API calls > involve a check to see if the embedder has access to the guest it's trying to > access. Still, this requires a security audit to make sure that normal web > renderer processes can't abuse this API to cause harm, as well as some > safeguards to ensure that new APIs won't pose a risk in the future. See > http://crbug.com/293724. how should we proceed with the security audit? Do we need to block this CL on the audit? https://codereview.chromium.org/23530029/diff/42001/chrome/renderer/chrome_co... File chrome/renderer/chrome_content_renderer_client.cc (left): https://codereview.chromium.org/23530029/diff/42001/chrome/renderer/chrome_co... chrome/renderer/chrome_content_renderer_client.cc:418: if (!extension_dispatcher_->IsExtensionActive(extension_id)) On 2013/09/18 17:42:23, creis wrote: > On 2013/09/16 22:27:02, guohui wrote: > > On 2013/09/13 21:17:24, creis wrote: > > > Why is this being removed? > > > > This check requires the extension to be active in the current process, meaning > > that the extension must be loaded in the top window. We need to remove this > > check to support webview in an iframed extension. > > > > Also this method is only called to check for browser plugin support, thus it > > should have no side effect. > > This is a very general sounding method (GetExtension), so it has implications > for future callers, not just the current ones. To remove any ambiguity about > what it does, let's rename it to GetExtensionByOrigin, and let's add a comment > to the declaration saying that it doesn't require the current process to be an > extension process. Done. https://codereview.chromium.org/23530029/diff/42001/content/browser/browser_p... File content/browser/browser_plugin/browser_plugin_embedder.cc (right): https://codereview.chromium.org/23530029/diff/42001/content/browser/browser_p... content/browser/browser_plugin/browser_plugin_embedder.cc:190: GURL(params.embedder_frame_url), On 2013/09/18 17:42:23, creis wrote: > On 2013/09/16 22:27:02, guohui wrote: > > On 2013/09/13 21:17:24, creis wrote: > > > URLs received from the renderer should be passed through > > > RenderViewHost::FilterURL, to prevent the renderer process from lying to > gain > > > more privileges. |empty_allowed| should be false for this context. > > > > > > Look around for examples of validated_params and FilterURL in other files. > > > (Fady, it looks like we may need to do this to BrowserPlugin messages more > > > generally.) > > > > Done. > > Great, thanks! Done. https://codereview.chromium.org/23530029/diff/42001/content/browser/web_conte... File content/browser/web_contents/render_view_host_manager.cc (right): https://codereview.chromium.org/23530029/diff/42001/content/browser/web_conte... content/browser/web_contents/render_view_host_manager.cc:703: if (pending_web_ui() && !render_view_host->GetProcess()->IsGuest()) On 2013/09/18 17:42:23, creis wrote: > On 2013/09/16 22:27:02, guohui wrote: > > On 2013/09/13 21:17:24, creis wrote: > > > Is this independent of the rest of the CL? Maybe we can land this part > first, > > > since it seems like a good check to have. > > > > yes it is, though without other changes in this CL, the check is never hit. > > It's still a useful defense-in-depth check, so I think it would be better to > have it landed separately from the rest in case we need to revert some other > part of the CL. I'm happy to quickly approve this part in its own CL. Done.
Looks good apart from the security review for making the webview API unblessed. Is there any way to test that the new support works? https://codereview.chromium.org/23530029/diff/42001/chrome/browser/chrome_con... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/23530029/diff/42001/chrome/browser/chrome_con... chrome/browser/chrome_content_browser_client.cc:824: if (!extension) { On 2013/09/19 13:48:41, guohui wrote: > On 2013/09/18 17:42:23, creis wrote: > > On 2013/09/16 22:27:02, guohui wrote: > > > On 2013/09/13 21:17:24, creis wrote: > > > > Fady, would it make sense to add || !extension->is_storage_isolated() to > > this > > > > check? (Or perhaps it belongs in SupportsBrowserPlugin?) > > > > > > > > If not, we might try to create a guest partition inside the default > profile > > > > directory, which would be very bad. > > > A webview (browser plugin) always has an isolated storage, regardless of the > > > isolation setting of the embedder ext/app. The partition is put under > [profile > > > name]/storage/ext/[ext-id]/[partition-id]. > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... > > > > > > Actually, storage isolation setting is only supported for apps, but not for > > > extensions (in the strict sense), so the check for is_stoarge_isolated does > > not > > > make sense for component extensions. > > > > Ok. Fady and I talked about this and skipping this check should be ok as long > > as we explicitly support creating storage/ext/[ext-id] directories for > > non-storage-isolated extensions when they can host guests. This requires > tests > > and making sure that the directory won't be deleted on cleanup. See > > http://crbug.com/293722. > Can we address this issue in a separate CL? Yes. As we discussed on https://codereview.chromium.org/23653012/, the StoragePartition management is a prerequisite to support webviews in WebUI and component extensions in general. As long as your component extension is the only one on the whitelist and you're ok with the risk of bugs, we can proceed with this independently of the fix for http://crbug.com/293722. https://codereview.chromium.org/23530029/diff/42001/chrome/browser/chrome_con... chrome/browser/chrome_content_browser_client.cc:2351: if (content::HasWebUIScheme(site_url)) On 2013/09/19 13:48:41, guohui wrote: > On 2013/09/18 17:42:23, creis wrote: > > On 2013/09/16 22:27:02, guohui wrote: > > > On 2013/09/13 21:17:24, creis wrote: > > > > Do we need all WebUI pages to be able to do this, or just particular ones > > that > > > > we might be able to whitelist here? There are enough requirements (e.g., > > must > > > > host in an component extension iframe, extension must be storage isolated) > > > that > > > > I'm hesitant to open it up to all WebUI pages. > > > > > > > > I'm also wondering if it's easy to check the embedder_frame_url here as > > you've > > > > done above, or if that's difficult to pass in here. > > > > > > As i replied to Fady, SupportsBrowserPlugin is called before RenderViewHost > is > > > even created, so there is no way we could get the frame URL at this time > > point. > > > > Ok, let's skip the frame URL check. > > > > > Also, this method is only called to determine whether the RenderViewHost > > should > > > monitor messages from guests, we have other checks in places that prevents > > > webview from creation outside of the context of a component extension with > > > webview permission declared. > > > > > > Where are those checks? Can we limit the support to particular WebUI URLs, or > > is it necessary to support webviews in all WebUI processes? I'd prefer to > lock > > it down to a whitelist if possible, since not all WebUI pages will have the > > constraints you're relying on. More general support will be possible with > > out-of-process iframes. > I prefer to allows it for all WebUI URLs if possible. The reason is we want to > embed the gaia auth extension in any WebUI that needs to trigger inline signin > flow. To avoid unwanted use until general support is available, i limited the > feature to whitelisted component extensions only in the CL 23530029. Ok. The whitelist in CL 23653012 should be sufficient. https://codereview.chromium.org/23530029/diff/42001/chrome/common/extensions/... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/23530029/diff/42001/chrome/common/extensions/... chrome/common/extensions/api/_api_features.json:617: "contexts": ["blessed_extension", "unblessed_extension"] On 2013/09/19 13:48:41, guohui wrote: > On 2013/09/18 17:42:23, creis wrote: > > On 2013/09/16 22:27:02, guohui wrote: > > > On 2013/09/13 21:17:24, creis wrote: > > > > What does unblessed_extension mean, and why do we need to add it? > > > > > > It means a JavaScript context for an inactive extension, that is for an > > > extension loaded through iframe. We need this to support webview in an > iframed > > > component extension. > > > > Ok. Fady and I talked about this and we think it's safe because the API calls > > involve a check to see if the embedder has access to the guest it's trying to > > access. Still, this requires a security audit to make sure that normal web > > renderer processes can't abuse this API to cause harm, as well as some > > safeguards to ensure that new APIs won't pose a risk in the future. See > > http://crbug.com/293724. > how should we proceed with the security audit? Do we need to block this CL on > the audit? I'll need to look over the existing webview API with another security team member to make sure it can't be abused. I'll try to schedule some time for it. Unfortunately, yes, this is a blocking issue for this CL. We cannot make the API unblessed until we know that it can't be abused by an exploited web renderer process.
>
> I'll need to look over the existing webview API with another security team
> member to make sure it can't be abused. I'll try to schedule some time for
it.
>
> Unfortunately, yes, this is a blocking issue for this CL. We cannot make the
> API unblessed until we know that it can't be abused by an exploited web
renderer
> process.
All chrome.webview.* can be found here.
Notice the pattern here is
WebViewGuest* guest = WebViewGuest::From(
render_view_host()->GetProcess()->GetID(), params->instance_id);
if (!guest)
return false;
The actual operations are all in WebViewGuest. If we don't have access to a
WebViewGuest,
then we can't do the operation. WebViewGuests are keyed on the Process ID, and
so even if
the process lies about the instance ID, it won't be able to get to a guest it
does not own.
If a given instance ID is not a <webview> (it's an <adview>, for example),
WebViewGuest::From will
return NULL.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
https://codereview.chromium.org/23530029/diff/42001/chrome/browser/chrome_con... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/23530029/diff/42001/chrome/browser/chrome_con... chrome/browser/chrome_content_browser_client.cc:824: if (!extension) { On 2013/09/19 17:21:22, creis wrote: > On 2013/09/19 13:48:41, guohui wrote: > > On 2013/09/18 17:42:23, creis wrote: > > > On 2013/09/16 22:27:02, guohui wrote: > > > > On 2013/09/13 21:17:24, creis wrote: > > > > > Fady, would it make sense to add || !extension->is_storage_isolated() to > > > this > > > > > check? (Or perhaps it belongs in SupportsBrowserPlugin?) > > > > > > > > > > If not, we might try to create a guest partition inside the default > > profile > > > > > directory, which would be very bad. > > > > A webview (browser plugin) always has an isolated storage, regardless of > the > > > > isolation setting of the embedder ext/app. The partition is put under > > [profile > > > > name]/storage/ext/[ext-id]/[partition-id]. > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... > > > > > > > > Actually, storage isolation setting is only supported for apps, but not > for > > > > extensions (in the strict sense), so the check for is_stoarge_isolated > does > > > not > > > > make sense for component extensions. > > > > > > Ok. Fady and I talked about this and skipping this check should be ok as > long > > > as we explicitly support creating storage/ext/[ext-id] directories for > > > non-storage-isolated extensions when they can host guests. This requires > > tests > > > and making sure that the directory won't be deleted on cleanup. See > > > http://crbug.com/293722. > > Can we address this issue in a separate CL? > > Yes. As we discussed on https://codereview.chromium.org/23653012/, the > StoragePartition management is a prerequisite to support webviews in WebUI and > component extensions in general. As long as your component extension is the > only one on the whitelist and you're ok with the risk of bugs, we can proceed > with this independently of the fix for http://crbug.com/293722. Done. https://codereview.chromium.org/23530029/diff/42001/chrome/browser/chrome_con... chrome/browser/chrome_content_browser_client.cc:2351: if (content::HasWebUIScheme(site_url)) On 2013/09/19 17:21:22, creis wrote: > On 2013/09/19 13:48:41, guohui wrote: > > On 2013/09/18 17:42:23, creis wrote: > > > On 2013/09/16 22:27:02, guohui wrote: > > > > On 2013/09/13 21:17:24, creis wrote: > > > > > Do we need all WebUI pages to be able to do this, or just particular > ones > > > that > > > > > we might be able to whitelist here? There are enough requirements > (e.g., > > > must > > > > > host in an component extension iframe, extension must be storage > isolated) > > > > that > > > > > I'm hesitant to open it up to all WebUI pages. > > > > > > > > > > I'm also wondering if it's easy to check the embedder_frame_url here as > > > you've > > > > > done above, or if that's difficult to pass in here. > > > > > > > > As i replied to Fady, SupportsBrowserPlugin is called before > RenderViewHost > > is > > > > even created, so there is no way we could get the frame URL at this time > > > point. > > > > > > Ok, let's skip the frame URL check. > > > > > > > Also, this method is only called to determine whether the RenderViewHost > > > should > > > > monitor messages from guests, we have other checks in places that prevents > > > > webview from creation outside of the context of a component extension with > > > > webview permission declared. > > > > > > > > > Where are those checks? Can we limit the support to particular WebUI URLs, > or > > > is it necessary to support webviews in all WebUI processes? I'd prefer to > > lock > > > it down to a whitelist if possible, since not all WebUI pages will have the > > > constraints you're relying on. More general support will be possible with > > > out-of-process iframes. > > I prefer to allows it for all WebUI URLs if possible. The reason is we want to > > embed the gaia auth extension in any WebUI that needs to trigger inline signin > > flow. To avoid unwanted use until general support is available, i limited the > > feature to whitelisted component extensions only in the CL 23530029. > > Ok. The whitelist in CL 23653012 should be sufficient. Done. https://codereview.chromium.org/23530029/diff/42001/chrome/common/extensions/... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/23530029/diff/42001/chrome/common/extensions/... chrome/common/extensions/api/_api_features.json:617: "contexts": ["blessed_extension", "unblessed_extension"] On 2013/09/19 17:21:22, creis wrote: > On 2013/09/19 13:48:41, guohui wrote: > > On 2013/09/18 17:42:23, creis wrote: > > > On 2013/09/16 22:27:02, guohui wrote: > > > > On 2013/09/13 21:17:24, creis wrote: > > > > > What does unblessed_extension mean, and why do we need to add it? > > > > > > > > It means a JavaScript context for an inactive extension, that is for an > > > > extension loaded through iframe. We need this to support webview in an > > iframed > > > > component extension. > > > > > > Ok. Fady and I talked about this and we think it's safe because the API > calls > > > involve a check to see if the embedder has access to the guest it's trying > to > > > access. Still, this requires a security audit to make sure that normal web > > > renderer processes can't abuse this API to cause harm, as well as some > > > safeguards to ensure that new APIs won't pose a risk in the future. See > > > http://crbug.com/293724. > > how should we proceed with the security audit? Do we need to block this CL on > > the audit? > > I'll need to look over the existing webview API with another security team > member to make sure it can't be abused. I'll try to schedule some time for it. > > Unfortunately, yes, this is a blocking issue for this CL. We cannot make the > API unblessed until we know that it can't be abused by an exploited web renderer > process. As suggested by Fady, i ll refactor this part into a separate CL.
On 2013/09/19 17:59:11, guohui wrote: > As suggested by Fady, i ll refactor this part into a separate CL. Interesting. So what would the implication be for landing the rest of this without the unblessed extension part? Would it let you could create webviews in WebUI but not use the webview APIs? I suppose that would be safe, though I'm not sure whether it's useful. In particular, how would you test that this is working? I suppose we could delay writing a test until https://codereview.chromium.org/24243007/, but eventually we should have something testing that these combined changes allow webviews in WebUI.
Hi Charlie, We're breaking up the overall change into many CLs to make it easier to understand and review. If the feature does not work until all CLs are committed, I think that fine. We just need to make sure we don't break existing functionality or introduce security holes in the mean time. Assuming nothing is broken in the mean time, is there any reason to hold this CL back?
On 2013/09/19 23:26:07, Roger Tawa wrote: > Hi Charlie, > > We're breaking up the overall change into many CLs to make it easier to > understand and review. If the feature does not work until all CLs are > committed, I think that fine. We just need to make sure we don't break existing > functionality or introduce security holes in the mean time. Thanks for explaining. > Assuming nothing is broken in the mean time, is there any reason to hold this CL > back? To be clear, I'm not trying to hold things up arbitrarily. As you mentioned, I'm trying to make sure that we don't expose security issues and bugs by using a feature in a way it's not yet ready to be used. (That was why Fady and I filed the bugs that are marked as blocking 285151.) I think this CL has reached a state that it won't cause problems, and we can proceed with it. My remaining question from my last two comments is how and when we're going to test this. (This is an important enough functional change that it will need to be tested, though I'm fine with that happening in the next CL as long as we know what the plan is.)
On 2013/09/20 00:07:10, creis wrote: > On 2013/09/19 23:26:07, Roger Tawa wrote: > > Hi Charlie, > > > > We're breaking up the overall change into many CLs to make it easier to > > understand and review. If the feature does not work until all CLs are > > committed, I think that fine. We just need to make sure we don't break > existing > > functionality or introduce security holes in the mean time. > > Thanks for explaining. > > > Assuming nothing is broken in the mean time, is there any reason to hold this > CL > > back? > > To be clear, I'm not trying to hold things up arbitrarily. As you mentioned, > I'm trying to make sure that we don't expose security issues and bugs by using a > feature in a way it's not yet ready to be used. (That was why Fady and I filed > the bugs that are marked as blocking 285151.) > > I think this CL has reached a state that it won't cause problems, and we can > proceed with it. My remaining question from my last two comments is how and > when we're going to test this. (This is an important enough functional change > that it will need to be tested, though I'm fine with that happening in the next > CL as long as we know what the plan is.) my plan is to add browser tests for the new use cases in web_view_browsertest.cc, though it has to be handled in a separate CL once all CLs are committed. Tracked in crbug/295243.
On 2013/09/20 00:43:12, guohui wrote: > my plan is to add browser tests for the new use cases in > web_view_browsertest.cc, though it has to be handled in a separate CL once all > CLs are committed. Tracked in crbug/295243. Ok. Thanks for filing the test bug. LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/guohui@chromium.org/23530029/85001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
+jochen, owner review for chrome/renderer/... +cdn, IPC audit for content/common/browser_plugin/browser_plugin_messages.h
IPC changes LGTM
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/guohui@chromium.org/23530029/85001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/guohui@chromium.org/23530029/111001
Failed to trigger a try job on win_rel HTTP Error 400: Bad Request
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/guohui@chromium.org/23530029/131001
Retried try job too often on win7_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/guohui@chromium.org/23530029/131001
Message was sent while issue was closed.
Committed patchset #11 manually as r225014 (presubmit successful). |
