Patch to fix asynchronous callbacks

[Aaron]

Hi,

I submitted a patch last week (issue 2 in google code). I thought I'd
sum it up here as well as no-one seems to have picked it up as an
issue.

I found when using the [async] option on callbacks, things worked fine
only if the callback was called from the browsers UI thread. Given the
purpose of asynchronous callback functions is to allow callbacks to be
triggered at arbitrary times, this seemed odd so I did some digging
and realised the browsers NPAPI memory allocator was being used to
allocated heap space for a NPCallback C++ class instance.

This was happening from whatever thread posted the asynchronous
callback event. If this thread was NOT the browsers UI thread, the
allocation would fail and the callback would not trigger.

My solution was to change the way the allocation and free of an
NPCallback() object was handled. It now uses the standard C++ new/
delete allocator, allocating the NPCallback() in the private plugin
thread and freeing it in the browser UI thread after successful
execution.

Hope its ok. I have only tested it on Win32 using FF3.5, safari 4 and
chrome 3.

Regards,
Aaron

[Antoine Labour]

Hi Aaron,

Sorry about the delay. Thank you for looking into this issue !

In general for patches, it's better to use the code review tools, gcl and friends. See http://code.google.com/p/nixysa/wiki/ContributingCode

I asked another developer who implemented async callbacks to look at your changes if he has time, otherwise I'll give a second look.

Before we can check in your patch however, we'll need you to sign the CLA (some lawyerese document that allows us to use your code), see http://code.google.com/legal/individual-cla-v1.0.html if you are an individual (and you can sign online), or http://code.google.com/legal/corporate-cla-v1.0.html if you work for a corporation.

Thanks,

Antoine

[apat...@google.com]

Sorry for the delay. I've had a cold. I looked at the patch. It's
mostly okay except I believe that if you invoke a callback
asynchronously on another thread, it will call NPN_RetainObject on
that thread (in the NPCallback) constructor for the function object
and any object arguments. As far as I know, browser do not do atomic
reference counting so reference counts would occasionally get out of
sync leading to crashes and memory leaks.

It would be cool to support asynchronous callbacks on other threads
but these issues need to be addressed first.

Please use the code review tool if you can. It's awesome!

Thanks!

Al

[Aaron Drew]

Hi Al,

Sorry its taken me a while to get back to you also. Everyone's busy it seems! 

I just had a bit of a play with gcl and I'll commit future patches with it from now on.

I'd like to fix this issue properly I can. There are a few things I wanted to change but didn't want to break existing code:

The basic definitions for the callback abstract meta classes for 0...n arguments are currently defined in O3D's "callbacks.h" file. I think these should be moved to nixysa's common.h (or even a separate callbacks.h file) as these have nothing to do with O3D per-se and everything to do with nixysa. This will break O3D so I guess a coordinated change would be required for that.

I have assumed calling NPN functions was thread safe but I can understand the need to exercise caution. Actually, the current callback mechanism already makes use of NPN calls from the non UI threads. When you call Run(), the generated glue code calls RunCallback() which uses NPN_ReleaseObject, StringToNPVariant, NPN_GetStringIdentifier, NPN_InvokeDefault (called from the worker thread if the browser doesn't support async) and NPN_ReleaseVariantValue. 

To avoid calling these functions from non UI threads, the bulk of the RunCallback glue code could be moved to my proposed NPCallback_DoAsyncCall() function in common.cc. This function is only ever called from the worker thread so it should be safe to do any NPN calls from here. 

As is done now, to make sure that the callback function doesn't disappear before we use it, either CreateObject() or the callback_glue class constructor should issue an NPN_RetainObject(). The corresponding NPN_ReleaseObject() cleanup code is currently commented out due to a firefox bug (anyone know the bug reference id?) so with no further changes that should be enough to safely isolate the NPN calls from any worker threads. 

If the NPN_ReleaseObject() code is needed in the future, it should probably make use of NPN_PluginThreadAsyncCall() in the destructor of the C++ (and thread-safe) NPCallback class to perform the final NPN_ReleaseObject on the callback function. That way, all the NPN calls are guarenteed to be performed from the ui thread.

In the case that async callbacks are not supported, should we throw an exception or return false or something? Right now nixysa tries to use NPN_InvokeDefault from the worker thread which is bound to cause problems if async callback functions are called on a thread unsafe NPN browser implementation.

Regards,

Aaron

[Alastair Patrick]

Hi Aaron,

I think the difference is O3D isn't calling (or shouldn't be calling) async callbacks off the UI thread. We're only using it to defer callbacks on the UI thread.

