[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 maxcarl...@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: [email protected]
>>>>> 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
>>>>>
>>>
>
