[DISCARD] While getting plug-in info, use securityOrigin() of the top level frame

** THIS CL DOESN'T FIX THE ISSUE **
While getting plug-in info, use securityOrigin() of the top level frame
instead of trying to access top level frame document's url().

If the top level frame in the renderer/ is a RemoteFrame, it does not
have a document() and trying to fetch document().url() would result in
crash.

We need top level frame's "origin url" only for getting plug-in info
anyway.

Open questions:
a. The top origin url param in ChromeViewHostMsg_GetPluginInfo can be
  changed to be of type base::string16 instead of GURL.
b. What happens if one of the call to frame->top()->securityOrigin().isNull()
  == true?  Is that possible?

BUG=426658
Test=with --site-per-process flag, see following scenarios work w/o crashing renderer
(terminal shouldn't see any SEGV_MAPPERR while accessing blink::WebDocument::url()):

1) Load a page that has a plugin and make sure you have that plugin enabled.
  e.g. http://a.tommycli.com/layout1_iframe_leiz.html
  Observe the plugin load correctly.

2) Disable the plugin from chrome://plugins, Observe that there is not renderer
  crash stacktrace.

3) Load a page with a plugin that is disabled or change your chrome://settings
  chrome://settings/content so that you have "Click to play" selected under "Plug-ins"
  section. This will trigger a "plug-in placeholder" thing.
  Observe the plugin placeholder load correctly.

[lazyboy, 2014-12-09 22:17:27]

Adding some notes:

