Skip to content

Commit

Permalink
Avoid the possibility of infinite loops (in JS only, SWF untested) wi…
Browse files Browse the repository at this point in the history 
…th xml watchers. This can happen if changes are made in watcher functions.
  • Loading branch information
@greg-dove
greg-dove committed Apr 6, 2022
1 parent 13b1158 commit acab343
Showing 1 changed file with 59 additions and 12 deletions.
71 changes: 59 additions & 12 deletions frameworks/projects/MXRoyaleBase/src/main/royale/mx/utils/XMLNotifier.as
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,14 @@
package mx.utils
{

import org.apache.royale.utils.ObjectMap;
//import org.apache.royale.utils.ObjectMap;
import mx.core.mx_internal;
import mx.utils.IXMLNotifiable;

COMPILE::SWF{
import flash.utils.Dictionary;
}

use namespace mx_internal;

/**
Expand Down Expand Up @@ -98,13 +102,32 @@ public class XMLNotifier
{
callee = notificationFunction;
}
var xmlWatchers:ObjectMap = callee["watched"];
if (xmlWatchers != null)
{
xmlWatchers.forEach( function(truevalue:Object,notifiable:Object,map:Object):void {
//var xmlWatchers:ObjectMap = callee["watched"];

COMPILE::SWF{
var xmlWatchers:Dictionary = callee["watched"];
for (var notifiable:Object in xmlWatchers) {
IXMLNotifiable(notifiable).xmlNotification(currentTarget, ty, tar, value, detail);
} );
}
}

COMPILE::JS{
var xmlWatchers:Map = callee["watched"];
if (xmlWatchers != null)
{
var collected:Array = [];
//note, if we don't collect these first and try to iterate directly, then there can be the case that iterating is infinite if the function caLL also somehow affects the xmlWatchers Map:
xmlWatchers.forEach( function(truevalue:Object,notifiable:Object,map:Object):void {
collected.push(notifiable);
} );

while(collected.length){
IXMLNotifiable(collected.shift()).xmlNotification(currentTarget, ty, tar, value, detail);
}
}
}


}

return notificationFunction;
Expand Down Expand Up @@ -181,13 +204,25 @@ public class XMLNotifier
}

// Watch lists are maintained on the notification function.
var xmlWatchers:ObjectMap;
if (watcherFunction["watched"] == undefined)
watcherFunction["watched"] = xmlWatchers = new ObjectMap(true,true);
if (watcherFunction["watched"] == undefined) {
// watcherFunction["watched"] = xmlWatchers = new ObjectMap(true,true);
COMPILE::SWF{
var xmlWatchers:Dictionary = watcherFunction["watched"] = new Dictionary();
}

COMPILE::JS{
var xmlWatchers:Map= watcherFunction["watched"] = new Map();
}
}
else
xmlWatchers = watcherFunction["watched"];

xmlWatchers.set(notifiable, true);
COMPILE::SWF{
xmlWatchers[notifiable]=true;
}
COMPILE::JS{
xmlWatchers.set(notifiable, true);
}
}
}

Expand Down Expand Up @@ -223,12 +258,24 @@ public class XMLNotifier
if (!(watcherFunction is Function))
return;

var xmlWatchers:ObjectMap;
COMPILE::SWF{
var xmlWatchers:Dictionary;
}

COMPILE::JS{
var xmlWatchers:Map;
}

if (watcherFunction["watched"] != undefined)
{
xmlWatchers = watcherFunction["watched"];
xmlWatchers.delete(notifiable);
COMPILE::SWF{
delete xmlWatchers[notifiable];
}
COMPILE::JS{
xmlWatchers.delete(notifiable);
}

}
}
}
Expand Down

10 comments on commit acab343

@estanglerbm
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you give an example of the problem?

And couldn't this be fixed in ObjectMap, to keep things simple? I went through a lot of trouble to unify this, and if there's a problem, shouldn't it be addressed in ObjectMap (since other uses could encounter the same problem)? (like addressing it in ObjectMap.forEach)

Aside: when MXRoyale and MXRoyaleBase split, how did we lose the history of files like this?

@greg-dove
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Edward, I'll get back to you over the weekend for the example It will take a while to make a minimal repro. Just thinking about it more has made me consider a specific difference that could be happening because Map is ordered and Dictionary is not.
I personally think ObjectMap does not add value here, only overhead. It certainly made it more difficult to debug with extra layers in the stack, but I take your point about the need for fixing it. I suspect ObjectMap was more valuable in the days when we were supporting IE versions < 11, but that is no longer the case. If we ignore IE11 going forward then it will be possible to more accurately emulate the original flash Dictionary with a new approach that includes weak keys and iteration, using modern browser features, which I hope to get to in the coming months (performance of such may end up being slower in JS, but I think it could be much closer in behaviour compared to the orignal flash version).
At the moment only TLF lib is really using ObjectMap within the framework, apart from this use in XMLNotifier, other code sites are doing the explicit lower level variations using Map/WeakMap and Dictionary, so there is no obligation to use it. However if you strongly believe the ObjectMap should be used here, then I will restore it, so please confirm.

In terms of the original commit history, I do have that visible in my Intellij IDE git history, which follows the file move commit back to the original location and history. I am not sure why you cannot. I will research this more over the weekend to see if I can find clues.

@estanglerbm
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the file history, I'm just going by what I see in GitHub when viewing the (new location) file and looking at history. Nothing there before the split.

Yes, I believe strongly in keeping ObjectMap, since it's basically needed wherever Dictionary was needed in Flex. It's much easier than rolling something in each place. ObjectMap can gladly use WeakMap, except when enumeration is needed; it uses Map if enumeration is needed, since it wasn't possible on WeakMap until recently (with WeakRefs + etc.), and even then, I'm not sure if iterable WeakMap would be a good thing. Basically, this stuff is really difficult to get right (at least for me), and I think it would be best to hide it behind ObjectMap or something similar.

But if your example shows something not covered, I could change my mind. (I haven't changed my mind looking at the code you added so far, which seems much more complicated.) And of course, ObjectMap's implementation could probably be simplified (I just used the existing implementation when I made changes; I added the forEach implementations).

Anyway, that's just my two cents.

@yishayw
Copy link
Contributor

@yishayw yishayw commented on acab343 Apr 8, 2022 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@estanglerbm
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yishayw Yeah, --follow works. So TortoiseGit requires a special checkbox, and I guess GitHub doesn't support it.

@greg-dove
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@estanglerbm I didn't get a minimal repro yet, I will investigate in-app tomorrow (it is a client app on a client computer, and I don't have access to the code on my regular work machine which I use for Royale framework/compiler work). If need be I can revert it in Royale (and restore a monkey patch in the client app) until I can get more details. I can see some other errors which I will address in the compiled code for XMLNotifier, (the iteration loops for XMLList are not correctly expressed because the loop target is untyped) but I don't believe they were relevant to the infinite loop issue I was seeing.

@estanglerbm
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't seem easy to accidentally cause an infinite loop in ObjectMap.forEach. For example:

trace("start");
				
var m:ObjectMap = new ObjectMap(true, true);

var a:Object = new A();
var b:Object = new B();
var c:Object = new C();

m.set(a, true);
m.set(b, true);

var i:int = 0;
m.forEach( function(v:Object,n:Object,map:Object):void {
	trace("forEach: " + n + "=" + v);

	if (!i) m.set(c, true);
	if (!i) m.delete(b);
	++i;
} );

trace("finish");

(The A, B, and C have a toString() returning "A", "B", etc.)

This produces the same result (A = true, C = true) in both SWF and JS.

That is, Dictionary + for-in loop doesn't make a copy of the mutating map, on the SWF side. So, was the original app code using XMLNotifier in Flex correct to begin with? It feels like it would have generated an infinite loop in Flex, not just Royale. [I have not tested real XMLNotifier from Flex to see how it handles mutations, though. But the implementation of ObjectMap.forEach() in SWF is the same.]

(If protecting against a mutating map is an enhancement in Royale, then we can easily create an ObjectMap.forEachMutating() or such, which does a "new Map(oldMap).forEach" or something like that.)

@greg-dove
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I also did something similar with Dictionary and Map to check as well and arrived at the same conclusion. I was seeing the infinite loop cycling through ObjectMap and believing it was the cause, but it was a symptom. There was no infinite loop in Flex with the same code, so it is something to do with the emulation differences in part of the related code. Are you ok if I revert it during the next couple of days? We can discuss the ObjectMap/no-ObjectMap in this scenario separately at some later point. For now I want to track down the root cause of the looping.

@estanglerbm
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whatever you want to do is fine. I have a branch in my repo where I've reverted the changes and am ready to make changes to ObjectMap, if needed, but I just need a test case that shows the problem. I figured it was either some problem that happened in the original Flex, too, or something very subtle.

@greg-dove
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@estanglerbm hopefully you saw that I reverted that change last week. I did manage to finally create a minimal repro showing some of the issues I am working on including that infinite loop issue, in case you are interested: #1198 I will continue working on that tomorrow (unless someone beats me to it)

Please sign in to comment.