Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23018005:
Start renderer in ContentViewCoreImpl::EvaluateJavaScript. (Closed)
|
Created:
7 years, 4 months ago by michaelbai Modified:
7 years, 4 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionStart renderer in ContentViewCoreImpl::EvaluateJavaScript.
In WebView, the app could evaluate the Javascript before loading any page. This patch start renderer if there is no existing one.
Since renderer was created without SiteInstance, the RenderViewCreated callback will not be called, this patch also call it even without SiteInstance.
BUG=273164
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=219111
Patch Set 1 #Patch Set 2 : really fix issue #Patch Set 3 : #
Total comments: 7
Patch Set 4 : Address comments #
Total comments: 11
Patch Set 5 : Address comments #
Total comments: 3
Patch Set 6 : Address comments #
Messages
Total messages: 29 (0 generated)
lgtm can we add a test for this? (suggest separate patch to keep merge simpler) Thanks! On 14 August 2013 13:29, <michaelbai@chromium.org> wrote: > Reviewers: joth, Yaron, benm, > > Description: > Start renderer in ContentViewCoreImpl::**EvaluateJavaScript. > > In WebView, the app could evaluate the Javascript before loading > any page. This patch start renderer if there is no existing one. > > BUG=273164 > > Please review this at https://codereview.chromium.**org/23018005/<https://codereview.chromium.org/2... > > SVN Base: https://chromium.googlesource.**com/chromium/src.git@master<https://chromium.... > > Affected files: > M content/browser/android/**content_view_core_impl.cc > > > Index: content/browser/android/**content_view_core_impl.cc > diff --git a/content/browser/android/**content_view_core_impl.cc > b/content/browser/android/**content_view_core_impl.cc > index 80fe628dd022781a85a0bd56fe22b3**2d2d270490..** > d40f9122b45bf6708eafb9b75a0514**c0849145b5 100644 > --- a/content/browser/android/**content_view_core_impl.cc > +++ b/content/browser/android/**content_view_core_impl.cc > @@ -1473,13 +1473,22 @@ void ContentViewCoreImpl::**EvaluateJavaScript(JNIEnv* > env, > jobject obj, > jstring script, > jobject callback) { > - RenderViewHost* host = web_contents_->**GetRenderViewHost(); > - DCHECK(host); > + RenderViewHostImpl* rvh = > + static_cast<**RenderViewHostImpl*>(web_** > contents_->GetRenderViewHost()**); > + DCHECK(rvh); > + > + if (!rvh->IsRenderViewLive()) { > + if (!rvh->CreateRenderView(**string16(), rvh->GetRoutingID(), -1)) { > + LOG(ERROR) << > + "Falied to create RenderView in EvaluateJavaScript"; > + return; > + } > + } > > if (!callback) { > // No callback requested. > - host->**ExecuteJavascriptInWebFrame(**string16(), // frame_xpath > - ConvertJavaStringToUTF16(env, > script)); > + rvh->**ExecuteJavascriptInWebFrame(**string16(), // frame_xpath > + ConvertJavaStringToUTF16(env, > script)); > return; > } > > @@ -1490,7 +1499,7 @@ void ContentViewCoreImpl::**EvaluateJavaScript(JNIEnv* > env, > content::RenderViewHost::**JavascriptResultCallback c_callback = > base::Bind(&**JavaScriptResultCallback, j_callback); > > - host->**ExecuteJavascriptInWebFrameCal**lbackResult( > + rvh->**ExecuteJavascriptInWebFrameCal**lbackResult( > string16(), // frame_xpath > ConvertJavaStringToUTF16(env, script), > c_callback); > > >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/23018005/1
On 2013/08/14 21:46:14, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/michaelbai%40chromium.org/23018005/1 Should this be somewhere in aw/? In chrome, you should have a renderer in order to have a CVC
On 2013/08/14 22:03:33, Yaron wrote: > On 2013/08/14 21:46:14, I haz the power (commit-bot) wrote: > > CQ is trying da patch. Follow status at > > https://chromium-status.appspot.com/cq/michaelbai%2540chromium.org/23018005/1 > > Should this be somewhere in aw/? In chrome, you should have a renderer in order > to have a CVC In theory, ContentViewCore could exist without renderer which will only be created when browser navigates to something; so ContentViewCore.EvaluateJavaScript() could be called without renderer.
On 14 August 2013 15:03, <yfriedman@chromium.org> wrote: > On 2013/08/14 21:46:14, I haz the power (commit-bot) wrote: > >> CQ is trying da patch. Follow status at >> https://chromium-status.**appspot.com/cq/michaelbai%** >> 40chromium.org/23018005/1<https://chromium-status.appspot.com/cq/michaelbai%40chromium.org/23018005/1> >> > > Should this be somewhere in aw/? In chrome, you should have a renderer in > order > to have a CVC > > not really - chrome could[1] create a CVC instance and delay calling loadURL on it, and in that intervening time it will existing with no renderer. (It will have a WebContents, but that does not mean it has a RVH yet) [1] "could" as in, might not do this in practice today for main tab CVC instances, but might do for some other usage we haven't thought of yet. Or T&C screen, help screen, etc etc. > https://chromiumcodereview.**appspot.com/23018005/<https://chromiumcodereview... >
Right, I meant that in practice we don't have that now and it would arguably be improper usage of CVC if you did.
The alternative of doing this entirely in aw/ means adding new public API to "start a render process for this webcontents'" or whatever, and that sounds a non-starter. middle ground: add an overload to CVC along the lines of EvaluateJavaScriptEvenIfNotYetNavigated() (or express this via an additional parameter) and use that from AwContents. On 14 August 2013 16:20, <yfriedman@chromium.org> wrote: > Right, I meant that in practice we don't have that now and it would > arguably be > improper usage of CVC if you did. > > https://chromiumcodereview.**appspot.com/23018005/<https://chromiumcodereview... >
On 2013/08/14 23:25:55, joth wrote: > The alternative of doing this entirely in aw/ means adding new public API > to "start a render process for this webcontents'" or whatever, and that > sounds a non-starter. > > middle ground: add an overload to CVC along the lines of > EvaluateJavaScriptEvenIfNotYetNavigated() > (or express this via an additional parameter) and use that from AwContents. > > > > > On 14 August 2013 16:20, <mailto:yfriedman@chromium.org> wrote: > > > Right, I meant that in practice we don't have that now and it would > > arguably be > > improper usage of CVC if you did. > > > > > https://chromiumcodereview.**appspot.com/23018005/%3Chttps://chromiumcoderevi...> > > OK, I will add a new method.
Need to create the renderer from WebContentsImpl, also call the observer for RenderViewCreated() https://codereview.chromium.org/23018005/diff/30001/content/browser/web_conte... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/23018005/diff/30001/content/browser/web_conte... content/browser/web_contents/web_contents_impl.cc:2871: WebContentsObserver, observers_, RenderViewCreated(render_view_host)); I think this might reasonable, since the RenderViewReady() will be called even for the renderer without NavigationEntry. It might be weird for observer to get RenderViewReady callback but no RenderViewCreated.
Wasn't quite clear if this is ready for review? https://codereview.chromium.org/23018005/diff/30001/content/browser/android/c... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/23018005/diff/30001/content/browser/android/c... content/browser/android/content_view_core_impl.cc:1482: static_cast<WebContentsImpl*>(web_contents_); just use web_contents_ -- it's already of type WebContentsImpl* https://codereview.chromium.org/23018005/diff/30001/content/browser/android/c... content/browser/android/content_view_core_impl.cc:1484: if (!web_contents_impl->CreateRenderView(rvh, MSG_ROUTING_NONE, -1)) { this looks kind of complex, and the change in WebContentsImpl I don't really understand. would it work to call CreateSwappedOutRenderView() here? This would give almost the same callpath, but the RenderViewHostManager would then also be involved. (It sounds like it should be in the know about this) https://codereview.chromium.org/23018005/diff/30001/content/browser/web_conte... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/23018005/diff/30001/content/browser/web_conte... content/browser/web_contents/web_contents_impl.cc:2874: style nit: I'd put "if (entry)" around all this code from 2875 - 2887 rather than have the early-out above. This way you can share the FOR_EACH_OBSERVER block
PTAL https://codereview.chromium.org/23018005/diff/30001/content/browser/android/c... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/23018005/diff/30001/content/browser/android/c... content/browser/android/content_view_core_impl.cc:1482: static_cast<WebContentsImpl*>(web_contents_); On 2013/08/20 05:00:44, joth wrote: > just use web_contents_ -- it's already of type WebContentsImpl* Done. https://codereview.chromium.org/23018005/diff/30001/content/browser/android/c... content/browser/android/content_view_core_impl.cc:1484: if (!web_contents_impl->CreateRenderView(rvh, MSG_ROUTING_NONE, -1)) { Tried, it didn't work, it seemed that a swapped out RenderView was created, the JavaScript wasn't sent to this RenderView until it becomes the current one, in order to make it current, it need to navigate to something, which doesn't meet our requirement that JavaScript needs executed before a page been loading. Basically, we need a way to start the RenderView without a navigation entry. On 2013/08/20 05:00:44, joth wrote: > this looks kind of complex, and the change in WebContentsImpl I don't really > understand. > > would it work to call CreateSwappedOutRenderView() here? This would give almost > the same callpath, but the RenderViewHostManager would then also be involved. > (It sounds like it should be in the know about this) https://codereview.chromium.org/23018005/diff/30001/content/browser/web_conte... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/23018005/diff/30001/content/browser/web_conte... content/browser/web_contents/web_contents_impl.cc:2874: On 2013/08/20 05:00:44, joth wrote: > style nit: I'd put "if (entry)" around all this code from 2875 - 2887 rather > than have the early-out above. This way you can share the FOR_EACH_OBSERVER > block Done.
+creis could you take a look at the web_contents_impl changes, or suggest someone who could? Thanks
Yes, I'll take a look soon. It looks like there may be some misunderstanding here, since it is not ok to create a WebContents or RenderViewHost without a SiteInstance.
As I mentioned, I don't think this is ok to add. Hopefully we can find another way to support the case you're thinking about. > Since renderer was created without SiteInstance Can you explain what this refers to? It concerns me that you have a WebContents without a SiteInstance, since SiteInstance is used to make process model and security decisions about the pages in the WebContents. https://codereview.chromium.org/23018005/diff/36001/content/browser/android/c... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/23018005/diff/36001/content/browser/android/c... content/browser/android/content_view_core_impl.cc:1482: if (!web_contents_->CreateRenderViewWithoutNavigationEntry(rvh)) { Maybe navigate it to about:blank first instead? https://codereview.chromium.org/23018005/diff/36001/content/browser/android/c... content/browser/android/content_view_core_impl.cc:1483: LOG(ERROR) << "Falied to create RenderView in EvaluateJavaScript"; nit: Failed https://codereview.chromium.org/23018005/diff/36001/content/browser/web_conte... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/23018005/diff/36001/content/browser/web_conte... content/browser/web_contents/web_contents_impl.h:159: bool CreateRenderViewWithoutNavigationEntry(RenderViewHost* render_view_host); This is not ok to add. For security reasons, there always needs to be a SiteInstance and NavigationEntry for a WebContents to have a renderer process. (The SiteInstance and NavigationEntry dictate what permissions are granted to the process, as well as which other pages need to end up in the same process to ensure they can script each other.) https://codereview.chromium.org/23018005/diff/36001/content/browser/web_conte... content/browser/web_contents/web_contents_impl.h:758: bool CreateRenderView(RenderViewHost* render_view_host, Please do not add this. max_page_id is an extremely difficult concept to get right and can lead to URL spoofs and renderer crashes if specified incorrectly.
The very basic use case is 1. The app runs JavaScript before load any page in WebView. like window.varible=value1 or JavaCallback.funct(); The JavaCallback is the name of a Java class, it allows the JavaScript to call into the Java, it requires the JavaScript must be executed before the second step. 2. Then, it loads a page from http://app.com, which has a JavaScript like if (window.variable == value1) do_something() There are actually 2 requirements here, - The JavaScript must be executed before loading a page. - The window scope variables should be accessed by the following page. I think we can assume the first JavaScript is part of the following loaded page, but it has to be executed before loading the page. Do you see any security issue here?
Just to add to what Michael said, this is not a "new" requirement being invented, rather a need to reveal existing WebKit functionality through the content/ API. *http://b/10123053#ISSUE_HistoryHeader30* has more background On 20 August 2013 15:25, <michaelbai@chromium.org> wrote: > The very basic use case is > > 1. The app runs JavaScript before load any page in WebView. > > like window.varible=value1 > > or JavaCallback.funct(); > > The JavaCallback is the name of a Java class, it allows the JavaScript to > call > into the Java, it requires the JavaScript must be executed before the > second > step. > > 2. Then, it loads a page from http://app.com, which has a JavaScript like > if (window.variable == value1) > do_something() > > > There are actually 2 requirements here, > - The JavaScript must be executed before loading a page. > - The window scope variables should be accessed by the following page. > > I think we can assume the first JavaScript is part of the following loaded > page, > but it has to be executed before loading the page. Do you see any security > issue > here? > > > https://codereview.chromium.**org/23018005/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Ok, after some discussion offline, it sounds like Android WebViews have a specific hack that lets them load script values into the initial empty document and have those values stick around when the page is loaded. We can debate the merits of that separately, since it's currently needed for compatibility reasons and isn't substantially worse than extension content scripts. To support it, we'll need the WebContents to have a SiteInstance, but it doesn't need to have a NavigationEntry until the first navigation. We just want to have a way to create the RenderView for the initial empty document, restricted to the Android WebView case. https://codereview.chromium.org/23018005/diff/36001/content/browser/web_conte... File content/browser/web_contents/web_contents_impl.cc (left): https://codereview.chromium.org/23018005/diff/36001/content/browser/web_conte... content/browser/web_contents/web_contents_impl.cc:3652: GetMaxPageIDForSiteInstance(render_view_host->GetSiteInstance()); These max page ID calls have to stay, or else the first page ID could be wrong. They should work fine if your RenderViewHost has a SiteInstance, which should be the case if you've already called WebContentsImpl::Init. https://codereview.chromium.org/23018005/diff/36001/content/browser/web_conte... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/23018005/diff/36001/content/browser/web_conte... content/browser/web_contents/web_contents_impl.cc:3680: return CreateRenderView(render_view_host, MSG_ROUTING_NONE, -1); Just call CreatRenderViewForRenderManager here. https://codereview.chromium.org/23018005/diff/36001/content/browser/web_conte... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/23018005/diff/36001/content/browser/web_conte... content/browser/web_contents/web_contents_impl.h:159: bool CreateRenderViewWithoutNavigationEntry(RenderViewHost* render_view_host); On 2013/08/20 22:03:49, creis wrote: > This is not ok to add. For security reasons, there always needs to be a > SiteInstance and NavigationEntry for a WebContents to have a renderer process. > (The SiteInstance and NavigationEntry dictate what permissions are granted to > the process, as well as which other pages need to end up in the same process to > ensure they can script each other.) If we are going to go down this road, we need to be much more explicit. The method should be CreateRenderViewForInitialEmptyDocument, and it should clearly state that this allows Android WebViews to use javascript: URLs that load into the DOMWindow before the first page load. We need to be very explicit that this is not safe to do in any context that a web page could get a reference to the DOMWindow before the first page load. Also, this should not take in a RenderViewHost, since it's only safe to do this with the current RednerViewHost of the WebContents.
Thanks creis, PTAL https://codereview.chromium.org/23018005/diff/36001/content/browser/android/c... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/23018005/diff/36001/content/browser/android/c... content/browser/android/content_view_core_impl.cc:1482: if (!web_contents_->CreateRenderViewWithoutNavigationEntry(rvh)) { Already tried, the Java context can not be transfer to the next page. On 2013/08/20 22:03:49, creis wrote: > Maybe navigate it to about:blank first instead? https://codereview.chromium.org/23018005/diff/36001/content/browser/android/c... content/browser/android/content_view_core_impl.cc:1483: LOG(ERROR) << "Falied to create RenderView in EvaluateJavaScript"; On 2013/08/20 22:03:49, creis wrote: > nit: Failed Done. https://codereview.chromium.org/23018005/diff/36001/content/browser/web_conte... File content/browser/web_contents/web_contents_impl.cc (left): https://codereview.chromium.org/23018005/diff/36001/content/browser/web_conte... content/browser/web_contents/web_contents_impl.cc:3652: GetMaxPageIDForSiteInstance(render_view_host->GetSiteInstance()); On 2013/08/21 22:06:26, creis wrote: > These max page ID calls have to stay, or else the first page ID could be wrong. > They should work fine if your RenderViewHost has a SiteInstance, which should be > the case if you've already called WebContentsImpl::Init. Done. https://codereview.chromium.org/23018005/diff/36001/content/browser/web_conte... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/23018005/diff/36001/content/browser/web_conte... content/browser/web_contents/web_contents_impl.cc:3680: return CreateRenderView(render_view_host, MSG_ROUTING_NONE, -1); On 2013/08/21 22:06:26, creis wrote: > Just call CreatRenderViewForRenderManager here. Done.
How bad is it if we break backwards compat with Android WebView's strange behavior?
Thanks. content/ LGTM, but let's have a plan for deprecating this when apps move to a new API. https://codereview.chromium.org/23018005/diff/45001/content/browser/web_conte... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/23018005/diff/45001/content/browser/web_conte... content/browser/web_contents/web_contents_impl.h:157: // In Android WebView, the RenderView needs created even there is no Wording nits in the first sentence: In Android WebView, the RenderView needs to be created even before the first navigation. This allows...
On 21 August 2013 16:40, <palmer@chromium.org> wrote: > How bad is it if we break backwards compat with Android WebView's strange > behavior? > > Bad. > https://codereview.chromium.**org/23018005/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
jochen@ for content/browser/web_contents
https://codereview.chromium.org/23018005/diff/45001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/23018005/diff/45001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContents.java:1384: mContentViewCore.evaluateJavaScriptEvenIfNotYetNavigated(script, jsCallback); Given creis's comments I don't think we want to do this unconditionally. I'd suggest plumbing a boolean through from the caller (right to ContentViewCore) to select which mode to use, but that's going to be really complex to merge and will cause this to cycle even more, so instead lets do this: do make the change above, instead add: public void evaluateJavaScriptEvenIfNotYetNavigated(String script) { mContentViewCore.evaluateJavaScriptEvenIfNotYetNavigated(script, null); } then in the glue layer call that method directly in the loadUrl("Javascipt:...") work around.
On 2013/08/22 05:24:48, michaelbai wrote: > jochen@ for content/browser/web_contents FYI, I'm a content/ owner, so that's sufficient for web_contents. I agree with joth's comment, though.
PTAL, I removed the callback parameter for evaluateJavaScriptEvenIfNotYetNavigated. https://codereview.chromium.org/23018005/diff/45001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/23018005/diff/45001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContents.java:1384: mContentViewCore.evaluateJavaScriptEvenIfNotYetNavigated(script, jsCallback); On 2013/08/22 05:44:41, joth wrote: > Given creis's comments I don't think we want to do this unconditionally. I'd > suggest plumbing a boolean through from the caller (right to ContentViewCore) to > select which mode to use, but that's going to be really complex to merge and > will cause this to cycle even more, so instead lets do this: > > do make the change above, instead add: > > public void evaluateJavaScriptEvenIfNotYetNavigated(String script) { > mContentViewCore.evaluateJavaScriptEvenIfNotYetNavigated(script, null); > } > > > then in the glue layer call that method directly in the > loadUrl("Javascipt:...") work around. Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/23018005/57001
Message was sent while issue was closed.
Change committed as 219111 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
