Call SendIq() from plugin asynchronously.

Call SendIq() from plugin asynchronously.

BUG=93951
TEST=Client doesn't crash when disconnecting

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98139

[Wez, 2011-08-24 20:44:45]

LGTM, but we're going to need to move away from synchronous scriptable
interfaces in the client, aren't we, so perhaps now is a good time to start work
on that?

http://codereview.chromium.org/7730002/diff/1/remoting/client/plugin/chromoti...
File remoting/client/plugin/chromoting_scriptable_object.cc (right):

http://codereview.chromium.org/7730002/diff/1/remoting/client/plugin/chromoti...
remoting/client/plugin/chromoting_scriptable_object.cc:353: void
ChromotingScriptableObject::DoSendIq(const std::string& message_xml) {
It's probably worth replacing the "Var() means..." comments in this and the
similar functions with a comment indicating that the ChromotingScriptableObject
must not be touched after Call() has returned, in case it has been freed.

http://codereview.chromium.org/7730002/diff/1/remoting/client/plugin/chromoti...
File remoting/client/plugin/chromoting_scriptable_object.h (right):

http://codereview.chromium.org/7730002/diff/1/remoting/client/plugin/chromoti...
remoting/client/plugin/chromoting_scriptable_object.h:220: // handlers may
destroy the plugin.
This comment says something is necessary, but doesn't say what the necessary
thing is. :)

[Sergey Ulanov, 2011-08-24 22:42:22]

> LGTM, but we're going to need to move away from 
> synchronous scriptable interfaces in the client, 
> aren't we, so perhaps now is a good time to start 
> work on that?
Not sure I understand what you mean here. When plugin is destroyed we need to
shutdown synchronously, so even if connect() and disconnect() were asynchronous
it wouldn't help here anyway.

http://codereview.chromium.org/7730002/diff/1/remoting/client/plugin/chromoti...
File remoting/client/plugin/chromoting_scriptable_object.cc (right):

http://codereview.chromium.org/7730002/diff/1/remoting/client/plugin/chromoti...
remoting/client/plugin/chromoting_scriptable_object.cc:353: void
ChromotingScriptableObject::DoSendIq(const std::string& message_xml) {
On 2011/08/24 20:44:45, Wez wrote:
> It's probably worth replacing the "Var() means..." comments in this and the
> similar functions with a comment indicating that the
ChromotingScriptableObject
> must not be touched after Call() has returned, in case it has been freed.

Done.

http://codereview.chromium.org/7730002/diff/1/remoting/client/plugin/chromoti...
File remoting/client/plugin/chromoting_scriptable_object.h (right):

http://codereview.chromium.org/7730002/diff/1/remoting/client/plugin/chromoti...
remoting/client/plugin/chromoting_scriptable_object.h:220: // handlers may
destroy the plugin.
On 2011/08/24 20:44:45, Wez wrote:
> This comment says something is necessary, but doesn't say what the necessary
> thing is. :)

Done.

[Wez, 2011-08-24 22:51:13]

On 2011/08/24 22:42:22, sergeyu wrote:
> > LGTM, but we're going to need to move away from 
> > synchronous scriptable interfaces in the client, 
> > aren't we, so perhaps now is a good time to start 
> > work on that?
> Not sure I understand what you mean here. When plugin is destroyed we need to
> shutdown synchronously, so even if connect() and disconnect() were
asynchronous
> it wouldn't help here anyway.

I think the model is that connect() and disconnect() become messages posted to
the plugin, rather than APIs, as would sendIq().

[Sergey Ulanov, 2011-08-24 23:07:00]

On 2011/08/24 22:51:13, Wez wrote:
> On 2011/08/24 22:42:22, sergeyu wrote:
> > > LGTM, but we're going to need to move away from 
> > > synchronous scriptable interfaces in the client, 
> > > aren't we, so perhaps now is a good time to start 
> > > work on that?
> > Not sure I understand what you mean here. When plugin is destroyed we need
to
> > shutdown synchronously, so even if connect() and disconnect() were
> asynchronous
> > it wouldn't help here anyway.
> 
> I think the model is that connect() and disconnect() become messages posted to
> the plugin, rather than APIs, as would sendIq().

I'm not convinced we really need to make connect(), disconnect() and other APIs
asynchronous? What benefits would it provide?

[Wez, 2011-08-25 01:11:23]

On 2011/08/24 23:07:00, sergeyu wrote:
> On 2011/08/24 22:51:13, Wez wrote:
> > On 2011/08/24 22:42:22, sergeyu wrote:
> > > > LGTM, but we're going to need to move away from 
> > > > synchronous scriptable interfaces in the client, 
> > > > aren't we, so perhaps now is a good time to start 
> > > > work on that?
> > > Not sure I understand what you mean here. When plugin is destroyed we need
> to
> > > shutdown synchronously, so even if connect() and disconnect() were
> > asynchronous
> > > it wouldn't help here anyway.
> > 
> > I think the model is that connect() and disconnect() become messages posted
to
> > the plugin, rather than APIs, as would sendIq().
> 
> I'm not convinced we really need to make connect(), disconnect() and other
APIs
> asynchronous? What benefits would it provide?

Firstly, the synchronous APIs are deprecated, as far as I'm aware.  Secondly,
it's better in that it avoids issues with mid-callback tear-down of components.

[Sergey Ulanov, 2011-08-25 20:25:59]

On Wed, Aug 24, 2011 at 6:11 PM, <wez@chromium.org> wrote:

> On 2011/08/24 23:07:00, sergeyu wrote:
>
>> On 2011/08/24 22:51:13, Wez wrote:
>> > On 2011/08/24 22:42:22, sergeyu wrote:
>> > > > LGTM, but we're going to need to move away from
>> > > > synchronous scriptable interfaces in the client,
>> > > > aren't we, so perhaps now is a good time to start
>> > > > work on that?
>> > > Not sure I understand what you mean here. When plugin is destroyed we
>> need
>> to
>> > > shutdown synchronously, so even if connect() and disconnect() were
>> > asynchronous
>> > > it wouldn't help here anyway.
>> >
>> > I think the model is that connect() and disconnect() become messages
>> posted
>>
> to
>
>> > the plugin, rather than APIs, as would sendIq().
>>
>
>  I'm not convinced we really need to make connect(), disconnect() and other
>>
> APIs
>
>> asynchronous? What benefits would it provide?
>>
>
> Firstly, the synchronous APIs are deprecated, as far as I'm aware.
>  Secondly,
> it's better in that it avoids issues with mid-callback tear-down of
> components.


Oh, now I understand it. You meant that we need to switch to massaging-based
interface from the deprecated ScriptableObject, not just make the plugin
calls asynchronous. Looks like it's a prerequisite for switching to NaCl.


>
>
>
http://codereview.chromium.**org/7730002/<http://codereview.chromium.org/7730...
>