Implement createfailed event for <extensionoptions>

Implement createfailed event for <extensionoptions>

This CL adds an event to the <extensionoptions> tag that fires
if the guest view is attempted to be created with an invalid
extension id.

BUG=386838
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288201

[ericzeng, 2014-07-30 03:31:30]

PTAL, I will be adding a few more tests tomorrow to test cases where the
extension attribute in <extensionoptions> is invalid.

[Fady Samuel, 2014-07-30 16:02:59]

Are there security concerns here? Could not an app detect which apps are
installed by plugging in a lot of extensionIDs to a bunch of <extensionoptions>
elements?

[ericzeng, 2014-07-30 17:14:46]

On 2014/07/30 16:02:59, Fady Samuel wrote:
> Are there security concerns here? Could not an app detect which apps are
> installed by plugging in a lot of extensionIDs to a bunch of
<extensionoptions>
> elements?

This event should be fired in any case where the extension id attribute is not
the
id of the embedder extension. Ids of installed extensions, not installed
extensions,
and random letters should all fire the event. I'm working on tests for all of
these
cases.

[not at google - send to devlin, 2014-07-30 17:17:26]

On 2014/07/30 17:14:46, Eric Zeng wrote:
> On 2014/07/30 16:02:59, Fady Samuel wrote:
> > Are there security concerns here? Could not an app detect which apps are
> > installed by plugging in a lot of extensionIDs to a bunch of
> <extensionoptions>
> > elements?
> 
> This event should be fired in any case where the extension id attribute is not
> the
> id of the embedder extension. Ids of installed extensions, not installed
> extensions,
> and random letters should all fire the event. I'm working on tests for all of
> these
> cases.

Good point re using this to fingerprint. It's ok at the moment since this tag
should only actually work for webui and embedding yourself. If/when we allow
other extensions to use this, in a non-whitelisted manner, we could revisit.

[not at google - send to devlin, 2014-07-30 17:33:10]

https://codereview.chromium.org/429763002/diff/20001/chrome/browser/guest_vie...
File chrome/browser/guest_view/extension_options/extension_options_apitest.cc
(right):

