Closed Bug 372323 Opened 17 years ago Closed 17 years ago

[FIX]regression: output with @value shows no label

Categories

(Core :: XML, defect, P4)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronr, Assigned: bzbarsky)

References

Details

(Keywords: regression)

Attachments

(5 files)

Attached file testcase
This is a weird one.  A regression sometime since 0.7.0.1 where if an output uses @value instead of @ref, its label won't show up.
Blocks: 353738
I tracked down the problem by grabbing nightlies until I found one that didn't have the problem.  So the problem was introduced between the nightly for 2007-02-01 and 2007-02-02.  Note there were no XForms changes in that time frame, so this isn't directly caused by a XForms checkin.

Then I checked out the trunk with MOZ_CO_DATE="2007-02-01 03:00PDT" and began applying each checkin, in order, one at a time.  This helped me narrow down the patch that is causing this grief.  This problem occurs once I apply the checkin that bz did for bug 369011.

bz, what we are seeing in XForms is that even though this attached testcase has a xf:label in the document, and upon load there is a xf:label the DOM (I can use JS to navigate the DOM and find the xf:label element), we don't see the XBL binding being applied (xbl constructor never hit) and the xf:label doesn't appear in the DOM Inspector.  But if I apply enough breakpoints via the debugger, then the label will appear.  Also, if I use a xf:label as a standalone element it will have no problem.  The behavior we are seeing is pretty odd to say the least.

When things are working, I see this warning in the console: WARNING: Textframe maps no content: file c:/ns50crap/mozilla/layout/generic/nsTextFrame.cpp, line 4967.  When the label fails to appear, that warning doesn't occur.  I have no idea what that means, just an observation :-)

bz, can you help us figure out what is going on?
So it sounds like your real problem is something to do with incremental XML parsing, no?  Bug 369011 just made sure to initialize members; it made no logic changes.

Given your symptoms, this sounds like an XBL bug, but I'm not really quite sure where to start debugging.  First of all, is that a minimal testcase?  That is, are both labels needed?
Also, how exactly do I tell when this is failing?  If I load that testcase in a current trunk Firefox with the XForms 0.7 extension, I see the text:

   444444444 555555555

What should I be seeing?
(In reply to comment #3)
> Also, how exactly do I tell when this is failing?  If I load that testcase in a
> current trunk Firefox with the XForms 0.7 extension, I see the text:
> 
>    444444444 555555555
> 
> What should I be seeing?
> 

As the testcase stands now, you should see "Calculate Value Should Be 444444444: 444444444 Calculate Value Should Be 555555555: 555555555"

I'll attach a minimal testcase.  Basically the same testcase with a smaller instance document (which isn't used anyhow since there is no binding happening in this testcase) and only one xf:output.
Attached file minimized testcase
So at a guess, here's what happens (based on 15 minutes of debugging):

1)  The sink opens the <output> element and binds it to the tree.
2)  nsXFormsControlStub::ParentChanged is called.
3)  It calls (indirectly) nsXFormsControlStub::BindToModel
4)  This calls (indirectly) nsXFormsUtils::FindParentContext
5)  This calls nsContentList::Item.
6)  This calls nsDocument::FlushPendingNotifications, which flushes out the
    notification for the just-added node.
7)  We never notify on the <xforms:label> or XBL screws up the notification.
Flags: blocking1.9?
So I bet the issue is that when we flush out from under BindToTree, we end up setting mNotifyLevel to the level _above_ the new thing we added, because the BindToTree call happens before we put the new node on the stack.  Then we push the node on the stack, and that means we won't notify on its kids.

We could either fix the sink to change the notification level in a better way, somehow, or we could declare that flushing out the sink from inside BindToTree is bad and that callers should instead wait for DoneCreatingElement().

The HTML sink does the former, if you note: it first pushes the new thing on the stack, then makes the AppendChildTo() calls.  So I think we should fix this in the sink.
Assignee: aaronr → xml
Status: ASSIGNED → NEW
Component: XForms → XML
Keywords: regression
OS: Windows XP → All
QA Contact: spride → ashshbhatt
Hardware: PC → All
In fact, HTML pushes the new node even before adding attributes (which makes sense if attribute sets do something that flushes the sink!)
Sounds like that's what we should do. Declaring that you shouldn't flush from BindToTree will probably never work.
OK, I tried fixing the notifications.

Then we get into nsCSSFrameConstructor::ContentAppended, hit a multiple-insertion-point situation, start going through the kids.  For the <label> kid we fetch the insertion point, init the content iterator... and don't find the <label> under it!

