Remove some usages of AsyncExtensionFunction::results_.

Introduce AlreadyResponded() response action from ExtensionFunction.

Currently ExtensionFunction::Run has to either:
call RespondNow() to synchronously respond or
call RespondLater() to asynchronously Respond() later point in time after
  Run() finishes.

There are some asynchronous ExtensionFunction implementations where,
due to how base:: callbacks are used in them, it can be difficult to
pick either one of the two responses above. This happens when a callback
used in Run() can sometimes synchronously fire and if the callback
calls Respond(), then returning RespondLater() from Run() would be
a wrong choice. 

This CL provides AlreadyResponded() response action for those cases.
While returning response from Run(), the ExtensionFunction can check
whether it has already responded and return AlreadyResponded(). Otherwise,
it can return RespondLater().

This CL removes some usages of AsyncExtensionFunction::results_ in turn.
This CL also converts those classes to directly inherit from
UIThreadExtensionFunction.

BUG=648275
Review-Url: https://codereview.chromium.org/2612873004
Cr-Commit-Position: refs/heads/master@{#445507}
Committed: https://chromium.googlesource.com/chromium/src/+/30abd07e271e93f99b70745a08b360793be779cc

[lazyboy, 2017-01-06 03:40:20]

Description was changed from

==========
Remove some usages of AsyncExtensionFunction::results_.

BUG=648275
==========

to

==========
Remove some usages of AsyncExtensionFunction::results_.

Convert those classes to directly inherit from UIThreadExtensionFunction.

BUG=648275
==========

[lazyboy, 2017-01-06 03:40:20]

lazyboy@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[lazyboy, 2017-01-06 03:40:30]

https://codereview.chromium.org/2612873004/diff/1/extensions/browser/api/mana...
File extensions/browser/api/management/management_api.cc (right):

https://codereview.chromium.org/2612873004/diff/1/extensions/browser/api/mana...
extensions/browser/api/management/management_api.cc:342: return
RespondNow(Error(""));
There was a Release(Ref) call in OnParseFailure(), which seems wrong. Fixed now.

https://codereview.chromium.org/2612873004/diff/1/extensions/browser/api/mana...
extensions/browser/api/management/management_api.cc:683: return
RespondNow(Error(error));
This another bug fix where this branch's extension API call would never return
because this branch wouldn't result in OnCloseShortcutPrompt.

[Devlin, 2017-01-06 15:51:06]

Thanks for helping! :)

https://codereview.chromium.org/2612873004/diff/1/extensions/browser/api/mana...
File extensions/browser/api/management/management_api.cc (right):

https://codereview.chromium.org/2612873004/diff/1/extensions/browser/api/mana...
extensions/browser/api/management/management_api.cc:342: return
RespondNow(Error(""));
let's make this kUnknownErrorDoNotUse.

https://codereview.chromium.org/2612873004/diff/1/extensions/browser/api/mana...
extensions/browser/api/management/management_api.cc:670: return RespondLater();
This is unfortunately wrong, because OnCloseShortcutPrompt() Respond()s
synchronously.  This is a limitation of the response system.  One common work
around is to post a task to OnCloseShortcutPrompt (thereby making the
RespondLater here correct).  I feel like there should be a better, more general
fix by somehow allowing this, but I haven't thought of a way that would still
let us have the determinism we want.

