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

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Jesse Ruderman, Assigned: mounir)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla2.0b7
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Reporter)

Description

8 years ago
Created attachment 474490 [details]
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
(Reporter)

Comment 1

8 years ago
Created attachment 474491 [details]
stack traces
(Reporter)

Comment 2

8 years ago
Created attachment 474492 [details]
stack traces
Attachment #474491 - Attachment is obsolete: true
(Assignee)

Comment 3

8 years ago
Given that is an assert, I guess it should be a blocker?
blocking2.0: --- → ?
OS: Mac OS X → All
Hardware: x86 → All
At least figuring out why it's happening, yes.
Yup, blocking on at least understanding this assertion.
Assignee: nobody → mounir.lamouri
blocking2.0: ? → betaN+
(Assignee)

Comment 6

8 years ago
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.
Status: NEW → ASSIGNED
(Assignee)

Comment 7

8 years ago
Created attachment 476929 [details] [diff] [review]
Patch v1
Attachment #476929 - Flags: review?(jonas)
(Assignee)

Comment 8

8 years ago
Hmm, maybe I should write a reftest/crashtest checking that there is no assert while loading this page.
(Assignee)

Comment 9

8 years ago
Created attachment 476936 [details] [diff] [review]
Patch #1 v1.1

With a crashtest. It will fail if there is an assert.
Attachment #476929 - Attachment is obsolete: true
Attachment #476936 - Flags: review?(jonas)
Attachment #476929 - Flags: review?(jonas)
(Assignee)

Comment 10

8 years ago
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 on attachment 476936 [details] [diff] [review]
Patch #1 v1.1

Stealing review, r=jst
Attachment #476936 - Flags: review?(jonas) → review+
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>
(Assignee)

Comment 14

8 years ago
Hmm, sounds like only half of the problem is fixed with this patch.
(Assignee)

Comment 15

8 years ago
Created attachment 477025 [details]
Testcase
Attachment #474490 - Attachment is obsolete: true
Attachment #474492 - Attachment is obsolete: true
(Assignee)

Comment 16

8 years ago
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?
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.
Yeah, reversing unbind order of kids scares me....
(Assignee)

Comment 19

8 years ago
Created attachment 477225 [details] [diff] [review]
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.
Attachment #476936 - Attachment is obsolete: true
Attachment #477225 - Flags: review?(jst)
(Assignee)

Updated

8 years ago
Attachment #476936 - Attachment description: Patch v1.1 → Patch #1 v1.1
Attachment #476936 - Attachment is obsolete: false
I'd really prefer to find another solution, even if this does pass tryserver
(Assignee)

Comment 21

8 years ago
(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.
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.
(Assignee)

Comment 23

8 years ago
(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.
(Assignee)

Comment 24

8 years ago
Created attachment 477327 [details] [diff] [review]
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.
Attachment #476936 - Attachment is obsolete: true
Attachment #477225 - Attachment is obsolete: true
Attachment #477327 - Flags: review?(jst)
Attachment #477225 - Flags: review?(jst)
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.
(Assignee)

Comment 26

8 years ago
Created attachment 477364 [details] [diff] [review]
Part 2 - Remove asserts with ugly hacks

Thanks Jonas for this patch :)
Attachment #477364 - Flags: review?(jst)
(Assignee)

Updated

8 years ago
Attachment #477327 - Attachment description: Patch v2 → Part 1 - Remove two asserts + tests

Updated

8 years ago
Attachment #477327 - Flags: review?(jst) → review+
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?
(Assignee)

Comment 28

8 years ago
(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).
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 on attachment 477364 [details] [diff] [review]
Part 2 - Remove asserts with ugly hacks

Ok then :).
Attachment #477364 - Flags: review?(jst) → review+
(Assignee)

Comment 31

8 years ago
Pushed:
http://hg.mozilla.org/mozilla-central/rev/05adb7427328
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in before you can comment on or make changes to this bug.