Don't crash browser process when OK passed as error code in FSP API.

Don't crash browser process when OK passed as error code in FSP API.

This CL fixes it by adding a guard to both custom bindings, and
API implementation.

TEST=Tested with a testing extension.
BUG=682556
Review-Url: https://codereview.chromium.org/2643013002
Cr-Commit-Position: refs/heads/master@{#451711}
Committed: https://chromium.googlesource.com/chromium/src/+/0891c24ebc475fb9b92c1d31a7e8d8e49a7de065

[mtomasz, 2017-01-19 07:57:13]

Description was changed from

==========
Don't crash browser process when OK passed as error code in FSP API.

This CL fixes it by adding a guard to both custom bindings, and
API implementation.

TEST=Tested with a testing extension.
BUG=682556
==========

to

==========
Don't crash browser process when OK passed as error code in FSP API.

This CL fixes it by adding a guard to both custom bindings, and
API implementation.

TEST=Tested with a testing extension.
BUG=682556
==========

[mtomasz, 2017-01-19 07:57:13]

mtomasz@chromium.org changed reviewers:
+ benwells@chromium.org

[mtomasz, 2017-01-19 07:57:21]

@benwells: PTAL. Thanks.

[mtomasz, 2017-01-19 07:57:27]

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

[commit-bot: I haz the power, 2017-01-19 07:57:34]

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, 2017-01-19 08:24:06]

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

[commit-bot: I haz the power, 2017-01-19 08:24:07]

Dry run: This issue passed the CQ dry run.

[benwells, 2017-01-30 01:50:51]

lgtm with a nit / question.

sorry about the delay, I was on leave but didn't update my handle...

https://codereview.chromium.org/2643013002/diff/1/chrome/renderer/resources/e...
File
chrome/renderer/resources/extensions/file_system_provider_custom_bindings.js
(right):

https://codereview.chromium.org/2643013002/diff/1/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/file_system_provider_custom_bindings.js:369:
verifyErrorForFailure(error);
Nit: should this bail on failure like the other onErrorCallbacks?

[mtomasz, 2017-02-21 05:25:59]

https://codereview.chromium.org/2643013002/diff/1/chrome/renderer/resources/e...
File
chrome/renderer/resources/extensions/file_system_provider_custom_bindings.js
(right):

https://codereview.chromium.org/2643013002/diff/1/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/file_system_provider_custom_bindings.js:369:
verifyErrorForFailure(error);
On 2017/01/30 01:50:50, benwells wrote:
> Nit: should this bail on failure like the other onErrorCallbacks?

Do you mean return if returned false? It would be a no-op here. If your question
is why we don't call fileSystemProviderInternal.operationRequestError here like
in other onErrorCallbacks, then the answer is I don't remember :(.

It's quite parallel to the issue this CL is addressing, so would you mind if I
look at it separately?

[benwells, 2017-02-21 05:35:11]

https://codereview.chromium.org/2643013002/diff/1/chrome/renderer/resources/e...
File
chrome/renderer/resources/extensions/file_system_provider_custom_bindings.js
(right):

https://codereview.chromium.org/2643013002/diff/1/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/file_system_provider_custom_bindings.js:369:
verifyErrorForFailure(error);
On 2017/02/21 05:25:59, mtomasz wrote:
> On 2017/01/30 01:50:50, benwells wrote:
> > Nit: should this bail on failure like the other onErrorCallbacks?
> 
> Do you mean return if returned false? It would be a no-op here. 

That is what I meant. It would be a no-op, but would be clear to whoever has to
come and implement the TODO.

If your question
> is why we don't call fileSystemProviderInternal.operationRequestError here
like
> in other onErrorCallbacks, then the answer is I don't remember :(.
> 
> It's quite parallel to the issue this CL is addressing, so would you mind if I
> look at it separately?

[mtomasz, 2017-02-21 05:56:43]

https://codereview.chromium.org/2643013002/diff/1/chrome/renderer/resources/e...
File
chrome/renderer/resources/extensions/file_system_provider_custom_bindings.js
(right):

https://codereview.chromium.org/2643013002/diff/1/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/file_system_provider_custom_bindings.js:369:
verifyErrorForFailure(error);
On 2017/02/21 05:35:11, benwells wrote:
> On 2017/02/21 05:25:59, mtomasz wrote:
> > On 2017/01/30 01:50:50, benwells wrote:
> > > Nit: should this bail on failure like the other onErrorCallbacks?
> > 
> > Do you mean return if returned false? It would be a no-op here. 
> 
> That is what I meant. It would be a no-op, but would be clear to whoever has
to
> come and implement the TODO.
> 
> If your question
> > is why we don't call fileSystemProviderInternal.operationRequestError here
> like
> > in other onErrorCallbacks, then the answer is I don't remember :(.
> > 
> > It's quite parallel to the issue this CL is addressing, so would you mind if
I
> > look at it separately?
> 

I took a look at the original CL adding this TODO, and there is nothing to
implement here until we have more clients dispatching this event. It's unlikely
to happen, so removed the TODO and replaced with a comment.

[benwells, 2017-02-21 05:58:37]

cool, thanks.

lgtm

[mtomasz, 2017-02-21 06:02:24]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2017-02-21 06:02:47]

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, 2017-02-21 06:36:17]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487656944324440,
"parent_rev": "1c34d25f88bf52dd34b4047b8218be96096ba9bc", "commit_rev":
"0891c24ebc475fb9b92c1d31a7e8d8e49a7de065"}

[commit-bot: I haz the power, 2017-02-21 06:37:03]

Description was changed from

==========
Don't crash browser process when OK passed as error code in FSP API.

This CL fixes it by adding a guard to both custom bindings, and
API implementation.

TEST=Tested with a testing extension.
BUG=682556
==========

to

==========
Don't crash browser process when OK passed as error code in FSP API.

This CL fixes it by adding a guard to both custom bindings, and
API implementation.

TEST=Tested with a testing extension.
BUG=682556

Review-Url: https://codereview.chromium.org/2643013002
Cr-Commit-Position: refs/heads/master@{#451711}
Committed:
https://chromium.googlesource.com/chromium/src/+/0891c24ebc475fb9b92c1d31a7e8...
==========

[commit-bot: I haz the power, 2017-02-21 06:37:04]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/0891c24ebc475fb9b92c1d31a7e8...