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
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+
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
Created attachment 476929 [details] [diff] [review] Patch v1
Hmm, maybe I should write a reftest/crashtest checking that there is no assert while loading this page.
Created attachment 476936 [details] [diff] [review] Patch #1 v1.1 With a crashtest. It will fail if there is an assert.
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>
Hmm, sounds like only half of the problem is fixed with this patch.
Created attachment 477025 [details] Testcase
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....
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.
I'd really prefer to find another solution, even if this does pass tryserver
(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.
(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.
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.
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.
Created attachment 477364 [details] [diff] [review] Part 2 - Remove asserts with ugly hacks Thanks Jonas for this patch :)
Attachment #477364 - Flags: review?(jst)
Attachment #477327 - Attachment description: Patch v2 → Part 1 - Remove two asserts + tests
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?
(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+
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
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.