Signal extension API schema corruption to the browser process.

Signal extension API schema corruption to the browser process.

Extension API schemas are parsed from string resources in the browser
binary, so failure to parse them usually indicates a problem with the
browser installation.

Crashing the extension process that tried to parse the schema makes it
look to the user like the extension is at fault (or worse, in the case
of component extensions, some browser features will misbehave for no
obvious reason).

This CL adds an IPC to indicate schema load failure back to the browser
and has the browser attempt to load the schema itself, and crash on
failure, so that the user has some indication that the browser itself
is broken (providing a better UX for this case is under review!).

Thanks to imcheng@ for implementing the bulk of this!

BUG=634423, 639104, 121424
Committed: https://crrev.com/73b8107d339588f342d4f9a92888c9a05a3707b9
Cr-Commit-Position: refs/heads/master@{#413602}

[Wez, 2016-08-19 01:12:36]

wez@chromium.org changed reviewers:
+ jschuh@chromium.org, rdevlin.cronin@chromium.org

[Wez, 2016-08-19 01:12:36]

PTAL

I wonder if we can add a browser-test for this to verify that we correctly crash
if we find a corrupt schema? Would require browser death-test capability and
we'd need a known-corrupt schema, so likely only makes sense as a DEBUG mode
test.

WDYT?

[Wez, 2016-08-19 01:12:45]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 01:13:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Wez, 2016-08-19 01:13:25]

Description was changed from

==========
Signal extension API schema corruption to the browser process.

Extension API schemas are parsed from string resources in the browser
binary, so failure to parse them usually indicates a problem with the
browser installation.

Crashing the extension process that tried to parse the schema makes it
look to the user like the extension is at fault (or worse, in the case
of component extensions, some browser features will misbehave for no
obvious reason).

This CL adds an IPC to indicate schema load failure back to the browser
and has the browser attempt to load the schema itself, and crash on
failure, so that the user has some indication that the browser itself
is broken (providing a better UX for this case is under review!).

BUG=634423, 639104, 121424
==========

to

==========
Signal extension API schema corruption to the browser process.

Extension API schemas are parsed from string resources in the browser
binary, so failure to parse them usually indicates a problem with the
browser installation.

Crashing the extension process that tried to parse the schema makes it
look to the user like the extension is at fault (or worse, in the case
of component extensions, some browser features will misbehave for no
obvious reason).

This CL adds an IPC to indicate schema load failure back to the browser
and has the browser attempt to load the schema itself, and crash on
failure, so that the user has some indication that the browser itself
is broken (providing a better UX for this case is under review!).

Thanks to imcheng@ for implementing the bulk of this!

BUG=634423, 639104, 121424
==========

[Wez, 2016-08-19 01:15:21]

Given that the IPC to the browser may not actually trigger a crash, I wonder if
we should also have the renderer PostTask() LoadSchemaListOrDie() with a delay,
so that it'll still eventually crash as it would have before, even if the
browser somehow doesn't?

It may even make sense to add a UMA stat so we can compare extension vs browser
schema load failure rates?

[commit-bot: I haz the power, 2016-08-19 01:37:12]

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

[commit-bot: I haz the power, 2016-08-19 01:37:13]

Dry run: Exceeded global retry quota

[Devlin, 2016-08-19 15:02:33]

https://codereview.chromium.org/2254383002/diff/1/extensions/common/extension...
File extensions/common/extension_api.cc (right):

https://codereview.chromium.org/2254383002/diff/1/extensions/common/extension...
extensions/common/extension_api.cc:75: // From() returns null if pass a null or
non-list value.
nit: "passed"

https://codereview.chromium.org/2254383002/diff/1/extensions/common/extension...
extensions/common/extension_api.cc:362: std::string api_name =
GetAPINameFromFullName(api, NULL);
nullptr

https://codereview.chromium.org/2254383002/diff/1/extensions/common/extension...
extensions/common/extension_api.cc:366: if
(!extensions_client->IsAPISchemaGenerated(api))
We aren't checking the unloaded schemas here.  That won't be a problem after
https://codereview.chromium.org/2257003002/ (done for orthogonal performance
reasons), but I don't plan to merge that back, so this should probably handle
that case.

