[Laszlo-dev] The contract of LzNode/destroy [Was For Review: Change 20100421-maxcarlson-v Summary: Update basecomponent to not set styles, construct() to automatically halt for deleted nodes]

[P T Withington]

[Adding laszlo-dev]

Actually, most of this (null pointer errors on deleted nodes) is fallout from our replicator/pooling strategy where we try to outwit the Javascript garbage collector.

The _requirement_ we have is that one be able to call `destroy` on a node and know that after that call, if you drop any references to the node that _you_ (the OL developer) created, the node will be garbage collected.

Hence `destroy`, internally to the LFC, must arrange to clean up any references to the node that would cause the node to "leak".  Any references to the node created by internal LFC bookkeeping (that the OL developer cannot know about because they are private), must be cleaned up by `destroy` and the __LZdeleted flag must ensure that any LFC methods that would "resurrect" the zombie node are prevented from doing so.

If there is non-LFC code that can cause resurrection on zombie nodes, that is the developer's responsibility.  Similarly, if the developer's code is sloppy enough to invoke methods on zombie nodes that fail because the node is no longer valid, we can't really protect them from that.

So:  I agree that for non-LFC code, operating on a destroyed node is undefined.  You can't do that.  But, within the LFC, we need to protect ourselves and ensure that the LFC does not either leave a destroyed node on life-support forever (i.e., leak) nor does it resurrect zombies (i.e., re-attach a node to some internal bookkeeping, such as events, that should have been dropped).

Given that the OL developer will use replication and that replication can magically destroy a node, I'm thinking that maybe there does need to be a public API for developers to ask if a node has been destroyed.

On 2010-04-21, at 20:16, Max Carlson wrote:

> I think 1) the behavior should be undefined.  We have two ways to determine a node has been deleted - override destroy() or listen to the ondestroy() event.  This allows 2) Destroyed objects try to ignore any invalid calls - but it puts the burden on the developer.  I don't know what we can do other than minimize the impact on a case-by-case basis.
> 
> Most of this is fallout from the components having been developed for AS2 where NPEs are much less serious.  We'll make the new components more robust!
> 
> On 4/21/10 4:18 PM, André Bargull wrote:
>> Well, this is the question we need to answer at first:
>> What is the expected behaviour if you call a method on a destroyed object?
>> There are two possibilities:
>> 1) The behaviour is undefined, because you must not call a method on a
>> destroyed object, it's simply forbidden. That means the caller is
>> responsible for any effects.
>> 2) Destroyed objects try to ignore any invalid calls, if possible. That
>> means the callee is responsible.
>> 
>> 
>> I've used "setStyle(..)" only for demonstration purposes, here are a few
>> other components and different methods. So it's not too difficult to
>> provoke null pointer exceptions in the components.
>> <canvas debug="true">
>> <dataset name="ds"><e/></dataset>
>> <grid id="mygrid"/>
>> <edittext id="myedit"/>
>> <datacombobox id="mydcmb"/>
>> <tree id="mytree"/>
>> <handler name="oninit">
>> var g = mygrid;
>> g.destroy();
>> g.clearSort();
>> 
>> var e = myedit;
>> e.destroy();
>> e.getText();
>> 
>> var d = mydcmb;
>> d.destroy();
>> d.getText();
>> 
>> var t = mytree;
>> t.destroy();
>> t.getChildIndex(null);
>> </handler>
>> </canvas>
>> 
>> 
>> 
>> 
>> On 4/22/2010 12:14 AM, Max Carlson wrote:
>>> On 4/21/10 2:54 PM, André Bargull wrote:
>>>> Not approved.
>>>> 
>>>> 1) __LZdeleted is LFC-internal, it must not be used in the components
>>> 
>>> Should we have a public flag, or make __LZdeleted public?
>>> 
>>> Otherwise, I can't see how to ignore the setStyle() call in your
>>> testcase at LPP-8880:
>>> c.destroy();
>>> // setStyle() calls _applystyle() later
>>> c.setStyle({textcolor: 0});
>>> 
>>>> 2) strict equality check to avoid string coercion, re-throw error if e
>>>> !=== '__LZdeleted'!
>>>>> + } catch(e) {
>>>>> + // Construct may, through many tangled webs of replication and
>>>>> + // placement, actually end up deleting us! Bail out completely.
>>>>> + if (e == '__LZdeleted') {
>>>>> + return;
>>>>> + }
>>>>> + }
>>> 
>>> Will do.
>>> 
>>>> 3) There's an important devnote right before the added
>>>> "throw('__LZdeleted');":
>>>>> // @devnote only add to subnodes if this node is not deleted which
>>>>> // may happen as a side-effect of calling determinePlacement().
>>>>> // We still need to set 'immediateparent' because legacy constructors
>>>>> // expect to see this property.
>>> 
>>> This shouldn't matter anymore, because the throw() should prevent any
>>> super calls from continuing... I'll remove/update the devnote.
>>> 
>>>> On 4/21/2010 11:21 PM, Max Carlson wrote:
>>>>> Change 20100421-maxcarlson-v by maxcarlson at friendly on 2010-04-21
>>>>> 14:09:52 PDT
>>>>> in /Users/maxcarlson/openlaszlo/trunk-clean
>>>>> for http://svn.openlaszlo.org/openlaszlo/trunk
>>>>> 
>>>>> Summary: Update basecomponent to not set styles, construct() to
>>>>> automatically halt for deleted nodes
>>>>> 
>>>>> Bugs Fixed: LPP-8880 - ERROR @lz/textlistitem.lzx?23: TypeError: Error
>>>>> #1009 in swf10, LPP-8929 - Prevent construction of destroyed LzNode
>>>>> subclasses
>>>>> 
>>>>> Technical Reviewer: andre.bargull at udo.edu
>>>>> QA Reviewer: ptw
>>>>> 
>>>>> Details: LzNode - Throw an exception when __LZdeleted is true in
>>>>> LzNode.construct().
>>>>> 
>>>>> LzInputText, LzText, LaszloView - Remove unneeded __LZdeleted test in
>>>>> construct().
>>>>> 
>>>>> textlistitem - Remove unneeded test.
>>>>> 
>>>>> basecomponent - Don't try to set styles for deleted components.
>>>>> 
>>>>> Tests: See LPP-8880,
>>>>> examples/components/component_sampler.lzx?debug=true in all runtimes
>>>>> 
>>>>> Files:
>>>>> M WEB-INF/lps/lfc/core/LzNode.lzs
>>>>> M WEB-INF/lps/lfc/views/LzInputText.lzs
>>>>> M WEB-INF/lps/lfc/views/LzText.lzs
>>>>> M WEB-INF/lps/lfc/views/LaszloView.lzs
>>>>> M lps/components/lz/textlistitem.lzx
>>>>> M lps/components/base/basecomponent.lzx
>>>>> 
>>>>> Changeset:
>>>>> http://svn.openlaszlo.org/openlaszlo/patches/20100421-maxcarlson-v.tar
>>>>> 
>>> 
>