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

RESOLVED FIXED

Status

Core Graveyard
XForms
P2
normal
RESOLVED FIXED
12 years ago
11 months ago

People

(Reporter: aaronr, Assigned: Merle Sterling)

Tracking

({fixed1.8.0.5, fixed1.8.1})

Trunk
fixed1.8.0.5, fixed1.8.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
Created attachment 190376 [details]
testcase

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

Comment 2

12 years ago
Created attachment 190377 [details]
tweaked testcase
(Reporter)

Updated

12 years ago
Attachment #190376 - Attachment is obsolete: true

Comment 3

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

Comment 4

12 years ago
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.
Assignee: aaronr → allan
Status: NEW → ASSIGNED
Attachment #193806 - Flags: review?(aaronr)

Comment 5

12 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

12 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

12 years ago
Priority: -- → P2
Hardware: PC → All
(Reporter)

Comment 7

11 years ago
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.
(Reporter)

Comment 8

11 years ago
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

11 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

11 years ago
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.
Attachment #214590 - Flags: review?(smaug)

Comment 11

11 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

11 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

11 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

11 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

11 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

11 years ago
Created attachment 214963 [details]
testcase2

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

Updated

11 years ago
Status: NEW → ASSIGNED

Comment 17

11 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

11 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

11 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

11 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

11 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
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Whiteboard: xf-to-branch

Updated

11 years ago
Keywords: fixed1.8.1

Updated

11 years ago
Keywords: fixed1.8.0.5

Updated

11 years ago
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.