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

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Jesse Ruderman]

Attachment: testcase

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

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase showing resulting bug

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

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.

Comment 6

[Jesse Ruderman]

With the patch, does the updating still happen soon enough if the web page script submits the form?

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/7c2f14994a8c

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 397872 [details] [diff] [review]
Proposed fix

I think it's worth fixing this correctness bug on 1.9.2 as well.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 397872 [details] [diff] [review]
Proposed fix

Given the regressions, it's clearly too early to take this on branches.