Fix webview.executeScript API

Fix a bug in the API webview.executeScript. After this fix, if the webview (who calls the executeScript API) is navigated to another source, the executeScript API won't be injected and executed in the new source.

BUG=239388
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263020

[Fady Samuel, 2014-04-04 19:35:05]

This looks great! Please specify the bug in BUG=

https://codereview.chromium.org/226043005/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/webview/webview_api.cc (right):

https://codereview.chromium.org/226043005/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/webview/webview_api.cc:304: std::string
WebviewExecuteCodeFunction::GetWebViewSrc() const {
Move down one line.

https://codereview.chromium.org/226043005/diff/1/chrome/renderer/resources/ex...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/226043005/diff/1/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/web_view.js:318: var args =
$Array.concat([this.instanceId],
[this.instanceId, this.src] instead.

[Xi Han, 2014-04-06 20:14:03]

I have no idea why the related tests on all test servers failed, and I cannot
open  the log of any of them. Any suggestion?

https://codereview.chromium.org/226043005/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/webview/webview_api.cc (right):

https://codereview.chromium.org/226043005/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/webview/webview_api.cc:304: std::string
WebviewExecuteCodeFunction::GetWebViewSrc() const {
On 2014/04/04 19:35:05, Fady Samuel wrote:
> Move down one line.

Done.

https://codereview.chromium.org/226043005/diff/1/chrome/renderer/resources/ex...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/226043005/diff/1/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/web_view.js:318: var args =
$Array.concat([this.instanceId],
On 2014/04/04 19:35:05, Fady Samuel wrote:
> [this.instanceId, this.src] instead.

Done.

[Fady Samuel, 2014-04-07 14:03:10]

One nit, but otherwise lgtm!

https://codereview.chromium.org/226043005/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/226043005/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/tabs/tabs_api.cc:1377: "",
std::string(),

[Xi Han, 2014-04-07 14:58:09]

Upload changes for review.

https://codereview.chromium.org/226043005/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/226043005/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/tabs/tabs_api.cc:1377: "",
On 2014/04/07 14:03:10, Fady Samuel wrote:
> std::string(),

Done.

[not at google - send to devlin, 2014-04-07 17:43:00]

main comment is around use of GURL.

https://codereview.chromium.org/226043005/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/webview/webview_api.cc (right):

https://codereview.chromium.org/226043005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/webview/webview_api.cc:271: }
make braces consistent i.e. here leave them out.

https://codereview.chromium.org/226043005/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/script_executor.h (right):

https://codereview.chromium.org/226043005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/script_executor.h:89: const std::string webview_src,
should be a const&.

but make it a GURL not a string (everywhere).

https://codereview.chromium.org/226043005/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/webview.json (right):

https://codereview.chromium.org/226043005/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/webview.json:332: "description": "The src of the
guest <webview> process."
why does this need to be exposed to the API?

https://codereview.chromium.org/226043005/diff/20001/chrome/renderer/extensio...
File chrome/renderer/extensions/user_script_scheduler.cc (right):

https://codereview.chromium.org/226043005/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_scheduler.cc:191:
WebString::fromUTF8(params.webview_src));
you should be able to use GURL == on this when everything is converted to a
GURL.

https://codereview.chromium.org/226043005/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_scheduler.cc:194:
!PermissionsData::CanExecuteScriptOnPage(
this is a very complex condition. Is there a way you can simplify it?

https://codereview.chromium.org/226043005/diff/20001/extensions/common/extens...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/226043005/diff/20001/extensions/common/extens...
extensions/common/extension_messages.h:114: IPC_STRUCT_MEMBER(std::string,
webview_src)
GURL?

[not at google - send to devlin, 2014-04-07 17:44:17]

that said, one reason to use a std::string is because the src= might be an
invalid GURL.

however, in that case what does it mean for the load state of the webview? can
you even execute a script in that case?

[Fady Samuel, 2014-04-07 17:49:31]

https://codereview.chromium.org/226043005/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/webview.json (right):

https://codereview.chromium.org/226043005/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/webview.json:332: "description": "The src of the
guest <webview> process."
On 2014/04/07 17:43:00, kalman wrote:
> why does this need to be exposed to the API?

This is an internal API.

[Fady Samuel, 2014-04-07 17:49:31]

https://codereview.chromium.org/226043005/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/webview.json (right):

https://codereview.chromium.org/226043005/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/webview.json:332: "description": "The src of the
guest <webview> process."
On 2014/04/07 17:43:00, kalman wrote:
> why does this need to be exposed to the API?

This is an internal API.

[Fady Samuel, 2014-04-07 20:39:34]

Check if it's invalid.

https://codereview.chromium.org/226043005/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/webview/webview_api.cc (right):

https://codereview.chromium.org/226043005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/webview/webview_api.cc:272: if
(guest_src_.is_empty())
if (guest_src_.is_empty() || guest_src_.invalid())

[not at google - send to devlin, 2014-04-07 20:43:17]

please mark things as Done when you do them so I know to have another look.

https://codereview.chromium.org/226043005/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/webview/webview_api.cc (right):

https://codereview.chromium.org/226043005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/webview/webview_api.cc:272: if
(guest_src_.is_empty())
On 2014/04/07 20:39:34, Fady Samuel wrote:
> if (guest_src_.is_empty() || guest_src_.invalid())

I think that empty implies invalid.

[Fady Samuel, 2014-04-07 20:44:11]

https://codereview.chromium.org/226043005/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/webview/webview_api.cc (right):

https://codereview.chromium.org/226043005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/webview/webview_api.cc:272: if
(guest_src_.is_empty())
On 2014/04/07 20:43:18, kalman wrote:
> On 2014/04/07 20:39:34, Fady Samuel wrote:
> > if (guest_src_.is_empty() || guest_src_.invalid())
> 
> I think that empty implies invalid.

Perhaps then if (guest_src_.invalid())?

[not at google - send to devlin, 2014-04-07 21:00:07]

please fix that is_valid thing, but other than that, lgtm.

https://codereview.chromium.org/226043005/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/webview/webview_api.cc (right):

https://codereview.chromium.org/226043005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/webview/webview_api.cc:272: if
(guest_src_.is_empty())
On 2014/04/07 20:44:12, Fady Samuel wrote:
> On 2014/04/07 20:43:18, kalman wrote:
> > On 2014/04/07 20:39:34, Fady Samuel wrote:
> > > if (guest_src_.is_empty() || guest_src_.invalid())
> > 
> > I think that empty implies invalid.
> 
> Perhaps then if (guest_src_.invalid())?

It would be

if (!guest_src_.is_valid())
  return false;

https://codereview.chromium.org/226043005/diff/40001/chrome/renderer/extensio...
File chrome/renderer/extensions/user_script_scheduler.cc (right):

https://codereview.chromium.org/226043005/diff/40001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_scheduler.cc:199:
child_frame->document().url() != params.webview_src)) {
much nicer, thanks!

[Xi Han, 2014-04-07 21:00:35]

Sorry, I will mark things done.

On 2014/04/07 20:43:17, kalman wrote:
> please mark things as Done when you do them so I know to have another look.
> 
>
https://codereview.chromium.org/226043005/diff/40001/chrome/browser/extension...
> File chrome/browser/extensions/api/webview/webview_api.cc (right):
> 
>
https://codereview.chromium.org/226043005/diff/40001/chrome/browser/extension...
> chrome/browser/extensions/api/webview/webview_api.cc:272: if
> (guest_src_.is_empty())
> On 2014/04/07 20:39:34, Fady Samuel wrote:
> > if (guest_src_.is_empty() || guest_src_.invalid())
> 
> I think that empty implies invalid.

[Xi Han, 2014-04-07 21:03:36]

https://codereview.chromium.org/226043005/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/webview/webview_api.cc (right):

https://codereview.chromium.org/226043005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/webview/webview_api.cc:271: }
On 2014/04/07 17:43:00, kalman wrote:
> make braces consistent i.e. here leave them out.

Done.

https://codereview.chromium.org/226043005/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/script_executor.h (right):

https://codereview.chromium.org/226043005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/script_executor.h:89: const std::string webview_src,
On 2014/04/07 17:43:00, kalman wrote:
> should be a const&.
> 
> but make it a GURL not a string (everywhere).

Done.

https://codereview.chromium.org/226043005/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/webview.json (right):

https://codereview.chromium.org/226043005/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/webview.json:332: "description": "The src of the
guest <webview> process."
On 2014/04/07 17:49:32, Fady Samuel wrote:
> On 2014/04/07 17:43:00, kalman wrote:
> > why does this need to be exposed to the API?
> 
> This is an internal API.

Done.

https://codereview.chromium.org/226043005/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/webview.json:332: "description": "The src of the
guest <webview> process."
On 2014/04/07 17:43:00, kalman wrote:
> why does this need to be exposed to the API?

Done.

https://codereview.chromium.org/226043005/diff/20001/chrome/renderer/extensio...
File chrome/renderer/extensions/user_script_scheduler.cc (right):

https://codereview.chromium.org/226043005/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_scheduler.cc:191:
WebString::fromUTF8(params.webview_src));
On 2014/04/07 17:43:00, kalman wrote:
> you should be able to use GURL == on this when everything is converted to a
> GURL.

Done.

https://codereview.chromium.org/226043005/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_scheduler.cc:194:
!PermissionsData::CanExecuteScriptOnPage(
On 2014/04/07 17:43:00, kalman wrote:
> this is a very complex condition. Is there a way you can simplify it?

Done.

https://codereview.chromium.org/226043005/diff/20001/extensions/common/extens...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/226043005/diff/20001/extensions/common/extens...
extensions/common/extension_messages.h:114: IPC_STRUCT_MEMBER(std::string,
webview_src)
On 2014/04/07 17:43:00, kalman wrote:
> GURL?

Done.

https://codereview.chromium.org/226043005/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/webview/webview_api.cc (right):

https://codereview.chromium.org/226043005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/webview/webview_api.cc:272: if
(guest_src_.is_empty())
On 2014/04/07 20:44:12, Fady Samuel wrote:
> On 2014/04/07 20:43:18, kalman wrote:
> > On 2014/04/07 20:39:34, Fady Samuel wrote:
> > > if (guest_src_.is_empty() || guest_src_.invalid())
> > 
> > I think that empty implies invalid.
> 
> Perhaps then if (guest_src_.invalid())?

Done.

https://codereview.chromium.org/226043005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/webview/webview_api.cc:272: if
(guest_src_.is_empty())
On 2014/04/07 20:39:34, Fady Samuel wrote:
> if (guest_src_.is_empty() || guest_src_.invalid())

Done.

https://codereview.chromium.org/226043005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/webview/webview_api.cc:272: if
(guest_src_.is_empty())
On 2014/04/07 20:43:18, kalman wrote:
> On 2014/04/07 20:39:34, Fady Samuel wrote:
> > if (guest_src_.is_empty() || guest_src_.invalid())
> 
> I think that empty implies invalid.

Done.

https://codereview.chromium.org/226043005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/webview/webview_api.cc:272: if
(guest_src_.is_empty())
On 2014/04/07 20:44:12, Fady Samuel wrote:
> On 2014/04/07 20:43:18, kalman wrote:
> > On 2014/04/07 20:39:34, Fady Samuel wrote:
> > > if (guest_src_.is_empty() || guest_src_.invalid())
> > 
> > I think that empty implies invalid.
> 
> Perhaps then if (guest_src_.invalid())?

Done.

https://codereview.chromium.org/226043005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/webview/webview_api.cc:272: if
(guest_src_.is_empty())
It makes sense to me, invalide() (actually !guest_src_.is_valid()) should
included the case of empty url.

On 2014/04/07 20:44:12, Fady Samuel wrote:
> On 2014/04/07 20:43:18, kalman wrote:
> > On 2014/04/07 20:39:34, Fady Samuel wrote:
> > > if (guest_src_.is_empty() || guest_src_.invalid())
> > 
> > I think that empty implies invalid.
> 
> Perhaps then if (guest_src_.invalid())?

[Xi Han, 2014-04-07 21:09:43]

https://codereview.chromium.org/226043005/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/226043005/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/tabs/tabs_api.cc:1377: "",
On 2014/04/07 14:03:10, Fady Samuel wrote:
> std::string(),

Done.

https://codereview.chromium.org/226043005/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/tabs/tabs_api.cc:1377: "",
On 2014/04/07 14:03:10, Fady Samuel wrote:
> std::string(),

Done.

https://codereview.chromium.org/226043005/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/webview/webview_api.cc (right):

https://codereview.chromium.org/226043005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/webview/webview_api.cc:272: if
(guest_src_.is_empty())
Thx for checking the real method name.

On 2014/04/07 21:00:07, kalman wrote:
> On 2014/04/07 20:44:12, Fady Samuel wrote:
> > On 2014/04/07 20:43:18, kalman wrote:
> > > On 2014/04/07 20:39:34, Fady Samuel wrote:
> > > > if (guest_src_.is_empty() || guest_src_.invalid())
> > > 
> > > I think that empty implies invalid.
> > 
> > Perhaps then if (guest_src_.invalid())?
> 
> It would be
> 
> if (!guest_src_.is_valid())
>   return false;

https://codereview.chromium.org/226043005/diff/40001/chrome/renderer/extensio...
File chrome/renderer/extensions/user_script_scheduler.cc (right):

https://codereview.chromium.org/226043005/diff/40001/chrome/renderer/extensio...
chrome/renderer/extensions/user_script_scheduler.cc:199:
child_frame->document().url() != params.webview_src)) {
On 2014/04/07 21:00:07, kalman wrote:
> much nicer, thanks!

Done.

[Fady Samuel, 2014-04-07 21:10:45]

https://codereview.chromium.org/226043005/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/webview/webview_api.cc (right):

https://codereview.chromium.org/226043005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/webview/webview_api.cc:307: GURL
WebviewExecuteCodeFunction::GetWebViewSrc() const { return guest_src_; }
You can probably return const GURL&

[Xi Han, 2014-04-07 21:42:15]

Update the three places where GetWebViewSrc() are declared:
 -execute_code_function.h
 -webview_api.h
 -tabs_api.h

Two places it is implemented:
 -webview_api.cc
 -tabs_api.cc

https://codereview.chromium.org/226043005/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/webview/webview_api.cc (right):

https://codereview.chromium.org/226043005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/webview/webview_api.cc:307: GURL
WebviewExecuteCodeFunction::GetWebViewSrc() const { return guest_src_; }
On 2014/04/07 21:10:45, Fady Samuel wrote:
> You can probably return const GURL&

Done.

[Fady Samuel, 2014-04-07 21:44:10]

lgtm

[Xi Han, 2014-04-08 17:00:15]

Could you please review the extension_message.h file? Thank you.

[Xi Han, 2014-04-08 18:27:23]

Hello Kalman,

Could you please take a final look at the code updates? I uploaded the code
changes for the invalid check for url. Thank you very much.

[kenrb, 2014-04-08 19:02:14]

lgtm for the IPC change.

[not at google - send to devlin, 2014-04-08 20:01:48]

lgtm

[Xi Han, 2014-04-08 20:17:18]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-04-08 20:18:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hanxi@chromium.org/226043005/80001

[commit-bot: I haz the power, 2014-04-08 20:18:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-08 20:18:25]

Failed to apply patch for chrome/renderer/extensions/user_script_scheduler.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/renderer/extensions/user_script_scheduler.cc
  Hunk #1 FAILED at 185.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/renderer/extensions/user_script_scheduler.cc.rej

Patch:       chrome/renderer/extensions/user_script_scheduler.cc
Index: chrome/renderer/extensions/user_script_scheduler.cc
diff --git a/chrome/renderer/extensions/user_script_scheduler.cc
b/chrome/renderer/extensions/user_script_scheduler.cc
index
b4cf64f135fad41982506854958dd402fbe9169e..6f2dd032a548bb64461b745a1609f8a3233422aa
100644
--- a/chrome/renderer/extensions/user_script_scheduler.cc
+++ b/chrome/renderer/extensions/user_script_scheduler.cc
@@ -185,15 +185,18 @@ void UserScriptScheduler::ExecuteCodeImpl(
       //
       // For child frames, we just skip ones the extension doesn't have access
       // to and carry on.
-      if (!params.is_web_view &&
-          !PermissionsData::CanExecuteScriptOnPage(
-              extension,
-              child_frame->document().url(),
-              frame_->document().url(),
-              extension_helper->tab_id(),
-              NULL,
-              -1,
-              NULL)) {
+
+      bool can_execute_script =
+          PermissionsData::CanExecuteScriptOnPage(extension,
+                                                 
child_frame->document().url(),
+                                                  frame_->document().url(),
+                                                  extension_helper->tab_id(),
+                                                  NULL,
+                                                  -1,
+                                                  NULL);
+      if ((!params.is_web_view && !can_execute_script) ||
+          (params.is_web_view &&
+           child_frame->document().url() != params.webview_src)) {
         if (child_frame->parent()) {
           continue;
         } else {

[Xi Han, 2014-04-08 21:06:33]

Updates cl after solving conflicts

[Xi Han, 2014-04-08 21:06:40]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-04-08 21:08:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hanxi@chromium.org/226043005/100001

[commit-bot: I haz the power, 2014-04-08 22:04:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-08 22:04:27]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[Xi Han, 2014-04-10 14:55:25]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-04-10 14:55:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hanxi@chromium.org/226043005/140001

[commit-bot: I haz the power, 2014-04-10 17:12:54]

Change committed as 263020