https://codereview.chromium.org/2612873004/diff/1/extensions/browser/api/mana...
extensions/browser/api/management/management_api.cc:748:
Respond(ArgumentList(management::GenerateAppForLink::Results::Create(
nit: maybe cleaner as something like:
ResponseAction action = extension ?
    ArgumentList(management::GenerateAppForLink::Results::Create(...)) :
    Error(keys::...);
Respond(std::move(action));
Release();  // Balanced in...

https://codereview.chromium.org/2612873004/diff/1/extensions/browser/api/netw...
File extensions/browser/api/networking_private/networking_private_api.cc
(right):

https://codereview.chromium.org/2612873004/diff/1/extensions/browser/api/netw...
extensions/browser/api/networking_private/networking_private_api.cc:64:
base::Bind(&NetworkingPrivateGetPropertiesFunction::Failure, this));
Here, too, these can conditionally fire synchronously.

https://codereview.chromium.org/2612873004/diff/1/extensions/browser/api/netw...
File extensions/browser/api/networking_private/networking_private_api.h (right):

https://codereview.chromium.org/2612873004/diff/1/extensions/browser/api/netw...
extensions/browser/api/networking_private/networking_private_api.h:40: //
ExtensionFunction::
nit: One ':'

https://codereview.chromium.org/2612873004/diff/1/extensions/browser/api/syst...
File extensions/browser/api/system_storage/system_storage_api.cc (right):

https://codereview.chromium.org/2612873004/diff/1/extensions/browser/api/syst...
extensions/browser/api/system_storage/system_storage_api.cc:49:
base::Bind(&SystemStorageEjectDeviceFunction::OnStorageMonitorInit,
can return sync

[lazyboy, 2017-01-07 02:55:18]

Still trying to figure out what we should do here...

https://codereview.chromium.org/2612873004/diff/1/extensions/browser/api/mana...
File extensions/browser/api/management/management_api.cc (right):

https://codereview.chromium.org/2612873004/diff/1/extensions/browser/api/mana...
extensions/browser/api/management/management_api.cc:670: return RespondLater();
On 2017/01/06 15:51:06, Devlin wrote:
> This is unfortunately wrong, because OnCloseShortcutPrompt() Respond()s
> synchronously.  This is a limitation of the response system.  One common work
> around is to post a task to OnCloseShortcutPrompt (thereby making the
> RespondLater here correct).  I feel like there should be a better, more
general
> fix by somehow allowing this, but I haven't thought of a way that would still
> let us have the determinism we want.

To understand the problem: with callbacks, this already happens, right?:
RunAsync() {
  some_api->Call(base::Bind(::Callback,...))
  return true;
}
Callback() {
  SetResult(..)
  SendResponse(true);
}
^^ SendResponse() can be called before RunAsync returns...

"Always PostTask when there's callback" approach seems a bit more unnecessary
code execution. On top of that, this wouldn't guarantee the determinism either
:( because you cannot stop implementer from calling Respond synchronously
anyway.

Callback seems tough for these: you can call them synchronously, you can call
them more than once.

Would there be broken-ness if someone Respond()ed before returning
RespondLater()? We can relax the contract of RespondLater by saying you're on
your own to Respond(), within Run() or later point in time... Not sure if that
goes against all the efforts to achieve better response system though.

[Devlin, 2017-01-09 22:05:26]

https://codereview.chromium.org/2612873004/diff/1/extensions/browser/api/mana...
File extensions/browser/api/management/management_api.cc (right):

https://codereview.chromium.org/2612873004/diff/1/extensions/browser/api/mana...
extensions/browser/api/management/management_api.cc:670: return RespondLater();
On 2017/01/07 02:55:18, lazyboy wrote:
> On 2017/01/06 15:51:06, Devlin wrote:
> > This is unfortunately wrong, because OnCloseShortcutPrompt() Respond()s
> > synchronously.  This is a limitation of the response system.  One common
work
> > around is to post a task to OnCloseShortcutPrompt (thereby making the
> > RespondLater here correct).  I feel like there should be a better, more
> general
> > fix by somehow allowing this, but I haven't thought of a way that would
still
> > let us have the determinism we want.
> 
> To understand the problem: with callbacks, this already happens, right?:
> RunAsync() {
>   some_api->Call(base::Bind(::Callback,...))
>   return true;
> }
> Callback() {
>   SetResult(..)
>   SendResponse(true);
> }
> ^^ SendResponse() can be called before RunAsync returns...
> 
> "Always PostTask when there's callback" approach seems a bit more unnecessary
> code execution. On top of that, this wouldn't guarantee the determinism either
> :( because you cannot stop implementer from calling Respond synchronously
> anyway.

Agreed that this isn't great, and I'd prefer something else (which is why I
haven't made these changes more widely) - but it *should* be guaranteed to be
correct.  If you post a task, it's guaranteed to execute after what you're
currently doing (% run loops, etc), so something like:
base::PostTask(OnCloseShortcutPrompt());
return RespondLater();

should always be safe and correct.

All that said, I don't want that to be the norm.

> Callback seems tough for these: you can call them synchronously, you can call
> them more than once.
> 
> Would there be broken-ness if someone Respond()ed before returning
> RespondLater()? We can relax the contract of RespondLater by saying you're on
> your own to Respond(), within Run() or later point in time... Not sure if that
> goes against all the efforts to achieve better response system though.

Yeah, that'd definitely makes me sad.  One of the biggest things here is that we
want to ensure that functions know what they're doing properly (which was
all-but-impossible in the old system).  If a function says RespondLater(), I'd
like it to be guaranteed that it has not yet responded.

The best solution I've come up with so far (which I'm not at all in love with)
would be to have an AlreadyResponded() ResponseAction.  At the very least, this
would require function implementations to be clear when they expect this to be
the case, and allow us to assert, etc.

[lazyboy, 2017-01-19 03:47:59]

I've updated the patch to use AlreadyResponded ResponseAction.

https://codereview.chromium.org/2612873004/diff/1/extensions/browser/api/mana...
File extensions/browser/api/management/management_api.cc (right):

https://codereview.chromium.org/2612873004/diff/1/extensions/browser/api/mana...
extensions/browser/api/management/management_api.cc:342: return
RespondNow(Error(""));
On 2017/01/06 15:51:06, Devlin (slow) wrote:
> let's make this kUnknownErrorDoNotUse.

Done.

https://codereview.chromium.org/2612873004/diff/1/extensions/browser/api/mana...
extensions/browser/api/management/management_api.cc:748:
Respond(ArgumentList(management::GenerateAppForLink::Results::Create(
On 2017/01/06 15:51:06, Devlin (slow) wrote:
> nit: maybe cleaner as something like:
> ResponseAction action = extension ?
>     ArgumentList(management::GenerateAppForLink::Results::Create(...)) :
>     Error(keys::...);
> Respond(std::move(action));
> Release();  // Balanced in...

Done.

https://codereview.chromium.org/2612873004/diff/1/extensions/browser/api/netw...
File extensions/browser/api/networking_private/networking_private_api.cc
(right):

https://codereview.chromium.org/2612873004/diff/1/extensions/browser/api/netw...
extensions/browser/api/networking_private/networking_private_api.cc:64:
base::Bind(&NetworkingPrivateGetPropertiesFunction::Failure, this));
On 2017/01/06 15:51:06, Devlin (slow) wrote:
> Here, too, these can conditionally fire synchronously.

Done.

https://codereview.chromium.org/2612873004/diff/1/extensions/browser/api/netw...
File extensions/browser/api/networking_private/networking_private_api.h (right):

https://codereview.chromium.org/2612873004/diff/1/extensions/browser/api/netw...
extensions/browser/api/networking_private/networking_private_api.h:40: //
ExtensionFunction::
On 2017/01/06 15:51:06, Devlin (slow) wrote:
> nit: One ':'

Done.

https://codereview.chromium.org/2612873004/diff/1/extensions/browser/api/syst...
File extensions/browser/api/system_storage/system_storage_api.cc (right):

https://codereview.chromium.org/2612873004/diff/1/extensions/browser/api/syst...
extensions/browser/api/system_storage/system_storage_api.cc:49:
base::Bind(&SystemStorageEjectDeviceFunction::OnStorageMonitorInit,
On 2017/01/06 15:51:06, Devlin (possibly slow) wrote:
> can return sync

Added comment and fixed the issue.

[Devlin, 2017-01-20 00:16:02]

Nice!

Please also update the CL description. :)

https://codereview.chromium.org/2612873004/diff/60001/extensions/browser/api/...
File extensions/browser/api/management/management_api.cc (right):

https://codereview.chromium.org/2612873004/diff/60001/extensions/browser/api/...
extensions/browser/api/management/management_api.cc:636: Release();
Can we add a "balanced by" comment?

https://codereview.chromium.org/2612873004/diff/60001/extensions/browser/exte...
File extensions/browser/extension_function.cc (right):

https://codereview.chromium.org/2612873004/diff/60001/extensions/browser/exte...
extensions/browser/extension_function.cc:414:
base::Bind(&ExtensionFunction::did_respond, this)));
Why not just DCHECK() here instead, and have Execute() do nothing?

[lazyboy, 2017-01-20 00:52:49]

Description was changed from

==========
Remove some usages of AsyncExtensionFunction::results_.

Convert those classes to directly inherit from UIThreadExtensionFunction.

BUG=648275
==========

to

==========
Introduce AlreadyResponded() response action from ExtensionFunction.

Currently ExtensionFunction::Run has to either:
call RespondNow() to synchronously respond or
call RespondLater() to asynchronously Respond() later point in time after
  Run() finishes.

There are some asynchronous ExtensionFunction implementations where,
due to how base:: callbacks are used in them, it can be difficult to
pick either one of the two responses above. This happens when a callback
used in Run() can sometimes synchronously fire and if the callback
calls Respond(), then returning RespondLater() from Run() would be
a wrong choice. 

This CL provides AlreadyResponded() response action for those cases.
While returning response from Run(), the ExtensionFunction can check
whether it has already responded and return AlreadyResponded(). Otherwise,
it can return RespondLater().

This CL removes some usages of AsyncExtensionFunction::results_ in turn.
This CL also converts those classes to directly inherit from
UIThreadExtensionFunction.

BUG=648275
==========

[lazyboy, 2017-01-20 00:54:47]

CL description updated.

https://codereview.chromium.org/2612873004/diff/60001/extensions/browser/api/...
File extensions/browser/api/management/management_api.cc (right):

https://codereview.chromium.org/2612873004/diff/60001/extensions/browser/api/...
extensions/browser/api/management/management_api.cc:636: Release();
On 2017/01/20 00:16:01, Devlin (slow) wrote:
> Can we add a "balanced by" comment?

Done.

https://codereview.chromium.org/2612873004/diff/60001/extensions/browser/exte...
File extensions/browser/extension_function.cc (right):

https://codereview.chromium.org/2612873004/diff/60001/extensions/browser/exte...
extensions/browser/extension_function.cc:414:
base::Bind(&ExtensionFunction::did_respond, this)));
On 2017/01/20 00:16:02, Devlin (slow) wrote:
> Why not just DCHECK() here instead, and have Execute() do nothing?

Right, thanks.
Done.

[Devlin, 2017-01-20 16:31:00]

lgtm

[lazyboy, 2017-01-20 18:58:41]

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

[commit-bot: I haz the power, 2017-01-20 18:59:00]

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-20 19:51:59]

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

[commit-bot: I haz the power, 2017-01-20 19:52:00]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[lazyboy, 2017-01-21 02:28:38]

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

[commit-bot: I haz the power, 2017-01-21 02:29:09]

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-21 03:35:49]

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

