Extensions: DCHECK that all app pages are blessed before use.

extensions/renderer/dispatcher.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/dispatcher.h"
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/command_line.h"
 #include "base/debug/alias.h"
@@ skipping 239 matching lines @@
     // "invalid". This isn't interesting.
     if (extension_id != "invalid") {
       LOG(ERROR) << "Extension \"" << extension_id << "\" not found";
       RenderThread::Get()->RecordAction(
           UserMetricsAction("ExtensionNotFound_ED"));
     }
 
     extension_id = "";
   }
 
+  bool is_within_platform_app = extension && extension->is_platform_app();
+
+  // If we are navigating to a platform app URL, the extension must first have
+  // been activated. If not, IsWithinPlatformApp will incorrectly return false,
+  // and we will be missing information about what features the app has.
+  // (Note: This DCHECK is invalid for non-app extensions, which do not get
+  // activated in "unblessed" contexts such as content scripts and iframes.)
+  if (is_within_platform_app)
+    DCHECK(IsExtensionActive(extension_id));

[not at google - send to devlin, 2014/07/07 14:45:55]

Good idea. A slightly stronger check would be to move the DCHECK to line 275 and
make it DCHECK_EQ(Feature::BLESSED_EXTENSION_CONTEXT, context_type) because an
extension being active doesn't necessarily imply that it's a blessed context (as
Sam already pointed out, iframes) - yet specifically all app contexts must be
privileged, at least in the current design where apps can't be hosted in
untrusted processes.

I think the comment could be just "Sanity check that all platform app contexts
are blessed".

as a nit I'd request you call the variable "is_platform_app_context" to avoid
mixing it up with IsWithinPlatformApp.

[Matt Giuca, 2014/07/10 08:04:56]

Done.

+
   Feature::Context context_type =
       ClassifyJavaScriptContext(extension,
                                 extension_group,
                                 ScriptContext::GetDataSourceURLForFrame(frame),
                                 frame->document().securityOrigin());
 
   ScriptContext* context =
       delegate_->CreateScriptContext(v8_context, frame, extension, context_type)
           .release();
   script_context_set_.Add(context);
@@ skipping 19 matching lines @@
   // lazily evalulate to Event from event_bindings.js. For extensions only
   // though, not all webpages!
   if (context->extension()) {
     v8::Handle<v8::Object> chrome = AsObjectOrEmpty(GetOrCreateChrome(context));
     if (!chrome.IsEmpty())
       module_system->SetLazyField(chrome, "Event", kEventBindings, "Event");
   }
 
   UpdateBindingsForContext(context);
 
-  bool is_within_platform_app = IsWithinPlatformApp();
   // Inject custom JS into the platform app context.
   if (is_within_platform_app) {
     module_system->Require("platformApp");
   }
 
   delegate_->RequireAdditionalModules(
       module_system, extension, context_type, is_within_platform_app);
 
   VLOG(1) << "Num tracked contexts: " << script_context_set_.size();
 }
@@ skipping 915 matching lines @@
     return v8::Handle<v8::Object>();
 
   if (bind_name)
     *bind_name = split.back();
 
   return bind_object.IsEmpty() ? AsObjectOrEmpty(GetOrCreateChrome(context))
                                : bind_object;
 }
 
 }  // namespace extensions