Opened 7 years ago
Closed 7 years ago
#17763 closed defect (fixed)
[patch][cla]destroy() can be fired twice when using own()
Reported by: | ido | Owned by: | bill |
---|---|---|---|
Priority: | undecided | Milestone: | 1.10 |
Component: | Dijit | Version: | 1.9.3 |
Keywords: | Cc: | ||
Blocked By: | Blocking: |
Description
Let say you have 2 widgets: A and B.
- A own B
- A manually call B.destroy()
- A.destroy() is called:
Because own() is attached to destroyRecursive and not to destroy, the handlers are not removed when A manually call B.destroy(). Therefore, destroying A will also call B.destroy()
<html> <head> <script type="text/javascript"> dojoConfig = { async: true }; </script> <script src="//ajax.googleapis.com/ajax/libs/dojo/1.9.2/dojo/dojo.js"></script> <script type="text/javascript"> require(['dojo/_base/declare', 'dijit/_WidgetBase'], function(declare, _widgetBase) { var W1 = declare([_widgetBase], { destroy: function() { console.log('destroy W1'); this.inherited(arguments); } }); var W2 = declare([_widgetBase], { postCreate: function() { var w1 = new W1(); this.own(w1); w1.destroy(); }, destroy: function() { console.log('destroy W2'); this.inherited(arguments); } }); var W3 = declare([_widgetBase], { postCreate: function() { var w1 = new W1(); this.own(w1); w1.destroyRecursive(); }, destroy: function() { console.log('destroy W2'); this.inherited(arguments); } }); console.group('W1 manually destroyed (by destroy) by W2, then call W2 destroyed') var w2 = new W2(); w2.destroy(); console.groupEnd(); console.group('W1 manually destroyed (by destroyRecursive) by W2, then call W2 destroyed') var w3 = new W3(); w3.destroy(); console.groupEnd(); }); </script> </head> <body> </body> </html>
Attachments (1)
Change History (13)
Changed 7 years ago by
Attachment: | sample.html added |
---|
comment:1 Changed 7 years ago by
comment:2 Changed 7 years ago by
Summary: | destroy() can be fired twice when using own() → [patch][cla]destroy() can be fired twice when using own() |
---|
Thanks for the patch. I'll take a look in detail when time permits.
The patch is a lot of code, although I agree that the destroy/destroyRecursive duality makes things complicated. At first glance though I'm actually not sure what should be happening. Since B.destroyRecursive() was never called manually , shouldn't A.destroy() call B.destroyRecursive()?
comment:3 Changed 7 years ago by
It isn't an easy question...
The actual problem it that destroyRecursive will execute destroy again...
Maybe we can solve the entire problem by adding
if(this._destroyed) { this.destroy() }
into destroyRecursive function.
Would that be a better approach ?
comment:4 Changed 7 years ago by
Well, this will all be fixed in V2 when we get rid of destroyRecursive().
To be honest, I don't see a reason to update the code before V2.
The simple thing for you to do is to make A manually call B.destroyRecursive() rather than calling B.destroy(). Generally speaking, you should always call destroyRecursive(), never destroy().
Is there some reason you can't do that?
comment:5 Changed 7 years ago by
Well, of course we can. We decided to call destroy() because destroyRecursive will be gone in 2.0 ;) A step too far maybe ;)
comment:6 Changed 7 years ago by
Yes, I think you went a step too far. :-) Having said that, I think we get a similar problem to what you are describing with handles that have both an unlink() and destroy() method (or was it unlink() and remove()?). So the code should probably be more robust in general.
comment:7 Changed 7 years ago by
@bill, a possible way would be to maintain a "handle mapping". For instance:
- if "destroy" is found, handles must be on "destroyRecursive" and "destroy"
- if "destroyRecursive" is found, handles must be on "destroyRecursive" and "destroy"
- if "remove" is found, handles must be on "remove" and "unlink" and "cancel"
- if "cancel" is found, handles must be on "remove" and "unlink" and "cancel"
Something like that... I can propose a patch for that. Do you agree on the principle ?
comment:8 Changed 7 years ago by
I think it's always on destroy() and at most one other thing? If so, a "handle mapping" might be a little complex.
My original thought was to listen for calls to any of the destroy, destroyRecursive, remove, and cancel methods that exist in this.
comment:9 Changed 7 years ago by
@bill, true, that is simpler and sufficient. I will prepare a patch asap :)
comment:10 Changed 7 years ago by
@bill: I've updated the pull request (https://github.com/dojo/dijit/pull/24/files).
comment:11 Changed 7 years ago by
Milestone: | tbd → 1.10 |
---|---|
Owner: | set to bill |
Status: | new → assigned |
comment:12 Changed 7 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Here is the pull request : https://github.com/dojo/dijit/pull/24
Can you update the title with [patch][cla] ?
Thanks