https://codereview.chromium.org/2254383002/diff/1/extensions/common/extension...
File extensions/common/extension_api.h (right):

https://codereview.chromium.org/2254383002/diff/1/extensions/common/extension...
extensions/common/extension_api.h:114: // Attempts to load the schema for |api|.
Called by the browser process when
nit: omit "called by the browser process...".  That should be obvious from call
sites, and this is bound to get out of date if we ever add more call sites. :)

[Devlin, 2016-08-19 15:04:26]

On 2016/08/19 01:12:36, Wez wrote:
> PTAL
> 
> I wonder if we can add a browser-test for this to verify that we correctly
crash
> if we find a corrupt schema? Would require browser death-test capability and
> we'd need a known-corrupt schema, so likely only makes sense as a DEBUG mode
> test.
> 
> WDYT?

Browser test sgtm. :)  I believe we do also have "check that we crash" test
capabilities, so I don't know that we need a debug-only test.  Though it might
be a little tricky to test otherwise, since dcheck is on with all trybots, so
the behavior might be a little tricky.

If you can add a reasonable test without a terrible amount of hassle, that'd be
great.  If it causes too much grief, we might be okay without it.

[Devlin, 2016-08-19 15:05:44]

On 2016/08/19 01:15:21, Wez wrote:
> Given that the IPC to the browser may not actually trigger a crash, I wonder
if
> we should also have the renderer PostTask() LoadSchemaListOrDie() with a
delay,
> so that it'll still eventually crash as it would have before, even if the
> browser somehow doesn't?
> 
> It may even make sense to add a UMA stat so we can compare extension vs
browser
> schema load failure rates?

My thought for this would be to kill the renderer with a bad IPC message,
because it would be indicative of a compromised renderer.  I *think* we may
already have UMA for which messages we kill with bad message, so that would also
serve as UMA for renderer vs browser parsing.

[Wez, 2016-08-19 18:19:33]

On 2016/08/19 15:04:26, Devlin wrote:
> On 2016/08/19 01:12:36, Wez wrote:
> > PTAL
> > 
> > I wonder if we can add a browser-test for this to verify that we correctly
> crash
> > if we find a corrupt schema? Would require browser death-test capability and
> > we'd need a known-corrupt schema, so likely only makes sense as a DEBUG mode
> > test.
> > 
> > WDYT?
> 
> Browser test sgtm. :)  I believe we do also have "check that we crash" test
> capabilities, so I don't know that we need a debug-only test.  Though it might
> be a little tricky to test otherwise, since dcheck is on with all trybots, so
> the behavior might be a little tricky.

We have EXPECT_DEATH() for synchronous activities in our tests, but I'm not
familiar with how async-crashes would be handled in a browser test - if you can
point me to an example, that'd be great.

DCHECK being on is fine - what matters is that we don't push builds with a
corrupt-for-testing schema in them out to production.

> If you can add a reasonable test without a terrible amount of hassle, that'd
be
> great.  If it causes too much grief, we might be okay without it.

If we have an example of a browser death test then I can give it a try.

[Wez, 2016-08-19 18:20:42]

On 2016/08/19 15:05:44, Devlin wrote:
> On 2016/08/19 01:15:21, Wez wrote:
> > Given that the IPC to the browser may not actually trigger a crash, I wonder
> if
> > we should also have the renderer PostTask() LoadSchemaListOrDie() with a
> delay,
> > so that it'll still eventually crash as it would have before, even if the
> > browser somehow doesn't?
> > 
> > It may even make sense to add a UMA stat so we can compare extension vs
> browser
> > schema load failure rates?
> 
> My thought for this would be to kill the renderer with a bad IPC message,
> because it would be indicative of a compromised renderer.  I *think* we may
> already have UMA for which messages we kill with bad message, so that would
also
> serve as UMA for renderer vs browser parsing.

Hmmm, so have the IPC handler return/set an error result? That seems reasonable.

[Wez, 2016-08-19 18:22:16]

