<browser>: Always read <browser>.src attribute from <object>.src.

<browser>: Always read <browser>.src attribute from <object>.src.
If we navigate to 'http://www.google.com', then navigate to '' (empty src), reading <browser>.src would now give us (expected) 'http://www.google.com' since we ignore setting empty src attribute in browser_plugin.cc.

BUG=149001
TEST=In web inspector, setting src on <browser> element and reading it.
Added platform app test for verifying the change.
Ran content_browsertests for BrowserPluginTest* and BrowserPluginHostTest*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162011

[lazyboy, 2012-10-12 18:49:22]

Fady, can you take a look?
Also looks like we were setting empty src value in browser_plugin.cc which I've
fixed, hopefully there won't be any side effects. (All tests pass too).

[Fady Samuel, 2012-10-12 18:54:44]

LGTM after comments addressed! Please get mihaip@ to review this too as he wrote
the browser tag shim.

https://chromiumcodereview.appspot.com/11111020/diff/5001/chrome/renderer/res...
File chrome/renderer/resources/extensions/browser_tag.js (right):

https://chromiumcodereview.appspot.com/11111020/diff/5001/chrome/renderer/res...
chrome/renderer/resources/extensions/browser_tag.js:61: // have different value
when empty src is set.
Also, BrowserPlugin updates its src attribute on guest-initiated navigations as
well. Please include that in the comment.

https://chromiumcodereview.appspot.com/11111020/diff/5001/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/11111020/diff/5001/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:139: if (!src.empty())
Hmm, do we want to continue doing the rest of BrowserPlugin::SetSrcAttribute if
src.empty()? Maybe exit early instead?

[lazyboy, 2012-10-12 19:16:17]

https://chromiumcodereview.appspot.com/11111020/diff/5001/chrome/renderer/res...
File chrome/renderer/resources/extensions/browser_tag.js (right):

https://chromiumcodereview.appspot.com/11111020/diff/5001/chrome/renderer/res...
chrome/renderer/resources/extensions/browser_tag.js:61: // have different value
when empty src is set.
On 2012/10/12 18:54:44, Fady Samuel wrote:
> Also, BrowserPlugin updates its src attribute on guest-initiated navigations
as
> well. Please include that in the comment.

Done.

https://chromiumcodereview.appspot.com/11111020/diff/5001/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/11111020/diff/5001/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:139: if (!src.empty())
On 2012/10/12 18:54:44, Fady Samuel wrote:
> Hmm, do we want to continue doing the rest of BrowserPlugin::SetSrcAttribute
if
> src.empty()? Maybe exit early instead?
I think we need to send the resize params above otherwise we need to start
re-doing src setting flow, so it's less brittle to do it here; I've added return
from here, let me know if that's good.

[Fady Samuel, 2012-10-12 20:03:36]

https://chromiumcodereview.appspot.com/11111020/diff/8001/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/11111020/diff/8001/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:141: return;
That seems wrong? If the src is not empty, we should navigate, here, you're
updating the member variable but not navigating?

[lazyboy, 2012-10-12 20:41:13]

https://chromiumcodereview.appspot.com/11111020/diff/8001/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/11111020/diff/8001/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:141: return;
On 2012/10/12 20:03:36, Fady Samuel wrote:
> That seems wrong? If the src is not empty, we should navigate, here, you're
> updating the member variable but not navigating?
I did the wrong change on that pass :S
Now moved the empty check at the very beginning, sanity check with resizes seems
ok and all browsertests pass.

[Fady Samuel, 2012-10-12 20:45:05]

https://chromiumcodereview.appspot.com/11111020/diff/17001/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/11111020/diff/17001/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:127: // non-empty
page, the action is considered no-op.
and should never be sent to the browser process.

[lazyboy, 2012-10-12 20:51:39]

https://chromiumcodereview.appspot.com/11111020/diff/17001/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/11111020/diff/17001/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:127: // non-empty
page, the action is considered no-op.
On 2012/10/12 20:45:05, Fady Samuel wrote:
> and should never be sent to the browser process.

Done.

[lazyboy, 2012-10-12 20:54:05]

+Mihai for review.
Mihai, can you take a look? Thanks.

[Mihai Parparita -not on Chrome, 2012-10-12 22:26:55]

LGTM

https://chromiumcodereview.appspot.com/11111020/diff/17002/chrome/renderer/re...
File chrome/renderer/resources/extensions/browser_tag.js (right):

https://chromiumcodereview.appspot.com/11111020/diff/17002/chrome/renderer/re...
chrome/renderer/resources/extensions/browser_tag.js:54: var objectNode =
this.objectNode_;
As mentioned in the other review, you shouldn't need this, you can just return
this.objectNode_.src below, since "this" is bound to the right thing in the
loop.