[commit-bot: I haz the power, 2017-01-21 03:35:50]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-01-21 04:02:12]

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

[commit-bot: I haz the power, 2017-01-21 04:02:28]

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-21 05:00:54]

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

[commit-bot: I haz the power, 2017-01-21 05:00:55]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-01-23 21:10:13]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2017-01-23 21:10:14]

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/2612873004/#ps120001
(title: "sync")

[commit-bot: I haz the power, 2017-01-23 21:10:53]

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-23 22:12:37]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1485205813880720,
"parent_rev": "8b0516856932e9b17a18a9616c477c420ed65d34", "commit_rev":
"30abd07e271e93f99b70745a08b360793be779cc"}

[commit-bot: I haz the power, 2017-01-23 22:13:06]

Description was changed from

==========
Introduce AlreadyResponded() response action from ExtensionFunction.

Currently ExtensionFunction::Run has to either:
call RespondNow() to synchronously respond or
call RespondLater() to asynchronously Respond() later point in time after
  Run() finishes.

There are some asynchronous ExtensionFunction implementations where,
due to how base:: callbacks are used in them, it can be difficult to
pick either one of the two responses above. This happens when a callback
used in Run() can sometimes synchronously fire and if the callback
calls Respond(), then returning RespondLater() from Run() would be
a wrong choice. 