Would it be possible to move the asynchronous call to to a higher level in your application? For example, have the application call NPN_PluginThreadAsyncCall to invoke a C++ function on the UI thread and then have that invoke the JavaScript callback? It wouldn't even need to be [async] then. We do that in O3D to signal the completion of gzip decompression and tar decoding on a worker thread I believe. You could get the same effect on browsers that do not support NPN_PluginThreadAsyncCall by using PostMessage to send your plugin window a message (on Windows at least). Better to use NPN_PluginThreadAsyncCall is it's available though.

Al

[Aaron Drew]

Ah! I see what you mean now. That explains why O3D hasn't hit the same problems as I have regarding calls to NPN_RetainObject from a local thread. 

I've been learning NPAPI along the way and initially it didn't occur to me to do anything besides NPN_InvokeDefault from within the function that NPN_PluginThreadAsyncCall triggers.

I certainly can fix my code. I was under the impression though that nixysa was "broken" in the sense that I assumed it was designed to be thread safe for some reason. Aside from asynchronous callbacks, is there anywhere else it is _not_ thread-safe within nixysa generated code? Is thread-safe operation a target for the nixysa project?

Should I revise (read: simplify) my patch and resubmit?

- Aaron

[Alastair Patrick]

Hi Aaron,

What would be the benefit of the patch if it doesn't give us thread safety? There doesn't seem to be any need for NPCallback to be an NPObject but I don't think that is really worth the effort if that's the only advantage.

Don't assume anything in nixysa is thread safe. It wasn't designed with that in mind. The only NPN function that can be safely called from another thread is NPN_PluginThreadAsyncCall. The NPVARIANT_* macros should also be okay I think because they don't make NPN calls. You can inspect the nixysa generated code to see if you think it's thread safe but it makes a lot of NPN calls.

Al

[Aaron Drew]

Hi Alastair,

Javascript initiated actions will always occur in the UI thread so thread-safety is not an issue in that direction. The NPN functions will always be called from the correct thread.

Plugin-initiated actions are limited to callback functions and, depending on the binding model, referencing and dereferencing (NPN_RetainObject, NPN_ReleaseObject) of class instances.

The aim of the patch would be to ensure that you can safely initiate a callback or retain/release an instance of something from any thread.

I am a bit concerned though that there is no guaranteed way to do this if  NPN_PluginThreadAsyncCall is not supported by the browser. Posting messages, as you say, only works on Windows. Should I forget about this for now?

- Aaron

[Alastair Patrick]

I think to safely retain or release an object from another thread you would have to use NPN_PluginThreadAsyncCall to ask the main thread to do it and then block until the request is processed. I can't think of any other way to do it safely.

I encourage you to use NPN_PluginThreadAsyncCall at a higher level if you can. NPN_PluginThreadAsyncCall is pretty well supported. Firefox 3+, Safari and Chrome all have it. The reason we support browser's without it is I think just for Firefox 2. That might not be a concern for you?

Also, if you need to do something similar in IE, you know you're in Windows so PostMessage is available. That's how we emulate NPN_PluginThreadAsyncCall for O3D in IE.

Al

[Aaron Drew]

Thanks for your help. I've got a much better grasp of things now. It does seem a lot simpler to make use of NPN_PluginThreadAsyncCall as neccessary instead of "thread-safeing" the entire library. A part of me wishes the whole AsyncCall interface was hidden from view by nixysa but as you say its going to be easier just to use it at a higher level.

Thanks again!

- Aaron

[Aaron]

Hi Alistair,

Sorry if I sound like I'm flogging a dead horse here Alistair but I
have a few more questions.

Is the point of deferring callbacks using [async] to avoid locking or
stack-related issues inside the O3D codebase? The fallback code simply
performs a regular NPN_InvokeDefault so is it safe to presume this is
more of a "nice thing to have" rather than a necessity in O3D?

You mentioned that on FF2 PostMessage is used to achieve similar
things but I can't find it in the O3D codebase anywhere. The only two
calls to PostMessage are for the ActiveX code-base for what I can
tell.

I am not super concerned about not supporting FF2, just curious.

- Aaron

On Oct 27, 11:56 am, Aaron Drew <aaron...@gmail.com> wrote:
> Thanks for your help. I've got a much better grasp of things now. It does
> seem a lot simpler to make use of NPN_PluginThreadAsyncCall as neccessary
> instead of "thread-safeing" the entire library. A part of me wishes the
> whole AsyncCall interface was hidden from view by nixysa but as you say its
> going to be easier just to use it at a higher level.
>
> Thanks again!
>
> - Aaron
>