lazyboy@osmium:/ssd/wblink2/src$ git gs  "\bChromeViewHostMsg_GetPluginInfo\b"
chrome/browser/plugins/plugin_info_message_filter.cc:207:   
IPC_MESSAGE_HANDLER_DELAY_REPLY(ChromeViewHostMsg_GetPluginInfo,
chrome/browser/plugins/plugin_info_message_filter.cc:277: 
ChromeViewHostMsg_GetPluginInfo::WriteReplyParams(reply_msg, output);
chrome/common/render_messages.h:100:// Output parameters for
ChromeViewHostMsg_GetPluginInfo message.
chrome/common/render_messages.h:448:IPC_SYNC_MESSAGE_CONTROL4_1(ChromeViewHostMsg_GetPluginInfo,
chrome/renderer/chrome_content_renderer_client.cc:589:  render_frame->Send(new
ChromeViewHostMsg_GetPluginInfo(
chrome/renderer/chrome_content_renderer_client.cc:679:      // actual mime type
via ChromeViewHostMsg_GetPluginInfo. In that case
chrome/renderer/chrome_content_renderer_client_browsertest.cc:88:     
IPC_MESSAGE_HANDLER(ChromeViewHostMsg_GetPluginInfo, OnGetPluginInfo)
chrome/renderer/plugins/chrome_plugin_placeholder.cc:306:      new
ChromeViewHostMsg_GetPluginInfo(routing_id(),
chrome/renderer/plugins/shadow_dom_plugin_placeholder.cc:50:      new
ChromeViewHostMsg_GetPluginInfo(render_frame->GetRoutingID(),

There are 3 places from where we send ChromeViewHostMsg_GetPluginInfo.

[lazyboy, 2014-12-09 22:20:40]

lazyboy@chromium.org changed reviewers:
+ creis@chromium.org

[lazyboy, 2014-12-09 22:20:41]

+Charlie for taking a look.

See the CL description and comment #1 first for issues that need to be resolved.

[Charlie Reis, 2014-12-09 23:36:11]

[+alexmos]

> Open questions:
> a. The top origin url param in ChromeViewHostMsg_GetPluginInfo can be
>  changed to be of type base::string16 instead of GURL.

Hmm.  On one hand, it might be nice to convert the IPC message to use
url::Origin rather than GURL, similar to how Alex passes it in the origin
replication CL:
https://codereview.chromium.org/692973005/diff/300001/content/common/frame_me...

However, the end use case pulls out the scheme, host, and port in the browser
process, which requires a GURL:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/plu...

Either we need a way to convert Origin to GURL, or we should convert
ChromePluginServiceFilter's RestrictedPluginPair to use Origin instead of GURL
(and thus we could just do a straight Origin comparison).

Alex, does that sound right?

> b. What happens if one of the call to frame->top()->securityOrigin().isNull()
>  == true?  Is that possible?

The security origin may be serialized to a string as "null" if it's a unique
origin, but I don't know if securityOrigin().isNull() can ever return true. 
Alex?

[Charlie Reis, 2014-12-09 23:42:32]

creis@chromium.org changed reviewers:
+ alexmos@chromium.org

[Charlie Reis, 2014-12-09 23:42:32]

Actually +alexmos this time.

[alexmos, 2014-12-10 00:36:08]

On 2014/12/09 23:36:11, Charlie Reis wrote:
> [+alexmos]
> 
> > Open questions:
> > a. The top origin url param in ChromeViewHostMsg_GetPluginInfo can be
> >  changed to be of type base::string16 instead of GURL.
> 
> Hmm.  On one hand, it might be nice to convert the IPC message to use
> url::Origin rather than GURL, similar to how Alex passes it in the origin
> replication CL:
>
https://codereview.chromium.org/692973005/diff/300001/content/common/frame_me...
> 
> However, the end use case pulls out the scheme, host, and port in the browser
> process, which requires a GURL:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/plu...
> 
> Either we need a way to convert Origin to GURL, or we should convert
> ChromePluginServiceFilter's RestrictedPluginPair to use Origin instead of GURL
> (and thus we could just do a straight Origin comparison).
> 
> Alex, does that sound right?

Yes.  I think using either GURLs or url::Origins seems OK here, but it's
important to stick with one throughout.  For example, GURL::GetOrigin can be
incompatible with   WebSecurityOrigin::toString() - GURL::GetOrigin() has a
trailing slash (which violates RFC 6454), but WebSecurityOrigin::toString() does
not, so you wouldn't want to compare them.  If you already have other places
using GURLs to deal with origins, I think the current GURL approach is fine.  If
this is isolated, using url::Origins throughout may better express that you're
passing origins and not arbitrary URLs. 

> 
> > b. What happens if one of the call to
frame->top()->securityOrigin().isNull()
> >  == true?  Is that possible?
> 
> The security origin may be serialized to a string as "null" if it's a unique
> origin, but I don't know if securityOrigin().isNull() can ever return true. 
> Alex?

I don't think web_frame->securityOrigin().isNull() would ever be null.  Local
frames should have a Document with a SecurityOrigin, and RemoteFrames get their
securityOrigin initialized at creation time.

Yes, securityOrigin().toString() can be the string "null" for unique origins. 
You may want to check for that with securityOrigin().isUnique().  

Do you need to handle file origins?  Those are returned as "file://" from
WebSecurityOrigin::toString() and I don't know if your comparison on the browser
side does the right thing for that.

[lazyboy, 2014-12-12 02:15:35]

On 2014/12/10 00:36:08, alexmos wrote:
> On 2014/12/09 23:36:11, Charlie Reis wrote:
> > [+alexmos]
> > 
> > > Open questions:
> > > a. The top origin url param in ChromeViewHostMsg_GetPluginInfo can be
> > >  changed to be of type base::string16 instead of GURL.
> > 
> > Hmm.  On one hand, it might be nice to convert the IPC message to use
> > url::Origin rather than GURL, similar to how Alex passes it in the origin
> > replication CL:
> >
>
https://codereview.chromium.org/692973005/diff/300001/content/common/frame_me...
> > 
> > However, the end use case pulls out the scheme, host, and port in the
browser
> > process, which requires a GURL:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/plu...
> > 
> > Either we need a way to convert Origin to GURL, or we should convert
> > ChromePluginServiceFilter's RestrictedPluginPair to use Origin instead of
GURL
> > (and thus we could just do a straight Origin comparison).
> > 
> > Alex, does that sound right?
> 
> Yes.  I think using either GURLs or url::Origins seems OK here, but it's
> important to stick with one throughout.  For example, GURL::GetOrigin can be
> incompatible with   WebSecurityOrigin::toString() - GURL::GetOrigin() has a
> trailing slash (which violates RFC 6454), but WebSecurityOrigin::toString()
does
> not, so you wouldn't want to compare them.  If you already have other places
> using GURLs to deal with origins, I think the current GURL approach is fine. 
If
> this is isolated, using url::Origins throughout may better express that you're
> passing origins and not arbitrary URLs. 
> 
> > 
> > > b. What happens if one of the call to
> frame->top()->securityOrigin().isNull()
> > >  == true?  Is that possible?
> > 
> > The security origin may be serialized to a string as "null" if it's a unique
> > origin, but I don't know if securityOrigin().isNull() can ever return true. 
> > Alex?
> 
> I don't think web_frame->securityOrigin().isNull() would ever be null.  Local
> frames should have a Document with a SecurityOrigin, and RemoteFrames get
their
> securityOrigin initialized at creation time.
> 
> Yes, securityOrigin().toString() can be the string "null" for unique origins. 
> You may want to check for that with securityOrigin().isUnique().  
> 
> Do you need to handle file origins?  Those are returned as "file://" from
> WebSecurityOrigin::toString() and I don't know if your comparison on the
browser
> side does the right thing for that.
Alex and I found that we get "null" for file:// URLs as
enforceFilePathSeparation is
always turned on.


However, I've looked at the complete code path and found some more info:

1)
The PluginServiceFilter::IsPluginAvailable() call should be fine b/c
the RestrictedPluginPair seems to only care about chrome extension URLs.

2)
However there is another code path that goes to ContentSettingsMap:
   PluginInfoMessageFilter::OnGetPluginInfo
     -> PluginInfoMessageFilter::PluginsLoaded
       -> PluginInfoMessageFilter::Contex::DecidePluginStatus(top_origin_url)
         -> PluginInfoMessageFilter::Context::GetPluginContentSetting(policy_url
= top_origin_url)
           -> HostContentSettingsMap::GetWebsiteSetting(primary_url =
policy_url, sometimes secondary_url = policy_url, content_type =
JAVASCRIPT/PLUGINS)
             -> HostContentSettingsMap::ShouldAllowAllContent(primary_url,
secondary_url)
               calls primary_url.SchemeIsSecure() to return true.
               calls primary_url.SchemeIs(chrome-extension)
                 calls secondary_url.SchemeIs(chrome-extension)
               calls primary_url.SchemeIs(devtools)
               calls primary_url.SchemeIs(chrome-ui)
             -> HostContentSettingsMap::GetWebsiteSettingInternal(primary_url,
secondary_url)
               -> content_settings::GetContentSettingValueAndPatterns
                 calls ContentSettingsPattern::Matches()

This gets more involving, e.g.
Matches() looks for file scheme and reads inner_url() of the GURL:
https://code.google.com/p/chromium/codesearch#chromium/src/components/content...
and then it also looks the file path:
https://code.google.com/p/chromium/codesearch#chromium/src/components/content...
I stopped here and have not looked further.

In theory though, content_settings stuff should be dealing *mostly* /w origins,
right?
But lack of file:// path seems like a prob for now.
This TODO suggests we should remove the "path" thing from
ContentSettingsPattern::PatternParts:
https://code.google.com/p/chromium/codesearch#chromium/src/components/content...


To wrap up:
1) Top level urls that have uniqueOrigin/"null" is a problem if we want
to feed the origin to HostContentSettingsMap::GetWebsiteSettings()
or we have to prove they should work OK in uniqueOrigin case.

2) The top level file:// URL becomes "null" is a problem b/c we
can't do special treatment to file URLs.

3) ContentSettingsPattern should not contain PatternParts::path.

[Charlie Reis, 2014-12-12 18:29:12]

On 2014/12/12 02:15:35, lazyboy wrote:
> On 2014/12/10 00:36:08, alexmos wrote:
> > On 2014/12/09 23:36:11, Charlie Reis wrote:
> > > [+alexmos]
> > > 
> > > > Open questions:
> > > > a. The top origin url param in ChromeViewHostMsg_GetPluginInfo can be
> > > >  changed to be of type base::string16 instead of GURL.
> > > 
> > > Hmm.  On one hand, it might be nice to convert the IPC message to use
> > > url::Origin rather than GURL, similar to how Alex passes it in the origin
> > > replication CL:
> > >
> >
>
https://codereview.chromium.org/692973005/diff/300001/content/common/frame_me...
> > > 
> > > However, the end use case pulls out the scheme, host, and port in the
> browser
> > > process, which requires a GURL:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/plu...
> > > 
> > > Either we need a way to convert Origin to GURL, or we should convert
> > > ChromePluginServiceFilter's RestrictedPluginPair to use Origin instead of
> GURL
> > > (and thus we could just do a straight Origin comparison).
> > > 
> > > Alex, does that sound right?
> > 
> > Yes.  I think using either GURLs or url::Origins seems OK here, but it's
> > important to stick with one throughout.  For example, GURL::GetOrigin can be
> > incompatible with   WebSecurityOrigin::toString() - GURL::GetOrigin() has a
> > trailing slash (which violates RFC 6454), but WebSecurityOrigin::toString()
> does
> > not, so you wouldn't want to compare them.  If you already have other places
> > using GURLs to deal with origins, I think the current GURL approach is fine.

> If
> > this is isolated, using url::Origins throughout may better express that
you're
> > passing origins and not arbitrary URLs. 
> > 
> > > 
> > > > b. What happens if one of the call to
> > frame->top()->securityOrigin().isNull()
> > > >  == true?  Is that possible?
> > > 
> > > The security origin may be serialized to a string as "null" if it's a
unique
> > > origin, but I don't know if securityOrigin().isNull() can ever return
true. 
> > > Alex?
> > 
> > I don't think web_frame->securityOrigin().isNull() would ever be null. 
Local
> > frames should have a Document with a SecurityOrigin, and RemoteFrames get
> their
> > securityOrigin initialized at creation time.
> > 
> > Yes, securityOrigin().toString() can be the string "null" for unique
origins. 
> > You may want to check for that with securityOrigin().isUnique().  
> > 
> > Do you need to handle file origins?  Those are returned as "file://" from
> > WebSecurityOrigin::toString() and I don't know if your comparison on the
> browser
> > side does the right thing for that.
> Alex and I found that we get "null" for file:// URLs as
> enforceFilePathSeparation is
> always turned on.
> 
> 
> However, I've looked at the complete code path and found some more info:
> 
> 1)
> The PluginServiceFilter::IsPluginAvailable() call should be fine b/c
> the RestrictedPluginPair seems to only care about chrome extension URLs.
> 
> 2)
> However there is another code path that goes to ContentSettingsMap:
>    PluginInfoMessageFilter::OnGetPluginInfo
>      -> PluginInfoMessageFilter::PluginsLoaded
>        -> PluginInfoMessageFilter::Contex::DecidePluginStatus(top_origin_url)
>          ->
PluginInfoMessageFilter::Context::GetPluginContentSetting(policy_url
> = top_origin_url)
>            -> HostContentSettingsMap::GetWebsiteSetting(primary_url =
> policy_url, sometimes secondary_url = policy_url, content_type =
> JAVASCRIPT/PLUGINS)
>              -> HostContentSettingsMap::ShouldAllowAllContent(primary_url,
> secondary_url)
>                calls primary_url.SchemeIsSecure() to return true.
>                calls primary_url.SchemeIs(chrome-extension)
>                  calls secondary_url.SchemeIs(chrome-extension)
>                calls primary_url.SchemeIs(devtools)
>                calls primary_url.SchemeIs(chrome-ui)
>              -> HostContentSettingsMap::GetWebsiteSettingInternal(primary_url,
> secondary_url)
>                -> content_settings::GetContentSettingValueAndPatterns
>                  calls ContentSettingsPattern::Matches()
> 
> This gets more involving, e.g.
> Matches() looks for file scheme and reads inner_url() of the GURL:
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/content...
> and then it also looks the file path:
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/content...
> I stopped here and have not looked further.
> 
> In theory though, content_settings stuff should be dealing *mostly* /w
origins,
> right?
> But lack of file:// path seems like a prob for now.
> This TODO suggests we should remove the "path" thing from
> ContentSettingsPattern::PatternParts:
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/content...
> 
> 
> To wrap up:
> 1) Top level urls that have uniqueOrigin/"null" is a problem if we want
> to feed the origin to HostContentSettingsMap::GetWebsiteSettings()
> or we have to prove they should work OK in uniqueOrigin case.
> 
> 2) The top level file:// URL becomes "null" is a problem b/c we
> can't do special treatment to file URLs.
> 
> 3) ContentSettingsPattern should not contain PatternParts::path.