https://codereview.chromium.org/429763002/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/extension_options/extension_options_apitest.cc:38:
FeatureSwitch::ScopedOverride enable_options(
you might as well make the feature switch a scoped_ptr<> private member of the
ExtensionOptionsApiTest, initialized in SetUpCommandLine, so you don't need to
spell it out all the time.

https://codereview.chromium.org/429763002/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/extension_options/extension_options_apitest.cc:44:
const Extension* embedded = InstallExtension(dir.AppendASCII("embedded"), 1);
you don't actually use |embedder| or |embedded| so you can do

ASSERT_TRUE(InstallExtenssion(dir.AppendASCII("embedder"), 1)

etc.

https://codereview.chromium.org/429763002/diff/20001/chrome/browser/guest_vie...
File chrome/browser/guest_view/extension_options/extension_options_guest.cc
(right):

https://codereview.chromium.org/429763002/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:68: if
(extension_id.empty()) {
this condition is actually covered by the one on line 72.

https://codereview.chromium.org/429763002/diff/20001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/extension_options.js (right):

https://codereview.chromium.org/429763002/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/extension_options.js:19:
this.setupEventProperty('createfailed');
i'm confused, where is the 'load' even set up in all of this?

https://codereview.chromium.org/429763002/diff/20001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/extension_options/embed_other/embedder/manifest.json
(right):

https://codereview.chromium.org/429763002/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/extension_options/embed_other/embedder/manifest.json:5:
"homepage_url": "http://example.com",
why do you need to specify a homepage url?

https://codereview.chromium.org/429763002/diff/20001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/extension_options/embed_other/embedder/test.js
(right):

https://codereview.chromium.org/429763002/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/extension_options/embed_other/embedder/test.js:8:
extensionoptions.addEventListener('createfailed', function() {
you should probably wrap this in a pass() or at least a callbackAdded().

https://codereview.chromium.org/429763002/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/extension_options/embed_other/embedder/test.js:14:
chrome.test.succeed();
and don't forget to remove it in a finally clause. that'd be easier if there
were a single assertion. perhaps:

var optionsPages = chrome.extensions.getViews().filter(function(view) {
  return view.document.location.pathname == '/options.html';
});
try {
  chrome.test.assertEq([], optionsPages);
} finally {
  document.body.removeChild(extensionoptions);
}

though... really this test isn't so interesting right? you should ideally be
sending that extension a message "do you have any options pages?"

https://codereview.chromium.org/429763002/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/extension_options/embed_other/embedder/test.js:18:
document.body.appendChild(extensionoptions);
also remove it.

[ericzeng, 2014-08-01 17:13:26]

https://codereview.chromium.org/429763002/diff/20001/chrome/browser/guest_vie...
File chrome/browser/guest_view/extension_options/extension_options_apitest.cc
(right):

https://codereview.chromium.org/429763002/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/extension_options/extension_options_apitest.cc:38:
FeatureSwitch::ScopedOverride enable_options(
On 2014/07/30 17:33:10, kalman wrote:
> you might as well make the feature switch a scoped_ptr<> private member of the
> ExtensionOptionsApiTest, initialized in SetUpCommandLine, so you don't need to
> spell it out all the time.

Done.

https://codereview.chromium.org/429763002/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/extension_options/extension_options_apitest.cc:44:
const Extension* embedded = InstallExtension(dir.AppendASCII("embedded"), 1);
On 2014/07/30 17:33:10, kalman wrote:
> you don't actually use |embedder| or |embedded| so you can do
> 
> ASSERT_TRUE(InstallExtenssion(dir.AppendASCII("embedder"), 1)
> 
> etc.

Done.

https://codereview.chromium.org/429763002/diff/20001/chrome/browser/guest_vie...
File chrome/browser/guest_view/extension_options/extension_options_guest.cc
(right):

https://codereview.chromium.org/429763002/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:68: if
(extension_id.empty()) {
On 2014/07/30 17:33:10, kalman wrote:
> this condition is actually covered by the one on line 72.
Removed.

https://codereview.chromium.org/429763002/diff/20001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/extension_options.js (right):

https://codereview.chromium.org/429763002/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/extension_options.js:19:
this.setupEventProperty('createfailed');
On 2014/07/30 17:33:10, kalman wrote:
> i'm confused, where is the 'load' even set up in all of this?

setupEventProperty() is called on 'load' in extension_options_events.js.
'createfailed' is setup in this file because in the case that there was an error
in createGuest, it lets the createGuest callback fire the event instead of C++
code in CreateWebContents.

https://codereview.chromium.org/429763002/diff/20001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/extension_options/embed_other/embedder/manifest.json
(right):

https://codereview.chromium.org/429763002/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/extension_options/embed_other/embedder/manifest.json:5:
"homepage_url": "http://example.com",
On 2014/07/30 17:33:10, kalman wrote:
> why do you need to specify a homepage url?
I don't

[not at google - send to devlin, 2014-08-04 19:12:03]

lgtm, but wait for fsamuel@.

https://codereview.chromium.org/429763002/diff/20001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/extension_options.js (right):

https://codereview.chromium.org/429763002/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/extension_options.js:19:
this.setupEventProperty('createfailed');
On 2014/08/01 17:13:26, ericzeng wrote:
> On 2014/07/30 17:33:10, kalman wrote:
> > i'm confused, where is the 'load' even set up in all of this?
> 
> setupEventProperty() is called on 'load' in extension_options_events.js.
> 'createfailed' is setup in this file because in the case that there was an
error
> in createGuest, it lets the createGuest callback fire the event instead of C++
> code in CreateWebContents. 

Ah, makes sense. Could you explain that in a comment?

https://codereview.chromium.org/429763002/diff/60001/chrome/browser/guest_vie...
File chrome/browser/guest_view/extension_options/extension_options_apitest.cc
(right):

https://codereview.chromium.org/429763002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/extension_options/extension_options_apitest.cc:24:
ExtensionApiTest::SetUpCommandLine(command_line);
ideally call the superclass SetUpCommandLine first, since that's the way that
construction order goes.

https://codereview.chromium.org/429763002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/extension_options/extension_options_apitest.cc:50:
std::stringstream script;
don't use stringstream. It's just as easy to construct this like

std::string script = base::StringPrintf(
    "chrome.storage.local.set({embeddedId: '%s}, function() {"
    "  window.domAutomationController.send('done injecting');"
    "});", embedded->id());

or thereabouts.

https://codereview.chromium.org/429763002/diff/60001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/extension_options.js (right):

https://codereview.chromium.org/429763002/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/extensions/extension_options.js:56: {bubbles:true,
supportsListeners: true, supportsFilters: true});
IMO these types of multi-parameter object arguments look better like:

var createFailedEvent = new Event('createfailed', {
  bubbles: true,
  supportsListeners: true,
  supportsFilters: true
});

https://codereview.chromium.org/429763002/diff/60001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/extension_options/embed_other/embedded/manifest.json
(right):

https://codereview.chromium.org/429763002/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/extension_options/embed_other/embedded/manifest.json:6:
"page": "background.html"
you should be able to do

"scripts": ["background.js"]

and not even need a background.html file?

https://codereview.chromium.org/429763002/diff/60001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/extension_options/embed_other/embedder/manifest.json
(right):

https://codereview.chromium.org/429763002/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/extension_options/embed_other/embedder/manifest.json:6:
"page": "background.html"
I'd say "likewise" but background.js doesn't actually do anything. do you need a
background page/script for this to work?

https://codereview.chromium.org/429763002/diff/60001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/extension_options/embed_other/embedder/test.js
(right):

https://codereview.chromium.org/429763002/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/extension_options/embed_other/embedder/test.js:7:
chrome.storage.local.get('embeddedId', function(items) {
wrap function(items) in a chrome.test.callbackPass - and the idiomatic way to do
that is for "var pass = chrome.test.callbackPass" at the top, then use
"pass(function(items) {.." here.

[ericzeng, 2014-08-04 21:15:34]

https://codereview.chromium.org/429763002/diff/60001/chrome/browser/guest_vie...
File chrome/browser/guest_view/extension_options/extension_options_apitest.cc
(right):

https://codereview.chromium.org/429763002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/extension_options/extension_options_apitest.cc:24:
ExtensionApiTest::SetUpCommandLine(command_line);
On 2014/08/04 19:12:03, kalman wrote:
> ideally call the superclass SetUpCommandLine first, since that's the way that
> construction order goes.

Done.

https://codereview.chromium.org/429763002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/extension_options/extension_options_apitest.cc:50:
std::stringstream script;
On 2014/08/04 19:12:03, kalman wrote:
> don't use stringstream. It's just as easy to construct this like
> 
> std::string script = base::StringPrintf(
>     "chrome.storage.local.set({embeddedId: '%s}, function() {"
>     "  window.domAutomationController.send('done injecting');"
>     "});", embedded->id());
> 
> or thereabouts.

Done.

https://codereview.chromium.org/429763002/diff/60001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/extension_options.js (right):

https://codereview.chromium.org/429763002/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/extensions/extension_options.js:56: {bubbles:true,
supportsListeners: true, supportsFilters: true});
On 2014/08/04 19:12:03, kalman wrote:
> IMO these types of multi-parameter object arguments look better like:
> 
> var createFailedEvent = new Event('createfailed', {
>   bubbles: true,
>   supportsListeners: true,
>   supportsFilters: true
> });

Done.

https://codereview.chromium.org/429763002/diff/60001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/extension_options/embed_other/embedded/manifest.json
(right):

https://codereview.chromium.org/429763002/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/extension_options/embed_other/embedded/manifest.json:6:
"page": "background.html"
On 2014/08/04 19:12:03, kalman wrote:
> you should be able to do
> 
> "scripts": ["background.js"]
> 
> and not even need a background.html file?

Done.

https://codereview.chromium.org/429763002/diff/60001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/extension_options/embed_other/embedder/manifest.json
(right):

https://codereview.chromium.org/429763002/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/extension_options/embed_other/embedder/manifest.json:6:
"page": "background.html"
On 2014/08/04 19:12:03, kalman wrote:
> I'd say "likewise" but background.js doesn't actually do anything. do you need
a
> background page/script for this to work?

This is kind of a hack so that the embedded extension's id can be passed to this
extension at runtime. The API test needs to run ExecuteScriptInBackgroundPage to
store the embedded extension's id in the embedder extension's local storage, and
that won't work without a background page.

https://codereview.chromium.org/429763002/diff/60001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/extension_options/embed_other/embedder/test.js
(right):

https://codereview.chromium.org/429763002/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/extension_options/embed_other/embedder/test.js:7:
chrome.storage.local.get('embeddedId', function(items) {
On 2014/08/04 19:12:03, kalman wrote:
> wrap function(items) in a chrome.test.callbackPass - and the idiomatic way to
do
> that is for "var pass = chrome.test.callbackPass" at the top, then use
> "pass(function(items) {.." here.

Done.

[not at google - send to devlin, 2014-08-04 21:16:49]

https://codereview.chromium.org/429763002/diff/60001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/extension_options/embed_other/embedder/manifest.json
(right):

https://codereview.chromium.org/429763002/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/extension_options/embed_other/embedder/manifest.json:6:
"page": "background.html"
On 2014/08/04 21:15:34, ericzeng wrote:
> On 2014/08/04 19:12:03, kalman wrote:
> > I'd say "likewise" but background.js doesn't actually do anything. do you
need
> a
> > background page/script for this to work?
> 
> This is kind of a hack so that the embedded extension's id can be passed to
this
> extension at runtime. The API test needs to run ExecuteScriptInBackgroundPage
to
> store the embedded extension's id in the embedder extension's local storage,
and
> that won't work without a background page. 

Ok cool. Add a comment then.

[ericzeng, 2014-08-04 21:32:22]

https://codereview.chromium.org/429763002/diff/60001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/extension_options/embed_other/embedder/manifest.json
(right):

https://codereview.chromium.org/429763002/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/extension_options/embed_other/embedder/manifest.json:6:
"page": "background.html"
On 2014/08/04 21:16:48, kalman wrote:
> On 2014/08/04 21:15:34, ericzeng wrote:
> > On 2014/08/04 19:12:03, kalman wrote:
> > > I'd say "likewise" but background.js doesn't actually do anything. do you
> need
> > a
> > > background page/script for this to work?
> > 
> > This is kind of a hack so that the embedded extension's id can be passed to
> this
> > extension at runtime. The API test needs to run
ExecuteScriptInBackgroundPage
> to
> > store the embedded extension's id in the embedder extension's local storage,
> and
> > that won't work without a background page. 
> 
> Ok cool. Add a comment then.

Done.

[ericzeng, 2014-08-06 21:21:32]

Could both of you take a look at this again? I added some
more tests to check the cases where the event should
be fired.

[Fady Samuel, 2014-08-06 21:46:45]

https://codereview.chromium.org/429763002/diff/160001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/extension_options.js (right):

https://codereview.chromium.org/429763002/diff/160001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/extension_options.js:64: supportsFilters:
true
supportsListeners and supportsFilters is not relevant for DOM events.

[not at google - send to devlin, 2014-08-06 21:51:10]

tests lgtm

https://codereview.chromium.org/429763002/diff/160001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/extension_options/embed_invalid/test.js
(right):

https://codereview.chromium.org/429763002/diff/160001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/extension_options/embed_invalid/test.js:34:
document.body.appendChild(extensionoptions);
these are basically the same test. you could have a function outside here like
testCannotEmbedExtensionId then call that firstly with 'thisisprobably...' and
secondly with chrome.runtime.id.

[ericzeng, 2014-08-06 23:49:24]

https://codereview.chromium.org/429763002/diff/160001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/extension_options.js (right):

https://codereview.chromium.org/429763002/diff/160001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/extension_options.js:64: supportsFilters:
true
On 2014/08/06 21:46:44, Fady Samuel wrote:
> supportsListeners and supportsFilters is not relevant for DOM events.

Done.

https://codereview.chromium.org/429763002/diff/160001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/extension_options/embed_invalid/test.js
(right):

https://codereview.chromium.org/429763002/diff/160001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/extension_options/embed_invalid/test.js:34:
document.body.appendChild(extensionoptions);
On 2014/08/06 21:51:10, kalman wrote:
> these are basically the same test. you could have a function outside here like
> testCannotEmbedExtensionId then call that firstly with 'thisisprobably...' and
> secondly with chrome.runtime.id.

Done.

[Fady Samuel, 2014-08-07 20:17:00]

lgtm

[ericzeng, 2014-08-07 20:18:13]

The CQ bit was checked by ericzeng@chromium.org

[commit-bot: I haz the power, 2014-08-07 20:26:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ericzeng@chromium.org/429763002/220001

[commit-bot: I haz the power, 2014-08-08 01:45:16]

Change committed as 288201