> On Tue, Oct 27, 2009 at 10:43 AM, Alastair Patrick <apatr...@google.com>wrote:
>
>
>
> > I think to safely retain or release an object from another thread you would
> > have to use NPN_PluginThreadAsyncCall to ask the main thread to do it and
> > then block until the request is processed. I can't think of any other way to
> > do it safely.
>
> > I encourage you to use NPN_PluginThreadAsyncCall at a higher level if you
> > can. NPN_PluginThreadAsyncCall is pretty well supported. Firefox 3+,
> > Safari and Chrome all have it. The reason we support browser's without it is
> > I think just for Firefox 2. That might not be a concern for you?
>
> > Also, if you need to do something similar in IE, you know you're in Windows
> > so PostMessage is available. That's how we emulate NPN_PluginThreadAsyncCall
> > for O3D in IE.
>
> > Al
>

> > On Mon, Oct 26, 2009 at 6:29 PM, Aaron Drew <aaron...@gmail.com> wrote:
>
> >> Hi Alastair,
>
> >> Javascript initiated actions will always occur in the UI thread so
> >> thread-safety is not an issue in that direction. The NPN functions will
> >> always be called from the correct thread.
>
> >>  Plugin-initiated actions are limited to callback functions and,
> >> depending on the binding model, referencing and dereferencing
> >> (NPN_RetainObject, NPN_ReleaseObject) of class instances.
>
> >> The aim of the patch would be to ensure that you can safely initiate a
> >> callback or retain/release an instance of something from any thread.
>
> >> I am a bit concerned though that there is no guaranteed way to do this if
> >>  NPN_PluginThreadAsyncCall is not supported by the browser. Posting
> >> messages, as you say, only works on Windows. Should I forget about this for
> >> now?
>
> >> - Aaron
>

> >> On Tue, Oct 27, 2009 at 4:12 AM, Alastair Patrick <apatr...@google.com>wrote:
>
> >>> Hi Aaron,
>
> >>> What would be the benefit of the patch if it doesn't give us thread
> >>> safety? There doesn't seem to be any need for NPCallback to be an NPObject
> >>> but I don't think that is really worth the effort if that's the only
> >>> advantage.
>
> >>> Don't assume anything in nixysa is thread safe. It wasn't designed with
> >>> that in mind. The only NPN function that can be safely called from another
> >>> thread is NPN_PluginThreadAsyncCall. The NPVARIANT_* macros should also be
> >>> okay I think because they don't make NPN calls. You can inspect the nixysa
> >>> generated code to see if you think it's thread safe but it makes a lot of
> >>> NPN calls.
>
> >>> Al
>

> >>> On Mon, Oct 26, 2009 at 11:40 AM, Aaron Drew <aaron...@gmail.com> wrote:
>
> >>>> Ah! I see what you mean now. That explains why O3D hasn't hit the same
> >>>> problems as I have regarding calls to NPN_RetainObject from a local thread.
>
> >>>> I've been learning NPAPI along the way and initially it didn't occur to
> >>>> me to do anything besides NPN_InvokeDefault from within the function that
> >>>> NPN_PluginThreadAsyncCall triggers.
>
> >>>> I certainly can fix my code. I was under the impression though that
> >>>> nixysa was "broken" in the sense that I assumed it was designed to be thread
> >>>> safe for some reason. Aside from asynchronous callbacks, is there anywhere
> >>>> else it is _not_ thread-safe within nixysa generated code? Is thread-safe
> >>>> operation a target for the nixysa project?
>
> >>>> Should I revise (read: simplify) my patch and resubmit?
>
> >>>> - Aaron
>

> >>>> On Tue, Oct 27, 2009 at 2:28 AM, Alastair Patrick <apatr...@google.com>wrote:
>
> >>>>> Hi Aaron,
>
> >>>>> I think the difference is O3D isn't calling (or shouldn't be calling)
> >>>>> async callbacks off the UI thread. We're only using it to defer callbacks on
> >>>>> the UI thread.
>
> >>>>> Would it be possible to move the asynchronous call to to a higher level
> >>>>> in your application? For example, have the application call
> >>>>> NPN_PluginThreadAsyncCall to invoke a C++ function on the UI thread and then
> >>>>> have that invoke the JavaScript callback? It wouldn't even need to be
> >>>>> [async] then. We do that in O3D to signal the completion of gzip
> >>>>> decompression and tar decoding on a worker thread I believe. You could get
> >>>>> the same effect on browsers that do not support NPN_PluginThreadAsyncCall by
> >>>>> using PostMessage to send your plugin window a message (on Windows at
> >>>>> least). Better to use NPN_PluginThreadAsyncCall is it's available though.
>
> >>>>> Al
>