[lazyboy, 2012-10-12 23:30:43]

https://chromiumcodereview.appspot.com/11111020/diff/17002/chrome/renderer/re...
File chrome/renderer/resources/extensions/browser_tag.js (right):

https://chromiumcodereview.appspot.com/11111020/diff/17002/chrome/renderer/re...
chrome/renderer/resources/extensions/browser_tag.js:54: var objectNode =
this.objectNode_;
On 2012/10/12 22:26:55, Mihai Parparita wrote:
> As mentioned in the other review, you shouldn't need this, you can just return
> this.objectNode_.src below, since "this" is bound to the right thing in the
> loop.

Hmmm, I think I'm one level too deep to do that.
'this' is bound correctly in the loop (the function body beginning on line 57),
but not for the inner function that defines get (line 59).
Or, maybe I'm missing something?

[Mihai Parparita -not on Chrome, 2012-10-13 00:12:44]

https://chromiumcodereview.appspot.com/11111020/diff/17002/chrome/renderer/re...
File chrome/renderer/resources/extensions/browser_tag.js (right):

https://chromiumcodereview.appspot.com/11111020/diff/17002/chrome/renderer/re...
chrome/renderer/resources/extensions/browser_tag.js:54: var objectNode =
this.objectNode_;
On 2012/10/12 23:30:43, lazyboy wrote:
> On 2012/10/12 22:26:55, Mihai Parparita wrote:
> > As mentioned in the other review, you shouldn't need this, you can just
return
> > this.objectNode_.src below, since "this" is bound to the right thing in the
> > loop.
> 
> Hmmm, I think I'm one level too deep to do that.
> 'this' is bound correctly in the loop (the function body beginning on line
57),
> but not for the inner function that defines get (line 59).
> Or, maybe I'm missing something?

Ah, yes, you're right. LGTM as is.

[lazyboy, 2012-10-13 02:54:04]

+Charlie for content/*/browser_plugin/ OWNER.

[Charlie Reis, 2012-10-15 18:57:28]

https://chromiumcodereview.appspot.com/11111020/diff/17002/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/11111020/diff/17002/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:129:
CHECK(!src.empty());
Hmm, we've got a few problems here, actually.  Keep in mind that renderer
processes aren't supposed to be trusted.  If they're exploited, they might send
any values back to the browser process.  So here, the renderer could kill the
browser process by sending an empty URL.

Worse, we're skipping the security checks we normally do about whether a URL is
allowed to be loaded.  (See uses of FilterURL in RenderViewHostImpl.)

Here's what I'd recommend: change this to a DCHECK but also keep the if, so that
we catch bugs in debug builds but don't crash in the wild.  Then put a
TODO(creis) comment to check the validity of the URL, referencing
http://crbug.com/139397.

https://chromiumcodereview.appspot.com/11111020/diff/17002/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/11111020/diff/17002/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:106: // If we haven't created
the guest yet, do so now, we will navigate it right
nit: Start a new sentence at "we will".

[lazyboy, 2012-10-15 20:45:57]

https://chromiumcodereview.appspot.com/11111020/diff/17002/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/11111020/diff/17002/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:129:
CHECK(!src.empty());
On 2012/10/15 18:57:28, creis wrote:
> Hmm, we've got a few problems here, actually.  Keep in mind that renderer
> processes aren't supposed to be trusted.  If they're exploited, they might
send
> any values back to the browser process.  So here, the renderer could kill the
> browser process by sending an empty URL.
> 
> Worse, we're skipping the security checks we normally do about whether a URL
is
> allowed to be loaded.  (See uses of FilterURL in RenderViewHostImpl.)
> 
> Here's what I'd recommend: change this to a DCHECK but also keep the if, so
that
> we catch bugs in debug builds but don't crash in the wild.  Then put a
> TODO(creis) comment to check the validity of the URL, referencing
> http://crbug.com/139397.

Done.

https://chromiumcodereview.appspot.com/11111020/diff/17002/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/11111020/diff/17002/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:106: // If we haven't created
the guest yet, do so now, we will navigate it right
On 2012/10/15 18:57:28, creis wrote:
> nit: Start a new sentence at "we will".

Done.

[Charlie Reis, 2012-10-15 21:03:55]

Thanks.  content/ LGTM.

[commit-bot: I haz the power, 2012-10-15 21:57:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/11111020/25001

[commit-bot: I haz the power, 2012-10-16 00:15:16]

Change committed as 162011