Closed Bug 301994 Opened 19 years ago Closed 18 years ago

CRASH trying to set value on a node that is part of the value

Categories

(Core Graveyard :: XForms, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aaronr, Assigned: msterlin)

References

Details

(Keywords: fixed1.8.0.5, fixed1.8.1)

Attachments

(4 files, 2 obsolete files)

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.
Attached file testcase (obsolete) —
click on an item in the select.  It will cause an infinite loop and browser
crash
Attached file tweaked testcase
Attachment #190376 - Attachment is obsolete: true
I haven't tested it myself, but that's what I note in bug 300591.
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: aaronr → allan
Status: NEW → ASSIGNED
Attachment #193806 - Flags: review?(aaronr)
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
Attachment #193806 - Attachment is obsolete: true
Attachment #193806 - Flags: review?(aaronr)
Priority: -- → P2
Hardware: PC → All
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.
Assignee: allan → msterlin
Status: ASSIGNED → NEW
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.
Attachment #214590 - Flags: review?(smaug)
(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.
Attached file testcase2
This testcase won't work with this patch and I think that it should be able to.
Status: NEW → ASSIGNED
Comment on attachment 214590 [details] [diff] [review]
Make nsXFormsSetValueElement::HandleAction non-reentrant for xforms-value-changed event

because of Aaron's testcase -r
Attachment #214590 - Flags: review?(smaug) → review-
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.
Depends on: 300591
(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.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
Keywords: fixed1.8.1
Keywords: fixed1.8.0.5
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.