https://codereview.chromium.org/2254383002/diff/1/extensions/common/extension...
File extensions/common/extension_api.cc (right):

https://codereview.chromium.org/2254383002/diff/1/extensions/common/extension...
extensions/common/extension_api.cc:75: // From() returns null if pass a null or
non-list value.
On 2016/08/19 15:02:32, Devlin wrote:
> nit: "passed"

Done.

https://codereview.chromium.org/2254383002/diff/1/extensions/common/extension...
extensions/common/extension_api.cc:362: std::string api_name =
GetAPINameFromFullName(api, NULL);
On 2016/08/19 15:02:32, Devlin wrote:
> nullptr

Sticking with NULL for consistency with the rest of this file; we can migrate
NULL->nullptr as a follow-up.

https://codereview.chromium.org/2254383002/diff/1/extensions/common/extension...
extensions/common/extension_api.cc:366: if
(!extensions_client->IsAPISchemaGenerated(api))
On 2016/08/19 15:02:32, Devlin wrote:
> We aren't checking the unloaded schemas here.  That won't be a problem after
> https://codereview.chromium.org/2257003002/ (done for orthogonal performance
> reasons), but I don't plan to merge that back, so this should probably handle
> that case.

Ah, OK, so we need to check for un-generated (why do we call them "unloaded,
BTW?) schemas, which are string resources in the binary.

Looking at the code for that, I notice that things are expecting that
InitDefaultConfiguration() has been called - I don't think we need to worry
about that here (it always will have been called, and if it hasn't been called
then things will safely no-op). Am I missing something?

https://codereview.chromium.org/2254383002/diff/1/extensions/common/extension...
File extensions/common/extension_api.h (right):

https://codereview.chromium.org/2254383002/diff/1/extensions/common/extension...
extensions/common/extension_api.h:114: // Attempts to load the schema for |api|.
Called by the browser process when
On 2016/08/19 15:02:32, Devlin wrote:
> nit: omit "called by the browser process...".  That should be obvious from
call
> sites, and this is bound to get out of date if we ever add more call sites. :)

Done.

[Devlin, 2016-08-19 18:32:11]

On 2016/08/19 18:19:33, Wez wrote:
> On 2016/08/19 15:04:26, Devlin wrote:
> > On 2016/08/19 01:12:36, Wez wrote:
> > > PTAL
> > > 
> > > I wonder if we can add a browser-test for this to verify that we correctly
> > crash
> > > if we find a corrupt schema? Would require browser death-test capability
and
> > > we'd need a known-corrupt schema, so likely only makes sense as a DEBUG
mode
> > > test.
> > > 
> > > WDYT?
> > 
> > Browser test sgtm. :)  I believe we do also have "check that we crash" test
> > capabilities, so I don't know that we need a debug-only test.  Though it
might
> > be a little tricky to test otherwise, since dcheck is on with all trybots,
so
> > the behavior might be a little tricky.
> 
> We have EXPECT_DEATH() for synchronous activities in our tests, but I'm not
> familiar with how async-crashes would be handled in a browser test - if you
can
> point me to an example, that'd be great.
> 
> DCHECK being on is fine - what matters is that we don't push builds with a
> corrupt-for-testing schema in them out to production.
> 
> > If you can add a reasonable test without a terrible amount of hassle, that'd
> be
> > great.  If it causes too much grief, we might be okay without it.
> 
> If we have an example of a browser death test then I can give it a try.

Hmm... maybe EXPECT_DEATH() is what I was thinking of.  Hypothetically, that
might work if we could force the renderer to load a corrupted schema by doing an
ExecuteScriptAndExtract*() method, which would send an IPC, execute the script
(which loads the schema, which fails, sends the ipc to crash the browser, and
*then* responds with the value), and would fake synchronicity through run loops.
 Perhaps that's worth a try?

[Devlin, 2016-08-19 18:40:57]

https://codereview.chromium.org/2254383002/diff/1/extensions/common/extension...
File extensions/common/extension_api.cc (right):

