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

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

This helps catch bugs where an app page is rendered but has not been
activated, which means it will not be given the correct context (styling
and APIs). This would catch bugs such as one previously committed with
launcher pages (see http://crbug.com/391137).

BUG=391137

[Matt Giuca, 2014-07-04 04:11:41]

Hi Ben,

I'm splitting this out of https://codereview.chromium.org/365013003/ (there is a
small existing discussion with benwells on that CL).

Repeating myself from that CL:

The DCHECK is good -- I *think* -- but I would like you (kalman) to think over
it because I don't understand this stuff well (though I talked with sammc about
it). I initially had this DCHECK for all extensions, but it crashed when an
extension tried to use a content script. Sam also told me that if you run an
extension in an iframe, it would not be activated.

It seems like for extensions, adding a script context without activating it is
by design -- it means you are running the extension in a restricted context.
Whereas for platform apps, it is always a bug, because the app won't have access
to its own APIs. So it seems logical to perform this DCHECK if and only if the
extension is actually a platform app. WDYT?

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

lgtm

https://codereview.chromium.org/371563005/diff/1/extensions/renderer/dispatch...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/371563005/diff/1/extensions/renderer/dispatch...
extensions/renderer/dispatcher.cc:268: DCHECK(IsExtensionActive(extension_id));
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]

Thanks. Note: I can't submit this until
https://codereview.chromium.org/365013003/ is submitted (because it would fail
this DCHECK).

https://codereview.chromium.org/371563005/diff/1/extensions/renderer/dispatch...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/371563005/diff/1/extensions/renderer/dispatch...
extensions/renderer/dispatcher.cc:268: DCHECK(IsExtensionActive(extension_id));
On 2014/07/07 14:45:55, kalman wrote:
> 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.

Done.

[Matt Giuca, 2014-07-11 03:54:04]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-07-11 03:55:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mgiuca@chromium.org/371563005/20001

[commit-bot: I haz the power, 2014-07-11 08:07:38]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-11 09:21:07]

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

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

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[Matt Giuca, 2014-07-16 01:53:35]

Unfortunately, it looks like LOTS of browser tests are failing because they are
setting up platform apps without "activating" them.

(I've tried rolling back the last patch set, so we're testing for activation and
not the full blessed context, but they still fail.)

It's not clear whether this means I should fix the test environment (potentially
a lot of work), or just abandon this DCHECK.

[not at google - send to devlin, 2014-07-16 16:22:46]

On 2014/07/16 01:53:35, Matt Giuca wrote:
> Unfortunately, it looks like LOTS of browser tests are failing because they
are
> setting up platform apps without "activating" them.
> 
> (I've tried rolling back the last patch set, so we're testing for activation
and
> not the full blessed context, but they still fail.)
> 
> It's not clear whether this means I should fix the test environment
(potentially
> a lot of work), or just abandon this DCHECK.

Well it actually just looks like webview tests that are failing. For some reason
those are being detected as content script contexts... I'd guess that this is
due to the <webview>.executeScript call
(https://developer.chrome.com/apps/tags/webview#method-executeScript) which is
creating a content script context for that extension.

You can probably make that DCHECK include this case.

[lazyboy, 2014-07-21 20:24:50]

https://chromiumcodereview.appspot.com/371563005/diff/20001/extensions/render...
File extensions/renderer/dispatcher.cc (right):

https://chromiumcodereview.appspot.com/371563005/diff/20001/extensions/render...
extensions/renderer/dispatcher.cc:271:
DCHECK_EQ(Feature::BLESSED_EXTENSION_CONTEXT, context_type);
@kalman, so you're suggesting to add
if (!extension->permissions_data()->HasAPIPermission(..kWebView)) to this
DCHECK?

[not at google - send to devlin, 2014-07-21 20:34:09]

https://chromiumcodereview.appspot.com/371563005/diff/20001/extensions/render...
File extensions/renderer/dispatcher.cc (right):

https://chromiumcodereview.appspot.com/371563005/diff/20001/extensions/render...
extensions/renderer/dispatcher.cc:271:
DCHECK_EQ(Feature::BLESSED_EXTENSION_CONTEXT, context_type);
On 2014/07/21 20:24:50, lazyboy wrote:
> @kalman, so you're suggesting to add
> if (!extension->permissions_data()->HasAPIPermission(..kWebView)) to this
> DCHECK?

That would work, or a check like isWebviewProcess() if such a thing exists. plus
a check to make sure that it is indeed a content script context being created?

[lazyboy, 2014-07-21 22:03:20]

https://chromiumcodereview.appspot.com/371563005/diff/20001/extensions/render...
File extensions/renderer/dispatcher.cc (right):

https://chromiumcodereview.appspot.com/371563005/diff/20001/extensions/render...
extensions/renderer/dispatcher.cc:271:
DCHECK_EQ(Feature::BLESSED_EXTENSION_CONTEXT, context_type);
On 2014/07/21 20:34:09, kalman wrote:
> On 2014/07/21 20:24:50, lazyboy wrote:
> > @kalman, so you're suggesting to add
> > if (!extension->permissions_data()->HasAPIPermission(..kWebView)) to this
> > DCHECK?
> 
> That would work, or a check like isWebviewProcess() if such a thing exists.
The process itself doesn't know much about he webview, but the script executor
has info about this,
ProgrammaticScriptInjector.params_.is_web_view, is that possible to reach to
that from here?
plus
> a check to make sure that it is indeed a content script context being created?
I have e.g. in WebViewTest.Shim_TestChromeExtensionURL, a <webview> that
contains a <script> tag, that's causing context_type to be UNBLESSED_*
guest_with_inline_script.html:
...
<script>window.console.log('...');</script>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/e...
So this is not a content script, right?

I wonder why we this script is run on the platform app, or is this expected?