272406 - External instance data not handled correctly

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Allan Beaufour]

nsXFormsInstanceElement receives two Load() events, and calls
nsXFormsModelElement::InstanceLoadFinished() on the first call, which is
premature, resulting in a too early nsXFormsModelElement::FinishConstruction().

Comment 1

[Allan Beaufour]

Attachment: Quick fix

Checks whether it's first load or not. It seems somewhat stupid to do it this
way, but is there something in aEvent we could use?

Comment 2

[Darin Fisher]

Why are there two load events?  Perhaps that indicates a gecko bug that we
should be concerned about?

Comment 3

[Darin Fisher]

I confirm that this patch works.  Again, though, I'm not sure why it is necessary.

Comment 4

[Darin Fisher]

OK, in my testing the problem is with the comment here:
http://lxr.mozilla.org/mozilla/source/extensions/xforms/nsXFormsInstanceElement.cpp#76

It seems that CreateInstanceDocument does not fail, and so we actually end up
loading the external instance document twice! :(

Comment 5

[Allan Beaufour]

(In reply to comment #4)
> OK, in my testing the problem is with the comment here:
>
http://lxr.mozilla.org/mozilla/source/extensions/xforms/nsXFormsInstanceElement.cpp#76
> 
> It seems that CreateInstanceDocument does not fail, and so we actually end up
> loading the external instance document twice! :(

Are you sure it loads both times? As far as I could see we only got the instance
data on the last load. But, admitted, I did not use too much time on it.

Comment 6

[Darin Fisher]

Attachment: v2 patch

This patch fixes the problem by ensuring that we ignore AttributeSet/Removed
notifications until after DoneAddingChildren.  The patch is fairly
straightforward.  I opted to use a boolean flag instead of attempting to modify
the notification mask on the XTF element wrapper because 1) we have no
backpointer to the XTF element wrapper, and 2) that would just be more code
anyways.

Comment 7

[Allan Beaufour]

(In reply to comment #6)
> Created an attachment (id=167608)
> v2 patch

Looks good. In fact, most of our XForms elements should have this. Do any of
them need to be informed about attribute changes before DoneAddingChildren is
called?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

What happens if you add xf:instance element dynamically and then
modify the src attribute? 
mIgnoreAttributeChanges is set true on Ctor, but never to false because
DoneAddingChildren() is called only during parsing.

Comment 9

[Darin Fisher]

> What happens if you add xf:instance element dynamically and then
> modify the src attribute? 
> mIgnoreAttributeChanges is set true on Ctor, but never to false because
> DoneAddingChildren() is called only during parsing.

yup, i think that would be a problem.  good catch.  then, i think it means that
we need your patch to move BeginAddingChildren up earlier before AttributeSet.

another option would be to allow the thing to load from AttributeSet, and then
remember that it had already started loading so we can then not bother loading
it again in DoneAddingChildren.  that might be better.  thoughts?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #9)
> 
> another option would be to allow the thing to load from AttributeSet, and then
> remember that it had already started loading so we can then not bother loading
> it again in DoneAddingChildren.  that might be better.  thoughts?

This sounds good.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v3

But actually I prefer BeginAddingChildren. This way it is clear that
the initialization of the instance document happens in DoneAddingChildren().

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 167930 [details] [diff] [review]
v3

patch v3 requires Bug 272814

Comment 13

[Darin Fisher]

Comment on attachment 167930 [details] [diff] [review]
v3

r=darin

Comment 14

[Darin Fisher]

fixed-on-trunk

bug will be fixed for real once the patch for bug 272814 lands.