https://codereview.chromium.org/2254383002/diff/1/extensions/common/extension...
extensions/common/extension_api.cc:362: std::string api_name =
GetAPINameFromFullName(api, NULL);
On 2016/08/19 18:22:15, Wez wrote:
> On 2016/08/19 15:02:32, Devlin wrote:
> > nullptr
> 
> Sticking with NULL for consistency with the rest of this file; we can migrate
> NULL->nullptr as a follow-up.

(Not blocking this change - feel free to ignore) I thought the decision was that
we preferred nullptr in all new code (even new code in existing files), but
didn't want to do mass updates (primarily for git-blame reasons)?  In which
case, putting nullptr here would be correct.

https://codereview.chromium.org/2254383002/diff/1/extensions/common/extension...
extensions/common/extension_api.cc:366: if
(!extensions_client->IsAPISchemaGenerated(api))
On 2016/08/19 18:22:15, Wez wrote:
> On 2016/08/19 15:02:32, Devlin wrote:
> > We aren't checking the unloaded schemas here.  That won't be a problem after
> > https://codereview.chromium.org/2257003002/ (done for orthogonal performance
> > reasons), but I don't plan to merge that back, so this should probably
handle
> > that case.
> 
> Ah, OK, so we need to check for un-generated (why do we call them "unloaded,
> BTW?) schemas, which are string resources in the binary.

re unloaded, no idea - the whole concept doesn't make sense (which is why I'm
killing them :))

> Looking at the code for that, I notice that things are expecting that
> InitDefaultConfiguration() has been called - I don't think we need to worry
> about that here (it always will have been called, and if it hasn't been called
> then things will safely no-op). Am I missing something?

InitDefaultConfiguration is called when we create the ExtensionAPI, so we should
be good.

https://codereview.chromium.org/2254383002/diff/20001/extensions/browser/exte...
File extensions/browser/extension_message_filter.cc (right):

https://codereview.chromium.org/2254383002/diff/20001/extensions/browser/exte...
extensions/browser/extension_message_filter.cc:326:
ExtensionAPI::GetSharedInstance()->LoadSchemaListOrDie(api);
Thinking about it, is there a reason to not just do:
const base::DictionaryValue* schema =
ExetnsionAPI::GetSharedInstance()->LoadSchema(api);
CHECK(schema);
KillBadMessageSender();

That way we wouldn't even need LoadSchemaListOrDie().

[Devlin, 2016-08-19 19:13:27]

https://codereview.chromium.org/2254383002/diff/20001/extensions/browser/exte...
File extensions/browser/extension_message_filter.cc (right):

https://codereview.chromium.org/2254383002/diff/20001/extensions/browser/exte...
extensions/browser/extension_message_filter.cc:326:
ExtensionAPI::GetSharedInstance()->LoadSchemaListOrDie(api);
On 2016/08/19 18:40:57, Devlin wrote:
> Thinking about it, is there a reason to not just do:
> const base::DictionaryValue* schema =
> ExetnsionAPI::GetSharedInstance()->LoadSchema(api);
> CHECK(schema);
> KillBadMessageSender();
> 
> That way we wouldn't even need LoadSchemaListOrDie().

(Another advantage to that is that then we don't need to worry about collisions
when we rip out unloaded_schemas)

[Wez, 2016-08-20 00:08:17]

On 2016/08/19 19:13:27, Devlin wrote:
>
https://codereview.chromium.org/2254383002/diff/20001/extensions/browser/exte...
> File extensions/browser/extension_message_filter.cc (right):
> 
>
https://codereview.chromium.org/2254383002/diff/20001/extensions/browser/exte...
> extensions/browser/extension_message_filter.cc:326:
> ExtensionAPI::GetSharedInstance()->LoadSchemaListOrDie(api);
> On 2016/08/19 18:40:57, Devlin wrote:
> > Thinking about it, is there a reason to not just do:
> > const base::DictionaryValue* schema =
> > ExetnsionAPI::GetSharedInstance()->LoadSchema(api);
> > CHECK(schema);
> > KillBadMessageSender();
> > 
> > That way we wouldn't even need LoadSchemaListOrDie().
> 
> (Another advantage to that is that then we don't need to worry about
collisions
> when we rip out unloaded_schemas)