Thanks for investigating.  Is there someone who works on ContentSettings that we
can ask about this?

I'm still arguing that we should not replicate the full URL for RemoteFrames if
at all possible.

[alexmos, 2014-12-12 20:19:52]

On 2014/12/12 18:29:12, Charlie Reis wrote:
> On 2014/12/12 02:15:35, lazyboy wrote:
> > On 2014/12/10 00:36:08, alexmos wrote:
> > > On 2014/12/09 23:36:11, Charlie Reis wrote:
> > > > [+alexmos]
> > > > 
> > > > > Open questions:
> > > > > a. The top origin url param in ChromeViewHostMsg_GetPluginInfo can be
> > > > >  changed to be of type base::string16 instead of GURL.
> > > > 
> > > > Hmm.  On one hand, it might be nice to convert the IPC message to use
> > > > url::Origin rather than GURL, similar to how Alex passes it in the
origin
> > > > replication CL:
> > > >
> > >
> >
>
https://codereview.chromium.org/692973005/diff/300001/content/common/frame_me...
> > > > 
> > > > However, the end use case pulls out the scheme, host, and port in the
> > browser
> > > > process, which requires a GURL:
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/plu...
> > > > 
> > > > Either we need a way to convert Origin to GURL, or we should convert
> > > > ChromePluginServiceFilter's RestrictedPluginPair to use Origin instead
of
> > GURL
> > > > (and thus we could just do a straight Origin comparison).
> > > > 
> > > > Alex, does that sound right?
> > > 
> > > Yes.  I think using either GURLs or url::Origins seems OK here, but it's
> > > important to stick with one throughout.  For example, GURL::GetOrigin can
be
> > > incompatible with   WebSecurityOrigin::toString() - GURL::GetOrigin() has
a
> > > trailing slash (which violates RFC 6454), but
WebSecurityOrigin::toString()
> > does
> > > not, so you wouldn't want to compare them.  If you already have other
places
> > > using GURLs to deal with origins, I think the current GURL approach is
fine.
> 
> > If
> > > this is isolated, using url::Origins throughout may better express that
> you're
> > > passing origins and not arbitrary URLs. 
> > > 
> > > > 
> > > > > b. What happens if one of the call to
> > > frame->top()->securityOrigin().isNull()
> > > > >  == true?  Is that possible?
> > > > 
> > > > The security origin may be serialized to a string as "null" if it's a
> unique
> > > > origin, but I don't know if securityOrigin().isNull() can ever return
> true. 
> > > > Alex?
> > > 
> > > I don't think web_frame->securityOrigin().isNull() would ever be null. 
> Local
> > > frames should have a Document with a SecurityOrigin, and RemoteFrames get
> > their
> > > securityOrigin initialized at creation time.
> > > 
> > > Yes, securityOrigin().toString() can be the string "null" for unique
> origins. 
> > > You may want to check for that with securityOrigin().isUnique().  
> > > 
> > > Do you need to handle file origins?  Those are returned as "file://" from
> > > WebSecurityOrigin::toString() and I don't know if your comparison on the
> > browser
> > > side does the right thing for that.
> > Alex and I found that we get "null" for file:// URLs as
> > enforceFilePathSeparation is
> > always turned on.
> > 
> > 
> > However, I've looked at the complete code path and found some more info:
> > 
> > 1)
> > The PluginServiceFilter::IsPluginAvailable() call should be fine b/c
> > the RestrictedPluginPair seems to only care about chrome extension URLs.
> > 
> > 2)
> > However there is another code path that goes to ContentSettingsMap:
> >    PluginInfoMessageFilter::OnGetPluginInfo
> >      -> PluginInfoMessageFilter::PluginsLoaded
> >        ->
PluginInfoMessageFilter::Contex::DecidePluginStatus(top_origin_url)
> >          ->
> PluginInfoMessageFilter::Context::GetPluginContentSetting(policy_url
> > = top_origin_url)
> >            -> HostContentSettingsMap::GetWebsiteSetting(primary_url =
> > policy_url, sometimes secondary_url = policy_url, content_type =
> > JAVASCRIPT/PLUGINS)
> >              -> HostContentSettingsMap::ShouldAllowAllContent(primary_url,
> > secondary_url)
> >                calls primary_url.SchemeIsSecure() to return true.
> >                calls primary_url.SchemeIs(chrome-extension)
> >                  calls secondary_url.SchemeIs(chrome-extension)
> >                calls primary_url.SchemeIs(devtools)
> >                calls primary_url.SchemeIs(chrome-ui)
> >              ->
HostContentSettingsMap::GetWebsiteSettingInternal(primary_url,
> > secondary_url)
> >                -> content_settings::GetContentSettingValueAndPatterns
> >                  calls ContentSettingsPattern::Matches()
> > 
> > This gets more involving, e.g.
> > Matches() looks for file scheme and reads inner_url() of the GURL:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/content...
> > and then it also looks the file path:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/content...
> > I stopped here and have not looked further.
> > 
> > In theory though, content_settings stuff should be dealing *mostly* /w
> origins,
> > right?
> > But lack of file:// path seems like a prob for now.
> > This TODO suggests we should remove the "path" thing from
> > ContentSettingsPattern::PatternParts:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/content...
> > 
> > 
> > To wrap up:
> > 1) Top level urls that have uniqueOrigin/"null" is a problem if we want
> > to feed the origin to HostContentSettingsMap::GetWebsiteSettings()
> > or we have to prove they should work OK in uniqueOrigin case.
> > 
> > 2) The top level file:// URL becomes "null" is a problem b/c we
> > can't do special treatment to file URLs.
> > 
> > 3) ContentSettingsPattern should not contain PatternParts::path.
> 
> Thanks for investigating.  Is there someone who works on ContentSettings that
we
> can ask about this?
> 
> I'm still arguing that we should not replicate the full URL for RemoteFrames
if
> at all possible.

