Last Comment Bug 301994 - CRASH trying to set value on a node that is part of the value
: CRASH trying to set value on a node that is part of the value
Status: RESOLVED FIXED
: fixed1.8.0.5, fixed1.8.1
Product: Core
Classification: Components
Component: XForms (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: ---
Assigned To: Merle Sterling
: Stephen Pride
Mentors:
Depends on: 300591
Blocks:
  Show dependency treegraph
 
Reported: 2005-07-24 23:28 PDT by aaronr
Modified: 2006-06-06 06:57 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (1.25 KB, application/xhtml+xml)
2005-07-24 23:30 PDT, aaronr
no flags Details
tweaked testcase (1.24 KB, application/xhtml+xml)
2005-07-24 23:31 PDT, aaronr
no flags Details
Patch (2.37 KB, patch)
2005-08-25 06:01 PDT, Allan Beaufour
no flags Details | Diff | Review
testcase without select (956 bytes, application/xhtml+xml)
2006-02-21 15:54 PST, aaronr
no flags Details
Make nsXFormsSetValueElement::HandleAction non-reentrant for xforms-value-changed event (2.68 KB, patch)
2006-03-09 11:52 PST, Merle Sterling
bugs: review-
Details | Diff | Review
testcase2 (1.28 KB, application/xhtml+xml)
2006-03-13 19:15 PST, aaronr
no flags Details

Description aaronr 2005-07-24 23:28:49 PDT
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.
Comment 1 aaronr 2005-07-24 23:30:13 PDT
Created attachment 190376 [details]
testcase

click on an item in the select.  It will cause an infinite loop and browser
crash
Comment 2 aaronr 2005-07-24 23:31:40 PDT
Created attachment 190377 [details]
tweaked testcase
Comment 3 Allan Beaufour 2005-07-25 00:55:54 PDT
I haven't tested it myself, but that's what I note in bug 300591.
Comment 4 Allan Beaufour 2005-08-25 06:01:41 PDT
Created attachment 193806 [details] [diff] [review]
Patch

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.
Comment 5 Allan Beaufour 2005-08-25 06:06:22 PDT
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 6 Allan Beaufour 2005-08-25 06:42:35 PDT
Comment on attachment 193806 [details] [diff] [review]
Patch

ignore it, does not work
Comment 7 aaronr 2006-02-21 15:54:59 PST
Created attachment 212656 [details]
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.
Comment 8 aaronr 2006-02-21 16:03:03 PST
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.
Comment 9 Allan Beaufour 2006-02-22 00:01:37 PST
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.
Comment 10 Merle Sterling 2006-03-09 11:52:15 PST
Created attachment 214590 [details] [diff] [review]
Make nsXFormsSetValueElement::HandleAction non-reentrant for xforms-value-changed event

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.
Comment 11 Olli Pettay [:smaug] 2006-03-09 12:16:50 PST
(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.
Comment 12 Allan Beaufour 2006-03-10 00:04:58 PST
(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 13 Olli Pettay [:smaug] 2006-03-12 03:11:42 PST
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.
Comment 14 Merle Sterling 2006-03-13 10:46:01 PST
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.

Comment 15 aaronr 2006-03-13 11:06:00 PST
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.
Comment 16 aaronr 2006-03-13 19:15:47 PST
Created attachment 214963 [details]
testcase2

This testcase won't work with this patch and I think that it should be able to.
Comment 17 Olli Pettay [:smaug] 2006-03-28 06:46:54 PST
Comment on attachment 214590 [details] [diff] [review]
Make nsXFormsSetValueElement::HandleAction non-reentrant for xforms-value-changed event

because of Aaron's testcase -r
Comment 18 aaronr 2006-04-03 12:49:40 PDT
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.
Comment 19 Allan Beaufour 2006-05-17 04:24:47 PDT
(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".
Comment 20 Allan Beaufour 2006-05-17 04:30:13 PDT
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.
Comment 21 Allan Beaufour 2006-05-24 02:30:30 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.