Closed
Bug 301994
Opened 19 years ago
Closed 19 years ago
CRASH trying to set value on a node that is part of the value
Categories
(Core Graveyard :: XForms, defect, P2)
Core Graveyard
XForms
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.
click on an item in the select. It will cause an infinite loop and browser
crash
Attachment #190376 -
Attachment is obsolete: true
Comment 3•19 years ago
|
||
I haven't tested it myself, but that's what I note in bug 300591.
Comment 4•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
Comment on attachment 193806 [details] [diff] [review]
Patch
ignore it, does not work
Attachment #193806 -
Attachment is obsolete: true
Attachment #193806 -
Flags: review?(aaronr)
Updated•19 years ago
|
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
Comment 9•19 years ago
|
||
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.
Assignee | ||
Comment 10•19 years ago
|
||
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)
Comment 11•19 years ago
|
||
(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•19 years ago
|
||
(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•19 years ago
|
||
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.
Assignee | ||
Comment 14•19 years ago
|
||
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.
Reporter | ||
Comment 15•19 years ago
|
||
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.
Reporter | ||
Comment 16•19 years ago
|
||
This testcase won't work with this patch and I think that it should be able to.
Updated•19 years ago
|
Status: NEW → ASSIGNED
Comment 17•19 years ago
|
||
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-
Reporter | ||
Comment 18•19 years ago
|
||
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
Comment 19•19 years ago
|
||
(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•19 years ago
|
||
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•19 years ago
|
||
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: 19 years ago
Resolution: --- → FIXED
Updated•19 years ago
|
Whiteboard: xf-to-branch
Updated•19 years ago
|
Keywords: fixed1.8.1
Updated•19 years ago
|
Keywords: fixed1.8.0.5
Updated•18 years ago
|
Whiteboard: xf-to-branch
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•