Just wondering -- it seems that in this case the renderer doesn't actually use
the top frame's URL for anything other than passing it to the browser process. 
Could we retrieve the top frame's URL on the browser side, from
PluginInfoMessageFilter::OnGetPluginInfo, rather than sending it in the message?
 For example, could OnGetPluginInfo get the RenderFrameHost that's receiving the
message, walk the tree using GetParent(), and then use GetLastCommittedURL()?

[Charlie Reis, 2014-12-12 20:28:29]

On 2014/12/12 20:19:52, alexmos wrote:
> Just wondering -- it seems that in this case the renderer doesn't actually use
> the top frame's URL for anything other than passing it to the browser process.

> Could we retrieve the top frame's URL on the browser side, from
> PluginInfoMessageFilter::OnGetPluginInfo, rather than sending it in the
message?
>  For example, could OnGetPluginInfo get the RenderFrameHost that's receiving
the
> message, walk the tree using GetParent(), and then use GetLastCommittedURL()?

I would love to get the data from the browser process-- that would be far
preferable, since the renderer could lie about it.  The catch here is that we're
on the IO thread in this code, and there's a TOCTOU issue with hopping over to
the UI thread to find out the last committed URL.

It may still be worthwhile to route the message to RenderFrameHost to begin
with, grab the last committed URL, and then post a task to the IO thread to
complete the rest of the work.  It's a bit slower, but that would give us data
we could trust.