I think you are thinking of GetSchema()?

Superficially yes that would work, GetSchema() does a lot of other stuff
internally, which makes it riskier to expose via an IPC. e.g. it seemingly
relies on the caller having previously ensured not to pass in a full-name for a
non-existent "child" API name - if a caller were able to do that with the
existing code then we'd return a null schema and V8 would check.

[Devlin, 2016-08-20 00:39:45]

On 2016/08/20 00:08:17, Wez wrote:
> On 2016/08/19 19:13:27, Devlin wrote:
> >
>
https://codereview.chromium.org/2254383002/diff/20001/extensions/browser/exte...
> > File extensions/browser/extension_message_filter.cc (right):
> > 
> >
>
https://codereview.chromium.org/2254383002/diff/20001/extensions/browser/exte...
> > extensions/browser/extension_message_filter.cc:326:
> > ExtensionAPI::GetSharedInstance()->LoadSchemaListOrDie(api);
> > On 2016/08/19 18:40:57, Devlin wrote:
> > > Thinking about it, is there a reason to not just do:
> > > const base::DictionaryValue* schema =
> > > ExetnsionAPI::GetSharedInstance()->LoadSchema(api);
> > > CHECK(schema);
> > > KillBadMessageSender();
> > > 
> > > That way we wouldn't even need LoadSchemaListOrDie().
> > 
> > (Another advantage to that is that then we don't need to worry about
> collisions
> > when we rip out unloaded_schemas)
> 
> I think you are thinking of GetSchema()?
> 
> Superficially yes that would work, GetSchema() does a lot of other stuff
> internally, which makes it riskier to expose via an IPC. e.g. it seemingly
> relies on the caller having previously ensured not to pass in a full-name for
a
> non-existent "child" API name - if a caller were able to do that with the
> existing code then we'd return a null schema and V8 would check.

Yeah, sorry, GetSchema().  Fair enough about the risk of passing a non-existent
schema, though I'm not sure if there's a ton of benefit for a compromised
renderer triggering a crash.

