Opened 6 years ago

Closed 6 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)

sample.html​ (1.3 KB) - added by ido 6 years ago.

Download all attachments as: .zip

Change History (13)

Changed 6 years ago by ido

Attachment: sample.html​ added

comment:1 Changed 6 years ago by ido

Here is the pull request : ​https://github.com/dojo/dijit/pull/24

Can you update the title with [patch][cla] ?

Thanks

comment:2 Changed 6 years ago by bill

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 6 years ago by ido

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 6 years ago by bill

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 6 years ago by ido

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 6 years ago by bill

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 6 years ago by ido

@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 6 years ago by bill

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 6 years ago by ido

@bill, true, that is simpler and sufficient. I will prepare a patch asap :)

comment:10 Changed 6 years ago by ido

@bill: I've updated the pull request (​https://github.com/dojo/dijit/pull/24/files).

comment:11 Changed 6 years ago by bill

Milestone: tbd → 1.10
Owner: set to bill
Status: new → assigned

comment:12 Changed 6 years ago by Bill Keese <bill@…>

Resolution: → fixed
Status: assigned → closed

In 0169fa4b37922aaf17cd1ebc034fc00b27353878/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled?
Note: See TracTickets for help on using tickets.