Fix extension bindings injection for iframes (reland)

extensions/renderer/script_context.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "extensions/renderer/script_context.h"
 
 #include <memory>
 
 #include "base/command_line.h"
 #include "base/logging.h"
@@ skipping 91 matching lines @@
                              Feature::Context effective_context_type)
     : is_valid_(true),
       v8_context_(v8_context->GetIsolate(), v8_context),
       web_frame_(web_frame),
       extension_(extension),
       context_type_(context_type),
       effective_extension_(effective_extension),
       effective_context_type_(effective_context_type),
       safe_builtins_(this),
       isolate_(v8_context->GetIsolate()),
-      url_(web_frame_ ? GetDataSourceURLForFrame(web_frame_) : GURL()),
       runner_(new Runner(this)) {
   VLOG(1) << "Created context:\n" << GetDebugString();
   gin::PerContextData* gin_data = gin::PerContextData::From(v8_context);
   CHECK(gin_data);
   gin_data->set_runner(runner_.get());
+  if (web_frame_) {
+    const blink::WebURL& weburl = web_frame_->document().url();
+    if (weburl.isEmpty()) {
+      blink::WebDataSource* data_source = web_frame_->provisionalDataSource()
+                                          ? web_frame_->provisionalDataSource()
+                                          : web_frame_->dataSource();

[asargent_no_longer_on_chrome, 2016/08/03 00:22:24]

Note that there's a little duplication here with the code in
GetDataSourceURLForFrame - I didn't want to just re-use GetDataSourceURLForFrame
as-is since this code runs on every frame creation and I wanted to avoid the
overhead of taking a blink::WebURL, converting it to a GURL to return from
GetDataSourceURLForFrame, and then immediately converting it back to a WebURL to
do the security origin test. I suppose I could refactor things to have a common
method that returns a blink::WebURL and then use that in both places. Let me
know what you think. 

[Devlin, 2016/08/03 16:16:49]

This also looks very similar to the new logic added in ScriptContextSet.  In
both cases we:
Get the document url
if that's empty, check the data source url
if the data source url is non-empty and the frame can access it, use the data
source url

This makes me think we should probably have a method at least to do that -
something like GetAccessCheckedFrameURL() (strawman name) that we can use in
both places.  There may also be some uses of GetDataSourceURL that should be
converted to that, but we can do that separately.

[asargent_no_longer_on_chrome, 2016/08/04 05:47:18]

Done.

+      if (data_source && web_frame_->getSecurityOrigin().canAccess(
+              blink::WebSecurityOrigin::create(data_source->request().url())))
+        url_ = GURL(data_source->request().url());
+    }
+    if (url_.is_empty())

[Devlin, 2016/08/03 16:16:49]

This can be an else, right?  Because we'll only set url_ above if weburl is
empty.

[asargent_no_longer_on_chrome, 2016/08/04 05:47:18]

Good point - I guess it doesn't help to assign to url_ from an empty weburl.
But, obsoleted by switching to a method. 

+      url_ = GURL(weburl);
+  }
 }
 
 ScriptContext::~ScriptContext() {
   VLOG(1) << "Destroyed context for extension\n"
           << "  extension id: " << GetExtensionID() << "\n"
           << "  effective extension id: "
           << (effective_extension_.get() ? effective_extension_->id() : "");
   CHECK(!is_valid_) << "ScriptContexts must be invalidated before destruction";
 }
 
@@ skipping 358 matching lines @@
     v8::Local<v8::Value> argv[]) {
   return context_->CallFunction(function, argc, argv);
 }
 
 gin::ContextHolder* ScriptContext::Runner::GetContextHolder() {
   v8::HandleScope handle_scope(context_->isolate());
   return gin::PerContextData::From(context_->v8_context())->context_holder();
 }
 
 }  // namespace extensions