V2: GetAPINameFromFullName() already returns an implicit success indication if
the API exists (if it doesn't, it returns an empty std::string()).  What about:

std::string name = api->GetApiNameFromFullName();
const base::DictionaryValue* schema = nullptr;
if (name.empty() ||
    (schema = api->GetSchema(name)) != nullptr)
  KillBadMessageSender();  // Sent a non-existent API or the API loads fine.
else
  CHECK(schema);  // Trigger browser crash.

[Wez, 2016-08-20 01:07:51]

On 2016/08/20 00:39:45, Devlin wrote:
> On 2016/08/20 00:08:17, Wez wrote:
> > On 2016/08/19 19:13:27, Devlin wrote:
> > >
> >
>
https://codereview.chromium.org/2254383002/diff/20001/extensions/browser/exte...
> > > File extensions/browser/extension_message_filter.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2254383002/diff/20001/extensions/browser/exte...
> > > extensions/browser/extension_message_filter.cc:326:
> > > ExtensionAPI::GetSharedInstance()->LoadSchemaListOrDie(api);
> > > On 2016/08/19 18:40:57, Devlin wrote:
> > > > Thinking about it, is there a reason to not just do:
> > > > const base::DictionaryValue* schema =
> > > > ExetnsionAPI::GetSharedInstance()->LoadSchema(api);
> > > > CHECK(schema);
> > > > KillBadMessageSender();
> > > > 
> > > > That way we wouldn't even need LoadSchemaListOrDie().
> > > 
> > > (Another advantage to that is that then we don't need to worry about
> > collisions
> > > when we rip out unloaded_schemas)
> > 
> > I think you are thinking of GetSchema()?
> > 
> > Superficially yes that would work, GetSchema() does a lot of other stuff
> > internally, which makes it riskier to expose via an IPC. e.g. it seemingly
> > relies on the caller having previously ensured not to pass in a full-name
for
> a
> > non-existent "child" API name - if a caller were able to do that with the
> > existing code then we'd return a null schema and V8 would check.
> 
> Yeah, sorry, GetSchema().  Fair enough about the risk of passing a
non-existent
> schema, though I'm not sure if there's a ton of benefit for a compromised
> renderer triggering a crash.
> 
> V2: GetAPINameFromFullName() already returns an implicit success indication if
> the API exists (if it doesn't, it returns an empty std::string()).  What
about:
> 
> std::string name = api->GetApiNameFromFullName();
> const base::DictionaryValue* schema = nullptr;
> if (name.empty() ||
>     (schema = api->GetSchema(name)) != nullptr)
>   KillBadMessageSender();  // Sent a non-existent API or the API loads fine.
> else
>   CHECK(schema);  // Trigger browser crash.

Yeah, using GetAPINameForFullName() to verify that we _should_ be able to load
something makes sense.

[Wez, 2016-08-20 01:38:15]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-20 01:39:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Wez, 2016-08-20 01:40:40]

https://codereview.chromium.org/2254383002/diff/20001/extensions/browser/exte...
File extensions/browser/extension_message_filter.cc (right):

https://codereview.chromium.org/2254383002/diff/20001/extensions/browser/exte...
extensions/browser/extension_message_filter.cc:326:
ExtensionAPI::GetSharedInstance()->LoadSchemaListOrDie(api);
On 2016/08/19 18:40:57, Devlin wrote:
> Thinking about it, is there a reason to not just do:
> const base::DictionaryValue* schema =
> ExetnsionAPI::GetSharedInstance()->LoadSchema(api);
> CHECK(schema);
> KillBadMessageSender();
> 
> That way we wouldn't even need LoadSchemaListOrDie().

Done.

[Devlin, 2016-08-20 01:49:39]

I think this LGTM as long as bots are happy

https://codereview.chromium.org/2254383002/diff/40001/extensions/browser/exte...
File extensions/browser/extension_message_filter.cc (right):

https://codereview.chromium.org/2254383002/diff/40001/extensions/browser/exte...
extensions/browser/extension_message_filter.cc:337: // API name is valid, so
check that we can parse the schema.
I'd add to this comment to indicate that we *expect* this to fail in any
non-compromised state.

[commit-bot: I haz the power, 2016-08-20 02:09:53]

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

[commit-bot: I haz the power, 2016-08-20 02:09:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Wez, 2016-08-20 02:57:47]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-20 02:58:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-20 03:48:50]

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

[commit-bot: I haz the power, 2016-08-20 03:48:51]

Dry run: This issue passed the CQ dry run.

[Wez, 2016-08-21 00:53:16]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-21 00:53:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Wez, 2016-08-21 00:58:41]

wez@chromium.org changed reviewers:
+ dcheng@chromium.org
- jschuh@chromium.org

[Wez, 2016-08-21 00:58:41]

-jschuh, +dcheng

I started looking at doing a browser test, but couldn't find any simple example
of a basic extension-API browser test to crib from - any suggestions, Devlin?

https://codereview.chromium.org/2254383002/diff/40001/extensions/browser/exte...
File extensions/browser/extension_message_filter.cc (right):

https://codereview.chromium.org/2254383002/diff/40001/extensions/browser/exte...
extensions/browser/extension_message_filter.cc:337: // API name is valid, so
check that we can parse the schema.
On 2016/08/20 01:49:39, Devlin wrote:
> I'd add to this comment to indicate that we *expect* this to fail in any
> non-compromised state.

Done.

[commit-bot: I haz the power, 2016-08-21 01:42:21]

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

[commit-bot: I haz the power, 2016-08-21 01:42:22]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-08-22 14:52:13]

On 2016/08/21 00:58:41, Wez wrote:
> -jschuh, +dcheng
> 
> I started looking at doing a browser test, but couldn't find any simple
example
> of a basic extension-API browser test to crib from - any suggestions, Devlin?

Check out ExtensionBrowserTest, ExtensionApiTest.

Feel free to ping in chat if you have any questions.

[nasko, 2016-08-22 21:20:12]