So this sounds like it might be an XBL bug also?  Not sure.
Blocks: 376307
No longer blocks: 353738
Comment on attachment 258316 [details] [diff] [review]
Patch to fix notifications

Looks good to me. Ship it! :)
Attachment #258316 - Flags: superreview+
Attachment #258316 - Flags: review+
Flags: blocking1.9? → blocking1.9+
Checked that patch in.  Note that that does not fix the bug -- see comment 10.
Flags: in-testsuite?
Blocks: 380896
No longer blocks: 380896
Assignee: xml → nobody
QA Contact: ashshbhatt → xml
Aaron, are you still seeing this?
Assignee: nobody → jonas
Priority: -- → P4
(In reply to comment #16)
> Aaron, are you still seeing this?
> 

yep, still occurs on a build that I pulled and built this afternoon.
Aaron, could you have a look here? I'm most likely not going to have time to work on this.

One thing that would probably be useful to figure out is what patch caused this regression.
Assignee: jonas → aaronr
(In reply to comment #18)
> Aaron, could you have a look here? I'm most likely not going to have time to
> work on this.
> 
> One thing that would probably be useful to figure out is what patch caused this
> regression.
> 

Hey Jonas, in comment #1 I mentioned that the patch that bz did for bug 369011 is what started the misbehavior.  I have no idea if that just unmasked another hidden problem or if it was actually the cause (bz voted for the former).

I don't know the XBL code.  I have no idea where to start.  If you guys can give me an idea of what might be wrong and what the right behavior is, I can probably debug to see if what you think is happening is what is actually happening.  I don't have time to learn the code on my own but if someone is willing to help walk me through it, I'll take a stab at it.
Attached patch FixSplinter Review
I happened to take a look at the document in Inspector, compared it to the binding document, and the problem became clear: the label was stuck in the wrong insertion point (the <children> and not the <children includes="label">).  But CSSFC looked for it in the correct insertion point, hence didn't find it.

The patch just makes ContentAppended perform the equivalent of ContentInserted on every one of the new kids if we have multiple insertion points.  That way the kids all end up in the right places.  I needed the helper method because we don't want to actually call ContentInserted, since it sends the notification on.  That large chunk of new code is basically moved from ContentInserted, except I made |ins| a weak pointer.  I think that's safe.

Non-xforms testcase that exercises this code coming up.
Assignee: aaronr → bzbarsky
Status: NEW → ASSIGNED
Attachment #287803 - Flags: superreview?(jonas)
Attachment #287803 - Flags: review?(jonas)
Attached patch ReftestSplinter Review
I wish we still did batch-appends of document fragments... this testcase could be a lot simpler then.
Attachment #287806 - Flags: review?(jonas)
I tried the patch and it fixes our problem!  Thanks a ton, bz, you are a good man!
Comment on attachment 287803 [details] [diff] [review]
Fix

Drive-by comments...

+  // aIndexInContainer is the index of the child in the parent.  It must be
+  // nonnegative.

Then why isn't it an unsigned argument?

- In nsBindingManager::HandleChildInsertion():

+  NS_PRECONDITION(aIndexInContainer != -1, "Bogus index");

Shouldn't that be aIndexInContainer >= 0 if aIndexInContainer remains signed?
> Then why isn't it an unsigned argument?

Hmm.  I could do that, sure.  Callers have a PRInt32, but they know it's not negative.  I'll make this unsigned.

> Shouldn't that be aIndexInContainer >= 0 if aIndexInContainer remains signed?

Could do, though we _really_ shouldn't have any negative indices less than -1... and I do assert it equals a return value of IndexOf()
Summary: regression: output with @value shows no label → [FIX]regression: output with @value shows no label
Comment on attachment 287803 [details] [diff] [review]
Fix

r/sr=me with jsts comment.
Attachment #287803 - Flags: superreview?(jonas)
Attachment #287803 - Flags: superreview+
Attachment #287803 - Flags: review?(jonas)
Attachment #287803 - Flags: review+
Comment on attachment 287806 [details] [diff] [review]
Reftest

Please put in a comment what documents those data-url decodes into so that it's possible to understand the testcase by looking at it.
Attachment #287806 - Flags: review?(jonas) → review+
> Please put in a comment what documents those data-url decodes into

You mean the "The data: URIs are this binding" comment I wrote?  ;)

I suppose I could move it closer to the <div>s....
Fixed with jst's comments and the comment in the test moved.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Depends on: 403962
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: