Closed Bug 513153 Opened 10 years ago Closed 10 years ago

[FIX]"ASSERTION: no common ancestor at all???" with several <input type="submit">s

Categories

(Core :: Layout: Form Controls, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(4 files)

Attached file testcase
###!!! ASSERTION: no common ancestor at all???: 'parent', file /Users/jruderman/central/layout/base/nsLayoutUtils.cpp, line 413
Attached file stack trace
Hmm.  That's happening because neither of the submit inputs has the form on its parent chain anymore by the time we're trying to remove the first one from the form.  And then we try to compare the second one to the image to see which should be the default input.

I need to think a bit to see whether this can get us into a situation where we end up with the wrong input being the default one.
OK, so when the assert fires we end up with 0 returned from CompareFormControlPosition.  Which means we set mDefaultSubmitElement to mFirstSubmitNotInElements.

Now if the assertion is firing, that means that either mFirstSubmitNotInElements or mFirstSubmitInElements or both are about to get passed to RemoveElement.  If mFirstSubmitNotInElements gets passed in, we will at that point recompute the correct mDefaultSubmitElement.  So that case is ok.  If mFirstSubmitInElements gets passed in, we will do nothing.  So the one possible failure case is when mFirstSubmitInElements in fact comes before mFirstSubmitNotInElements but there's another in-elements submit that comes before the not-in-elements submit.
Attached patch Proposed fixSplinter Review
This puts the updating of the default input off on a script runner so we make sure to do it after the removal is all done.  As a side benefit, we have fewer compares to do now if lots of submits in a row are removed.
Attachment #397872 - Flags: review?(jst)
Assignee: nobody → bzbarsky
Summary: "ASSERTION: no common ancestor at all???" with several <input type="submit">s → [FIX]"ASSERTION: no common ancestor at all???" with several <input type="submit">s
With the patch, does the updating still happen soon enough if the web page script submits the form?
It happens before control returns to JS.  But in any case, script submitting the form doesn't depend on the default submit; the only things that do are :default styling, enter behavior, and some accessibility stuff.
Attachment #397872 - Flags: review?(jst) → review+
Pushed http://hg.mozilla.org/mozilla-central/rev/7c2f14994a8c
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Comment on attachment 397872 [details] [diff] [review]
Proposed fix

I think it's worth fixing this correctness bug on 1.9.2 as well.
Attachment #397872 - Flags: approval1.9.2?
Depends on: 515703
Depends on: 515829
Attachment #397872 - Flags: approval1.9.2?
Comment on attachment 397872 [details] [diff] [review]
Proposed fix

Given the regressions, it's clearly too early to take this on branches.
You need to log in before you can comment on or make changes to this bug.