nasko@chromium.org changed reviewers:
+ nasko@chromium.org

[nasko, 2016-08-22 21:20:13]

GetAPINameFromFullName should probably handle empty string for "full_name" and
return early, but that is orthogonal to this CL. 

IPC LGTM

[Wez, 2016-08-22 22:18:18]

wez@chromium.org changed reviewers:
+ mfoltz@chromium.org

[Wez, 2016-08-22 22:18:19]

+mfoltz

Devlin, Mark: Unit-testing this looks fiddly. We need to:
1. Have a "bad" schema in the generated-schemas table in DEBUG builds.
2. Instantiate enough of the browser process to have the real API schema table.
3. Inject a set of notification IPCs to test the bad schema, a good schema, and
some unknown schema names, to EXPECT_DEATH, and non-death, appropriately.

WDYT?

[Devlin, 2016-08-22 22:21:34]

On 2016/08/22 22:18:19, Wez wrote:
> +mfoltz
> 
> Devlin, Mark: Unit-testing this looks fiddly. We need to:
> 1. Have a "bad" schema in the generated-schemas table in DEBUG builds.
> 2. Instantiate enough of the browser process to have the real API schema
table.
> 3. Inject a set of notification IPCs to test the bad schema, a good schema,
and
> some unknown schema names, to EXPECT_DEATH, and non-death, appropriately.
> 
> WDYT?

I'm fine with holding off on a test for now.  The logic is pretty
straightforward (hopefully), the worst we'll do is open up a controlled crash to
a renderer (not a security vulnerability), and we can monitor crash rates to
ensure the number of affected users remains low.

[Wez, 2016-08-22 22:37:12]

The CQ bit was checked by wez@chromium.org

[Wez, 2016-08-22 22:37:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2254383002/#ps80001
(title: "Tweak comments")

[commit-bot: I haz the power, 2016-08-22 22:37:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 23:36:55]

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

[commit-bot: I haz the power, 2016-08-22 23:36:57]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Wez, 2016-08-22 23:47:08]

The CQ bit was checked by wez@chromium.org

[commit-bot: I haz the power, 2016-08-22 23:47:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 00:26:35]

Description was changed from

==========
Signal extension API schema corruption to the browser process.

Extension API schemas are parsed from string resources in the browser
binary, so failure to parse them usually indicates a problem with the
browser installation.

Crashing the extension process that tried to parse the schema makes it
look to the user like the extension is at fault (or worse, in the case
of component extensions, some browser features will misbehave for no
obvious reason).

This CL adds an IPC to indicate schema load failure back to the browser
and has the browser attempt to load the schema itself, and crash on
failure, so that the user has some indication that the browser itself
is broken (providing a better UX for this case is under review!).

Thanks to imcheng@ for implementing the bulk of this!

BUG=634423, 639104, 121424
==========

to

==========
Signal extension API schema corruption to the browser process.

Extension API schemas are parsed from string resources in the browser
binary, so failure to parse them usually indicates a problem with the
browser installation.

Crashing the extension process that tried to parse the schema makes it
look to the user like the extension is at fault (or worse, in the case
of component extensions, some browser features will misbehave for no
obvious reason).

This CL adds an IPC to indicate schema load failure back to the browser
and has the browser attempt to load the schema itself, and crash on
failure, so that the user has some indication that the browser itself
is broken (providing a better UX for this case is under review!).

Thanks to imcheng@ for implementing the bulk of this!

BUG=634423, 639104, 121424
==========

[commit-bot: I haz the power, 2016-08-23 00:26:37]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-23 00:29:22]

Description was changed from

==========
Signal extension API schema corruption to the browser process.

Extension API schemas are parsed from string resources in the browser
binary, so failure to parse them usually indicates a problem with the
browser installation.

Crashing the extension process that tried to parse the schema makes it
look to the user like the extension is at fault (or worse, in the case
of component extensions, some browser features will misbehave for no
obvious reason).

This CL adds an IPC to indicate schema load failure back to the browser
and has the browser attempt to load the schema itself, and crash on
failure, so that the user has some indication that the browser itself
is broken (providing a better UX for this case is under review!).

