595606 - "ASSERTION: no common ancestor at all" and more with @form, duplicate ids

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: no common ancestor at all???: 'parent', 
file layout/base/nsLayoutUtils.cpp, line 525

###!!! ASSERTION: Form controls not ordered correctly: 'CompareFormControlPosition(aControls[i], aControls[i + 1], aForm) < 0', 
file content/html/content/src/nsHTMLFormElement.cpp, line 1069

Comment 1

[Jesse Ruderman]

Attachment: stack traces

Comment 2

[Jesse Ruderman]

Attachment: stack traces

Comment 3

[Mounir Lamouri (:mounir)]

Given that is an assert, I guess it should be a blocker?

Comment 4

[Boris Zbarsky [:bzbarsky]]

At least figuring out why it's happening, yes.

Comment 5

[Johnny Stenback (:jst)]

Yup, blocking on at least understanding this assertion.

Comment 6

[Mounir Lamouri (:mounir)]

The element's id handling in document isn't working very fine.
If we have this situation:
<div>
  <div id='a'>
    <form id='a'></form>
  </div>
</div>

If the first div is removed, UnbindFromTree will be called for the frist div then the second and finally the form. Each time UnbindFromTree is called, the element will inform the document that it's id is no longer valid. So, during a small laps of time, the document will think that GetElementById('a') should return the form element which is wrong considering that the form element is no longer in the tree when the first div is removed.

It happens to assert in form code because there is FormIdUpdated callback which is a callback called by the document when the content related to an id changes. In parameter is passed the new element pointed by the id. Here, the document will inform elemnets with form='a' that the new element pointed by 'a' is now the form. But the form is no longer in the document so there is no common ancestor between them.

There is a simple fix: remove the id from the document table _after_ UnbindFromTree call so this will be done for inner-most element first. So, in that case, when <div id='a'> will tell the document that it's id is no longer valid, the document will just tell the elements looking to the id 'a' that no elements now have this id in the document.

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 8

[Mounir Lamouri (:mounir)]

Hmm, maybe I should write a reftest/crashtest checking that there is no assert while loading this page.

Comment 9

[Mounir Lamouri (:mounir)]

Attachment: Patch #1 v1.1

With a crashtest. It will fail if there is an assert.

Comment 10

[Mounir Lamouri (:mounir)]

I got a full green run on Linux-opt and mostly full green on OS X-opt (m-oth is pending). So no orange on try server so far. Worth to be reviewed Jonas? :)

(I will add a comment here if something goes wrong on the try server)

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 476936 [details] [diff] [review]
Patch #1 v1.1

Stealing review, r=jst

Comment 12

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/fe353b7ac4b7

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Can't you get the same assertion by changing the subtree to look like:

  <div id="x">
    <form id="a">
      <select></select>
    </form>
    <form id="a">
      <select></select>
    </form>  </div>

Comment 14

[Mounir Lamouri (:mounir)]

Hmm, sounds like only half of the problem is fixed with this patch.

Comment 15

[Mounir Lamouri (:mounir)]

Attachment: Testcase

Comment 16

[Mounir Lamouri (:mounir)]

A naive fix would be to Unbind children from the last to the first. That seems to be a sensitive change, though.
On top of your heads, is there a reason why such a change can't be done?

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

None that I can think of off the top of my head, but that doesn't make me a whole lot less worried.

I wouldn't be surprised if XBL unbinding order matters for example.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Yeah, reversing unbind order of kids scares me....

Comment 19

[Mounir Lamouri (:mounir)]

Attachment: Patch #2 v1

The try server is happy with this patch but I don't know how much XUL/XBL stuff is tested so I can't say if that's safe. I will let you choose.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'd really prefer to find another solution, even if this does pass tryserver

Comment 21

[Mounir Lamouri (:mounir)]

(In reply to comment #20)
> I'd really prefer to find another solution, even if this does pass tryserver

Ignore the assert? This assert is probably harmless. The only consequence is during a small laps of time, the element will have a form owner not in the document.

Otherwise, when updating the form owner, the element can try to look at the parent chain and when we are in this situation, it should be broken at some point. However, this sounds to be a quite dirty fix.

I would vote for ignoring this assert for Gecko 2.0 and try to invert the unbind order for the next version if you think that could be technically done.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

If you use the nsContentUtils::PositionIsBefore rather than the layout utils function to compare position, then the assert should go away.

Might be worth reverting the other change then too.

After FF4 branches, we should consider inverting the unbindfromtree order.

Comment 23

[Mounir Lamouri (:mounir)]

(In reply to comment #22)
> If you use the nsContentUtils::PositionIsBefore rather than the layout utils
> function to compare position, then the assert should go away.

That would work but that would be less efficient considering the current method use the fact that most form controls have a common ancestor (the form element). This new method will walk the entire parent chain everytime.

This said, we can also do that and open a follow-up to optimize it if a possible common ancestor is given.

Comment 24

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - Remove two asserts + tests

Assigning the review to jst but Jonas, feel free to take it.

This patch revert the first patch (but keeps the test). This is adding the new test case and changes the method. The new method does not assert. However, a method is run on debug checking that the form controls are correctly ordered. Considering the value returned by PositionIsBefore will be meaningless if nodes have no common ancestor, this method is asserting.

I've open bug 598471 and bug 598472 for the real fixes of this bug.
And bug 598468 to have an optimized method using the form element.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We could just as jesse to ignore the assertion for now, but that's not a great solution. Another solution is to silence the assertion by for example setting a global debug-only variable during the calls that we know can assert.

Comment 26

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Remove asserts with ugly hacks

Thanks Jonas for this patch :)

Comment 27

[Johnny Stenback (:jst)]

Comment on attachment 477364 [details] [diff] [review]
Part 2 - Remove asserts with ugly hacks

If this is a harmless assertion, do we really care to get rid of it with this hack if we'll be fixing it correctly later on in bug 598468?

Comment 28

[Mounir Lamouri (:mounir)]

(In reply to comment #27)
> Comment on attachment 477364 [details] [diff] [review]
> Part 2 - Remove asserts with ugly hacks
> 
> If this is a harmless assertion, do we really care to get rid of it with this
> hack if we'll be fixing it correctly later on in bug 598468?

IIRC, Jonas asked for this second patch because the first one alone might decrease performance (PositionIsBefore might be slower).

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, I think this is the approach we should take. Generally if this assert fires we have an actual bug in the code, so we don't want to remove the assert entirely.

We also don't want to use nsContentUtils::PositionIsBefore as that is slower in the common case.

Comment 30

[Johnny Stenback (:jst)]

Comment on attachment 477364 [details] [diff] [review]
Part 2 - Remove asserts with ugly hacks

Ok then :).

Comment 31

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/05adb7427328