373073 - calIItipProcessor needs to be stateless

Status: RESOLVED FIXED

(Calendar :: Lightning Only, defect)

Description

[cmtalbert]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1.2) Gecko/20070219 Firefox/2.0.0.2
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1.3pre) Gecko/20070306 Calendar/0.5pre

Regarding http://lxr.mozilla.org/mozilla1.8/source/calendar/base/src/calItipProcessor.js

> > The itip processor's implementation is stateful. Reading the idl, IMO users 
> > are
> > tempted to use it as a (shared) service, which I would appreciate BTW.
> > Moreover, I see no strong reason to keep that processing data as member of the
> > processor object. All that helper (member) functions can get those by 
> > argument.
> Here is where we disagree.  There is no way that a caller function can retrieve
> those attributes -- they are not defined in the IDL. The only public interface
> into the calIItipProcessor interface is ProcessItipItem.  So, while the
> calItipProcessor does have internal states, I don't see how that matters w.r.t.
> calling clients.  If another client were to call ProcessItipItem, that client
> would send in all this information encapsulated in a calItipItem and these
> variables would be overwritten with the new information.  Perhaps this is
> something about JS that I'm not familiar with.  Those variables are intended to
> be private members on the implementation.
> 
> In C++ those variables were private member variables on the implementation.  I
> think having them as such will help keep the functions simpler as we increase
> complexity by supporting more of the iTIP state machine.

My point wasn't about that clients could access that state information, but
about that they are tempted to use the processor object in a shared fashion,
which has been my first impression: a clean static stateless object I just get
once via getService() and use forever. Any runtime parameters I pass to it upon
processItipItem are thread-local. Moreover, I really didn't see any urging
reason why to save the runtime parameters at the processor object, than just
convenience /no need to explicitly pass them to the inner helper functions.

My above comment is just a recommendation. If you stick to the current
implementation, then please comment in the IDL that users must not use
processors as a service; that the object is stateful.

This is a follow on bug for this issue, so that this issue could be addressed in a more manageable patch than the one on bug 334685.

Reproducible: Always

Steps to Reproduce:
1. Use calIItipProcessor object in one thread, creating with getService
2. Use another instance on another thread, creating with getService

Actual Results:  
Race conditions and unreliable internal states.

Expected Results:  
Should be ok with being used this way.

Comment 1

[cmtalbert]

Attachment: removes all member variables from calItipProcessor

This patch addresses Daniel's concerns by removing all the member variables as he suggested.  It parametrizes all the private methods instead.

Comment 2

[Matthew (lilmatt) Willis]

Comment on attachment 257779 [details] [diff] [review]
removes all member variables from calItipProcessor

>-    _processCalendarAction: function cipPCA(aCalItem, aOperation, aListener) {
>+    _processCalendarAction: function cipPCA(aCalItem,
>+                                            aOperation,
>+                                            aTargetCalendar,
>+                                            aListener) {
>         switch (aOperation) {
>             case CAL_ITIP_PROC_ADD_OP:
>                 // We assume all adds take place on the target calendar
>-                this.mTargetCalendar.addItem(aCalItem, aListener);
>+                aTargetCalendar.addItem(aCalItem, aListener);


The "We assume all adds..." comment doesn't make as much sense now that we're passing in the target calendar we want to add to.  I'd pull that.

r=lilmatt with that

Comment 3

[cmtalbert]

Checked in on MOZILLA_1_8_BRANCH and trunk --> Fixed