We seem to be getting into an infinite loop because of a setvalue action that is a handler for a xforms-value-chaged event. setvalue will cause a refresh, that will note the bound node is changed, which generates a value changed event, triggering the setvalue action that will cause a refresh, etc. If I had to guess, I'd say that the problem seems to lie in why the MDG thinks that the nodes value has changed even though it is the node change that triggered the initial set value action. Testcase coming.

click on an item in the select. It will cause an infinite loop and browser crash

I haven't tested it myself, but that's what I note in bug 300591.

Problem was that the two refreshes interfered, both appending to the mChangedNodes, thus sending two value-changed events to the select control, creating the infinite loop. This patch makes refresh take a local copy of the changed nodes and clearing the member array. A nsCOMPtr::Swap() would have been nice.

btw, I'm a bit uncertain about the more "generic fix" in bug 300591, so this bug might be revised again when I do that. But let's fix this use case for now at least.

Comment on attachment 193806 [details] [diff] [review] Patch ignore it, does not work

another testcase with similar results. Might be easier to debug since won't have to go through all of select's refresh magic.

reassigning to msterlin. Allan's got a lot on his plate already and this should be pretty straight forward to understand, at least. In the case of the testcase I attached, XSmiles gets an overflow like us, but formsPlayer and Novell both just handle the setvalue once and it appears to stop there.

Ok. For a fix for this use case. But it should really be fixed in bug 300591. Our whole rrr routine needs to be rewritten.

nsXFormsSetValueElement::handleAction is called repeatedly with xforms-value-changed event until we get a StackOverflow, just as described in the intial description of the bug. Fixing by setting a flag when we process the xforms-value-changed event the first time and returning from subsequent invocations of that method. An alternative might have been to unset the eFlag_DISPATCH_VALUE_CHANGED flag in nsXFormsMDGEngine::Recalculate so that ShouldDispatchValueChanged would return false, but being new to the codebase I am unsure of how that might affect other scenarios that cause a recalculate.

(In reply to comment #10) > Created an attachment (id=214590) [edit] Does this handle all cases when there is a separate <setvalue> for for example xforms-recalculate (which is dispatched in nsXFormsSetValueElement::HandleAction) which then modifies the same instance data. Or what if there is only one <setvalue> but that is the handler for many different events (xforms-value-changed events which are dispatched to different targets, or something like that). Allan, has WG discussed or ensured that it is not possible to create loops using action module elements and xml events.

(In reply to comment #11) > (In reply to comment #10) > > Created an attachment (id=214590) [edit] > Does this handle all cases when there is a separate <setvalue> for > for example xforms-recalculate (which is dispatched in > nsXFormsSetValueElement::HandleAction) which then modifies the same > instance data. Or what if there is only one <setvalue> but that > is the handler for many different events (xforms-value-changed events > which are dispatched to different targets, or something like that). I haven't looked at the patch, but as I wrote in comment 9, bug 300591 is where this should get fixed. Aaron & Co wants this usecase fixed first. > Allan, has WG discussed or ensured that it is not possible to create > loops using action module elements and xml events. If you want to create a loop, you can... the RRR routine should be more robust than it is now, but it can not cover stupid forms.

Comment on attachment 214590 [details] [diff] [review] Make nsXFormsSetValueElement::HandleAction non-reentrant for xforms-value-changed event This assumes that nsXFormsSetValueElement::HandleAction is *always* re-called after xforms-value-changed, right? Is that really true? Otherwise this could affect some valid nsXFormsSetValueElement::HandleAction call.

The patch only checks if handleAction has been called with xforms-value-changed multiple times - all other events get through. The other setValue elements in the test case still work.

Ugh, I just thought of a scenario that will bother this patch and that I never considered when talking to Merle. What if I have the same xf:setvalue element as the handler for xforms-value-changed events with two different targets? I'll see if I can't come up with a testcase that demonstrates what I mean.

This testcase won't work with this patch and I think that it should be able to.

Comment on attachment 214590 [details] [diff] [review] Make nsXFormsSetValueElement::HandleAction non-reentrant for xforms-value-changed event because of Aaron's testcase -r

Looks like to fix this will be much more involved than originally anticipated because of the possibility of multiple setvalue event handlers responding to the same event. The approaches we've come up with to battle this are kinda hacky. We'll wait for bug 300591 to be completed and then verify that it does indeed fix this bug.

(In reply to comment #7) > Created an attachment (id=212656) [edit] > testcase without select > > another testcase with similar results. Might be easier to debug since won't > have to go through all of select's refresh magic. I believe that this testcase is invalid. Loops are "valid".

Eh, maybe I've failed to actually read this bug, and just assumed what it was about. Actually, this entire bug is invalid is it not? If you hook up on the xforms-value-change event and change the node that the control is bound to, you will get an infinite loop. "Dispatched in response to: a change to an instance data node bound to a form control." http://www.w3.org/TR/2006/REC-xforms-20060314/slice4.html#evt-valueChanged "This action explicitly sets the value of the specified instance data node." http://www.w3.org/TR/2006/REC-xforms-20060314/slice10.html#action-setvalue Hooking those to up to the same node is an infinite loop, and that is what you get. We should possibly detect that, and not just go looping forever, but it is a faulty form.

This is fixed by bug 300591. That is, the testcases are imho valid XForms, and should go into infinite loops, but the loop detection from bug 300591 catches it and avoids spinning around for all eternity.