This CL provides AlreadyResponded() response action for those cases.
While returning response from Run(), the ExtensionFunction can check
whether it has already responded and return AlreadyResponded(). Otherwise,
it can return RespondLater().

This CL removes some usages of AsyncExtensionFunction::results_ in turn.
This CL also converts those classes to directly inherit from
UIThreadExtensionFunction.

BUG=648275
==========

to

==========
Introduce AlreadyResponded() response action from ExtensionFunction.

Currently ExtensionFunction::Run has to either:
call RespondNow() to synchronously respond or
call RespondLater() to asynchronously Respond() later point in time after
  Run() finishes.

There are some asynchronous ExtensionFunction implementations where,
due to how base:: callbacks are used in them, it can be difficult to
pick either one of the two responses above. This happens when a callback
used in Run() can sometimes synchronously fire and if the callback
calls Respond(), then returning RespondLater() from Run() would be
a wrong choice. 

This CL provides AlreadyResponded() response action for those cases.
While returning response from Run(), the ExtensionFunction can check
whether it has already responded and return AlreadyResponded(). Otherwise,
it can return RespondLater().

This CL removes some usages of AsyncExtensionFunction::results_ in turn.
This CL also converts those classes to directly inherit from
UIThreadExtensionFunction.

BUG=648275

Review-Url: https://codereview.chromium.org/2612873004
Cr-Commit-Position: refs/heads/master@{#445507}
Committed:
https://chromium.googlesource.com/chromium/src/+/30abd07e271e93f99b70745a08b3...
==========

[commit-bot: I haz the power, 2017-01-23 22:13:07]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/30abd07e271e93f99b70745a08b3...