278368 - MDG fails to find all dependencies, breaks refreshing

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Allan Beaufour]

The patch in bug 265467 has a hack that disables the selective refreshing in
nsXFormsModelElement::Revalidate().

Comment 1

[Allan Beaufour]

Attachment: Patch (not done)

Comment 2

[Allan Beaufour]

Attachment: Testcase

Comment 3

[Allan Beaufour]

Attachment: Testcase v2

Added trigger with setvalues without using action

Comment 4

[Allan Beaufour]

Attachment: Patch v2 (still not done)

Simplified the patch a bit. There's a still a problem with output's with
@value, but I'm getting there.

Comment 5

[Allan Beaufour]

(In reply to comment #3)
> Created an attachment (id=175430) [edit]
> Testcase v2
> 
> Added trigger with setvalues without using action

The reason for this is that I'm experiencing weird behaviour with deferred
actions. Using the patch and triggering the setvalues inside the action-block,
only updates some of the outputs. Refreshing the page and trying again may
update some other outputs. Weird weird. At the moment I'm not sure where the bug
is hiding.

Comment 6

[Allan Beaufour]

Attachment: Patch v3

This should take care of it. Only problem is that it's less effective, but
apparently I cannot use the pointers.

Comment 7

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

Comment on attachment 175821 [details] [diff] [review]
Patch v3

>+    nsCOMPtr<nsIDOM3Node> curChanged;
>+    PRBool rebind = PR_FALSE;
>     PRBool refresh = PR_FALSE;
> 
>     for (PRInt32 j = 0; j < mChangedNodes.Count(); ++j) {
>-      curChanged = mChangedNodes[j];
>-
>-      if (curChanged == boundNode) {
>-        refresh = PR_TRUE;
>-        // We cannot break here, as we need to to check for any changed
>-        // dependencies
>-      }
>-
>-      if (depPos == depCount) {
>-        continue;
>-      }
>+      curChanged = do_QueryInterface(mChangedNodes[j]);

Hmm, DOM3Node is a tearoff, so this creates quite many temporary objects.
But what would be a better way to do this (?)


>-    if (   aNode->mEndIndex < 0
>-        || aNode->mStartIndex >= aNode->mEndIndex
>-        || ((PRUint32) aNode->mEndIndex == mCurExprString->Length() && aNode->mStartIndex == 0)) { 
>+    if (aNode->mEndIndex < 0 ||
>+        aNode->mStartIndex >= aNode->mEndIndex) {
nit, remove the newline after ||

r=me, though I don't know the nsXFormsXPathAnalyzer code too well.
(The patch isn't quite up-to-date with trunk.)

Comment 8

[Allan Beaufour]

(In reply to comment #7)
> (From update of attachment 175821 [details] [diff] [review] [edit])
> Hmm, DOM3Node is a tearoff, so this creates quite many temporary objects.
> But what would be a better way to do this (?)

Beats me.

Comment 9

[Allan Beaufour]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 175821 [details] [diff] [review] [edit] [edit])
> > Hmm, DOM3Node is a tearoff, so this creates quite many temporary objects.
> > But what would be a better way to do this (?)
> 
> Beats me.

Fixing bug 235512 might make the previous method work... let's see.

Comment 10

[Allan Beaufour]

Attachment: updated to current CVS

Could you do the 2nd review Aaron?

Comment 11

[aaronr]

Comment on attachment 179839 [details] [diff] [review]
updated to current CVS

+      nsAutoString depNodeName;
+      for (PRUint32 t = 0; t < depCount; ++t) {
+	 nsCOMPtr<nsIDOMNode> tmpdep = deps->ObjectAt(t);
+	 NS_ENSURE_STATE(tmpdep);

I'm not a big fan of bailing in debug code when you don't bail in the regular
code with the same condition.  Could cause havoc if a customer	sees one
behavior in the field and we try to debug it and see a different behavior.

+      if (!refresh && boundNode) {
+	 curChanged->IsSameNode(boundNode, &refresh);
+	 if (refresh)
+	   continue;
       }

I'd like more comments in ::Revalidate.  Especially   what refresh and rebind
are for and why the tests that you are making.	F.x., why does 'refresh' get
set to true but then you do a continue on to the next node?

Comment 12

[Allan Beaufour]

Attachment: Fixed Aaron's comments

Comment 13

[aaronr]

Comment on attachment 180030 [details] [diff] [review]
Fixed Aaron's comments

dandy! r=me

Comment 14

[Allan Beaufour]

Checked in.

Comment 15

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050411
Firefox/1.0+
I am not sure at all, but I think this patch may have caused extension BugMeNot
(http://roachfiend.com/archives/2005/02/07/bugmenot/) to make firefox crash. I
know extensions aren't your responsibility, but I thought I'd let you know
incase it's something you can use to fix stuff.
The crashing is reproducable. Install BugMeNot, go to any webpage, and bam! a crash.

TBID TB5003308K

Apologies if this patch isn't anything to do with what I am seeing.

Comment 16

[Allan Beaufour]

(In reply to comment #15)
> I am not sure at all, but I think this patch may have caused extension BugMeNot
> (http://roachfiend.com/archives/2005/02/07/bugmenot/) to make firefox crash.

The patch was to 'extensions/xforms' only, so unless BugMeNot uses XForms (which
I doubt) I'm quite sure it's not related.