598468 - Remove ugly code preventing assert in DoCompareTreePosition when called from nsHTMLFormElement.cpp

Status: NEW

(Core :: DOM: Forms, task, P5)

Description

[Mounir Lamouri (:mounir)]

Somewhat like: nsLayoutUtils::CompareTreePosition.
This should be used in nsHTMLFormElement.cpp, CompareFormControlPosition. Most form controls have the form element has a common ancestor so it would be nice to take this in consideration.

Comment 1

[Mounir Lamouri (:mounir)]

Jonas wants to use a global var to prevent the assert so we are going to keep calling the same method with a fix to prevent the asserts...

Comment 2

[Sylvestre Ledru [:Sylvestre]]

hsinyi, should we just remove the dead code in https://searchfox.org/mozilla-central/source/dom/html/HTMLFormElement.cpp#1108 ?
(grep for 598468)
seems that nothing will ever happen here.

Comment 3

[John Dai[:jdai]]

This bug was introduced by bug 595606 comment 6. Per spec, it should make sure elements in tree order in HTMLFormControlsCollection[1] and Constructing the entry list[2]. I think it's still valid and we should fix this bug. It's safe to remove the code in https://searchfox.org/mozilla-central/source/dom/html/HTMLFormElement.cpp#1108 because we can make sure elements in tree order by tests. However, I'll leave it for DOM Peer to do the final call.

[1] https://html.spec.whatwg.org/#the-htmlformcontrolscollection-interface
[2] https://html.spec.whatwg.org/#constructing-form-data-set

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

This looks valid bug to me. And there is no dead code (in debug builds).

Comment 5

[Chris Peterson [:cpeterson]]

These -Wunreachable-code warnings in HTMLFormElement::AssertDocumentOrder prevent building a debug build in non-unified mode (ac_add_options --disable-unified-build):

dom/html/HTMLFormElement.cpp:1140:8 [-Wunreachable-code] code will never be executed
dom/html/HTMLFormElement.cpp:1166:8 [-Wunreachable-code] code will never be executed

https://searchfox.org/mozilla-central/rev/50b131865834c5c1a3090fe739b4d129a2f7fd5e/dom/html/HTMLFormElement.cpp#1136-1138,1162-1164

Comment 6

[Marco Castelluccio [:marco]]

(In reply to Chris Peterson [:cpeterson] from comment #5)

These -Wunreachable-code warnings in HTMLFormElement::AssertDocumentOrder prevent building a debug build in non-unified mode (ac_add_options --disable-unified-build):

dom/html/HTMLFormElement.cpp:1140:8 [-Wunreachable-code] code will never be executed
dom/html/HTMLFormElement.cpp:1166:8 [-Wunreachable-code] code will never be executed

https://searchfox.org/mozilla-central/rev/50b131865834c5c1a3090fe739b4d129a2f7fd5e/dom/html/HTMLFormElement.cpp#1136-1138,1162-1164

Is this still a problem?

Comment 7

[[:sergesanspaille]]

https://phabricator.services.mozilla.com/D139947 commented out that code portion. On a local build configured with

ac_add_options --disable-unified-build
ac_add_options --enable-debug

and the following patch applied

diff --git a/dom/html/HTMLFormElement.cpp b/dom/html/HTMLFormElement.cpp
index ce19a6cf77403..5b05fabcd9ec6 100644
--- a/dom/html/HTMLFormElement.cpp
+++ b/dom/html/HTMLFormElement.cpp
@@ -1133,7 +1133,6 @@ void HTMLFormElement::AssertDocumentOrder(
     const nsTArray<nsGenericHTMLFormElement*>& aControls, nsIContent* aForm) {
   // TODO: remove the if directive with bug 598468.
   // This is done to prevent asserts in some edge cases.
-#  if 0
   // Only iterate if aControls is not empty, since otherwise
   // |aControls.Length() - 1| will be a very large unsigned number... not what
   // we want here.
@@ -1144,7 +1143,6 @@ void HTMLFormElement::AssertDocumentOrder(
           "Form controls not ordered correctly");
     }
   }
-#  endif
 }
 
 /**
@@ -1159,7 +1157,6 @@ void HTMLFormElement::AssertDocumentOrder(
     nsIContent* aForm) {
   // TODO: remove the if directive with bug 598468.
   // This is done to prevent asserts in some edge cases.
-#  if 0
   // Only iterate if aControls is not empty, since otherwise
   // |aControls.Length() - 1| will be a very large unsigned number... not what
   // we want here.
@@ -1170,7 +1167,6 @@ void HTMLFormElement::AssertDocumentOrder(
           "Form controls not ordered correctly");
     }
   }
-#  endif
 }
 #endif

I don't get any warning, but it feels like I'm missing something.

Comment 8

[Jan Varga [:janv]]

(In reply to [:sergesanspaille] from comment #7)

https://phabricator.services.mozilla.com/D139947 commented out that code portion.

That patch just converted an early return to a commented out code. I'm not sure why we would want to make the code live.
You probably want to ask someone who knows this code better.