Opened 4 years ago

Closed 4 years ago

#18637 closed defect (fixed)

aspect.before and aspect.after can cause errors if multiple aspect handles are removed within a handler

Reported by: Kenneth G. Franqueiro Owned by: Kenneth G. Franqueiro
Priority: undecided Milestone: 1.8.11
Component: Core Version: 1.10.4
Keywords: Cc:
Blocked By: Blocking:

Description

This is probably easiest to explain with a couple of examples.

var handle1 = aspect.before(obj, 'method', function () {
	// ...
});

var handle2 = aspect.before(obj, 'method', function () {
	// ...
	handle2.remove();
	handle1.remove();
});

obj.method(); // throws error
var handle1 = aspect.after(obj, 'method', function () {
	handle1.remove();
	handle2.remove();
	// ...
});

var handle2 = aspect.after(obj, 'method', function () {
	// ...
});

obj.method(); // throws error

If you reverse the order in which the handles are removed, the error does not occur.

I believe what's happening is that in these cases, when you remove the handles in this order, the handle from the first removed aspect still maintains a reference to the second removed aspect, and it still attempts to run that as a result when the aspect code navigates to next. When removed in the reverse order, all of the next and previous references that need to be updated end up properly updated.

This can be prevented with a couple of checks in the respective code paths, since the error that occurs is always due to the advice function being null. I'll be submitting a PR against master shortly, including tests.

Backporting this will require some additional work to backport the tests to the DOH modules.

Change History (3)

comment:1 Changed 4 years ago by Kenneth G. Franqueiro

Milestone: tbd1.11
Owner: set to Kenneth G. Franqueiro
Status: newassigned

comment:2 Changed 4 years ago by Kenneth G. Franqueiro

Milestone: 1.111.10.5

Merged to master via https://github.com/dojo/dojo/pull/160. Needs backports.

comment:3 Changed 4 years ago by Kenneth G. Franqueiro

Milestone: 1.10.51.8.11
Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.