[Aaron]

Hi Alistair,

Looking at this again, it seems to me like it would be very difficult
to cleanly pass control to the plugin thread via
NPN_PluginThreadAsyncCall without making changes to nixysa. Are such
changes out of the question for the official codebase?

From what I can imagine, the 80-90% use-case for using callbacks (and
the basis for my initial misunderstanding) is where a worker thread
wants to call a JavaScript function.

Heres another quick and ugly patch that changes RunCallback() such
that all arguments are copied and the NPAPI code is deferred to the
plugin thread.

- Aaron

--- nixysa/npapi_generator.py (revision 57)
+++ nixysa/npapi_generator.py (working copy)
@@ -818,8 +818,11 @@
_namespace_glue_cpp_tail]))

_callback_glue_cpp_template = string.Template("""
-${RunCallback} {
+${AsyncCallbackStruct}
+
+${AsyncRunCallback} {
${StartException}
+ ${AsyncCallbackStructToParams}
const char *error=NULL;
const char **error_handle = &error;
bool success = true;
@@ -861,6 +861,17 @@
return ${ReturnValue};
${EndException}
}
+
+${RunCallback} {
+ ${ParamsToAsyncCallbackStruct}
+ if (async && NPCallback::SupportsAsync()) {
+ NPN_PluginThreadAsyncCall(npp, &AsyncRunCallback, asyncargs);
+ } else {
+ AsyncRunCallback(asyncargs);
+ }
+ return ${ReturnValue};
+}
+
""")

_callback_no_param_glue_cpp_template = string.Template("""
@@ -1843,6 +1847,30 @@
run_callback = ('%s RunCallback(NPP npp, NPObject *npobject, bool
async%s)'
% (return_type_string, ', '.join(param_strings)))

+ async_run_callback = '%s AsyncRunCallback(void *data)' %
(return_type_string)
+ async_callback_struct = "struct AsyncArgs { \n"
+ async_callback_struct+= " AsyncArgs() {};\n"
+ async_callback_struct+= " NPP npp;\n"
+ async_callback_struct+= " NPObject *npobject;\n"
+ async_callback_struct+= " bool async;\n"
+ for i in obj.params:
+ async_callback_struct += " %s %s;\n" %
(i.type_defn.binding_model.CppMemberString(scope, i.type_defn)[0],
i.name)
+
+ async_callback_struct += "};"
+ async_callback_struct_to_params = "AsyncArgs *asyncargs =
(AsyncArgs *)data;\n"
+ async_callback_struct_to_params+= " NPP npp = asyncargs->npp;\n"
+ async_callback_struct_to_params+= " NPObject *npobject =
asyncargs->npobject;\n"
+ async_callback_struct_to_params+= " bool async = asyncargs-
>async;\n"
+ for i in obj.params:
+ async_callback_struct_to_params += " %s %s = asyncargs->%s;\n"
% (i.type_defn.binding_model.CppMemberString(scope, i.type_defn)[0],
i.name, i.name)
+ async_callback_struct_to_params += "delete asyncargs;"
+
+ params_to_async_callback_struct = "AsyncArgs *asyncargs = new
AsyncArgs();\n"
+ params_to_async_callback_struct+= " asyncargs->npp = npp;\n"
+ params_to_async_callback_struct+= " asyncargs->npobject =
npobject;\n"
+ params_to_async_callback_struct+= " asyncargs->async = async;\n"
+ params_to_async_callback_struct+= "%s" % "\n".join([" asyncargs->
%s = %s;" % (i.name, i.name) for i in obj.params])
+
return_eval, return_value = bm.NpapiFromNPVariant(scope,
return_type,
'result',
'retval',
'success',
@@ -1851,6 +1879,10 @@
start_exception, end_exception = GenExceptionContext(
_exception_macro_name, "callback return value", "<no name>")
subst_dict = {'RunCallback': run_callback,
+ 'AsyncRunCallback': async_run_callback,
+ 'AsyncCallbackStruct': async_callback_struct,
+ 'ParamsToAsyncCallbackStruct':
params_to_async_callback_struct,
+ 'AsyncCallbackStructToParams':
async_callback_struct_to_params,
'ArgCount': str(len(obj.params)),
'ParamsToVariantsPre': '\n'.join
(param_to_variant_pre),
'ParamsToVariantsPost': '\n'.join
(param_to_variant_post),