Thanks to imcheng@ for implementing the bulk of this!

BUG=634423, 639104, 121424
==========

to

==========
Signal extension API schema corruption to the browser process.

Extension API schemas are parsed from string resources in the browser
binary, so failure to parse them usually indicates a problem with the
browser installation.

Crashing the extension process that tried to parse the schema makes it
look to the user like the extension is at fault (or worse, in the case
of component extensions, some browser features will misbehave for no
obvious reason).

This CL adds an IPC to indicate schema load failure back to the browser
and has the browser attempt to load the schema itself, and crash on
failure, so that the user has some indication that the browser itself
is broken (providing a better UX for this case is under review!).

Thanks to imcheng@ for implementing the bulk of this!

BUG=634423, 639104, 121424

Committed: https://crrev.com/73b8107d339588f342d4f9a92888c9a05a3707b9
Cr-Commit-Position: refs/heads/master@{#413602}
==========

[commit-bot: I haz the power, 2016-08-23 00:29:23]

Patchset 5 (id:??) landed as
https://crrev.com/73b8107d339588f342d4f9a92888c9a05a3707b9
Cr-Commit-Position: refs/heads/master@{#413602}

[dcheng, 2016-08-23 05:07:09]

https://codereview.chromium.org/2254383002/diff/80001/extensions/common/exten...
File extensions/common/extension_api.cc (right):

https://codereview.chromium.org/2254383002/diff/80001/extensions/common/exten...
extensions/common/extension_api.cc:65: base::snprintf(buf, arraysize(buf), "%s:
(%d) '%s'", name.c_str(),
Out of curiosity, why not just use base::StringPrintf?

[Devlin, 2016-08-23 19:34:16]

https://codereview.chromium.org/2254383002/diff/80001/extensions/common/exten...
File extensions/common/extension_api.cc (right):

https://codereview.chromium.org/2254383002/diff/80001/extensions/common/exten...
extensions/common/extension_api.cc:65: base::snprintf(buf, arraysize(buf), "%s:
(%d) '%s'", name.c_str(),
On 2016/08/23 05:07:08, dcheng wrote:
> Out of curiosity, why not just use base::StringPrintf?

My guess is because we don't know how long error_message could be - e.g. if json
decided to say "invalid json: [whole schema]", we wouldn't like that.

[Wez, 2016-08-24 00:00:02]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2275813002/ by wez@chromium.org.

The reason for reverting is: Patch does not have the desired effect under
Windows, which has a multi-binary installation..

[dcheng, 2016-08-24 00:12:21]

https://codereview.chromium.org/2254383002/diff/80001/extensions/common/exten...
File extensions/common/extension_api.cc (right):

https://codereview.chromium.org/2254383002/diff/80001/extensions/common/exten...
extensions/common/extension_api.cc:65: base::snprintf(buf, arraysize(buf), "%s:
(%d) '%s'", name.c_str(),
On 2016/08/23 19:34:16, Devlin wrote:
> On 2016/08/23 05:07:08, dcheng wrote:
> > Out of curiosity, why not just use base::StringPrintf?
> 
> My guess is because we don't know how long error_message could be - e.g. if
json
> decided to say "invalid json: [whole schema]", we wouldn't like that.

There's a printf specifier to limit the max length of a string.

[Wez, 2016-08-24 00:24:13]

https://codereview.chromium.org/2254383002/diff/80001/extensions/common/exten...
File extensions/common/extension_api.cc (right):

https://codereview.chromium.org/2254383002/diff/80001/extensions/common/exten...
extensions/common/extension_api.cc:65: base::snprintf(buf, arraysize(buf), "%s:
(%d) '%s'", name.c_str(),
On 2016/08/24 00:12:21, dcheng wrote:
> On 2016/08/23 19:34:16, Devlin wrote:
> > On 2016/08/23 05:07:08, dcheng wrote:
> > > Out of curiosity, why not just use base::StringPrintf?
> > 
> > My guess is because we don't know how long error_message could be

Actually it's just that that's what the original code there did ;)