503190 - [HTML5] Translation from Java to C++ loses assertions

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect, P3)

Description

[Jesse Ruderman]

The HTML5 parser has a few Java "assert" statements, but they don't make it over into the C++.  They should turn into NS_ASSERTION or NS_ABORT_IF_FALSE so they'll at least be found during fuzzing ;)

Comment 1

[Henri Sivonen (:hsivonen)]

Attachment: Include assertions

In most cases, the assertions on the Java side lack a human-readable message. In those cases, the translator generates the message "Assertion failed."

Comment 2

[Henri Sivonen (:hsivonen)]

The change in TreeBuilder.java is a Java-only fix for another bug. This bug just happens to be the next Gecko bug that shows the changed TreeBuilder.java.

Comment 3

[Henri Sivonen (:hsivonen)]

Oh and MOZ_ASSERT(clearLastStackSlot(), "Assertion failed."); is deliberate. The best approximation of getting debug-only side effects in Java is to call a method that has side effects in an assertion check. clearLastStackSlot() nulls out some memory that does not really need to be nulled to make it more obvious in a debugger what stack entries are valid and what entries are invalid.

Comment 4

[Jesse Ruderman]

> In most cases, the assertions on the Java side lack a human-readable message. In 
> those cases, the translator generates the message "Assertion failed."

MOZ_ASSERT can accept 1 or 2 parameters.  When you don't have an informative message, you might as well leave out the second parameter.

> Oh and MOZ_ASSERT(clearLastStackSlot(), "Assertion failed."); is deliberate.

Could these callers be changed to "debugOnlyClearLastStackSlot()", so the weirdness only happens in one place?

Comment 5

[Henri Sivonen (:hsivonen)]

Attachment: Include assertions, address Jesse's comments

(In reply to Jesse Ruderman from comment #4)
> > In most cases, the assertions on the Java side lack a human-readable message. In 
> > those cases, the translator generates the message "Assertion failed."
> 
> MOZ_ASSERT can accept 1 or 2 parameters.  When you don't have an informative
> message, you might as well leave out the second parameter.

Done.

> > Oh and MOZ_ASSERT(clearLastStackSlot(), "Assertion failed."); is deliberate.
> 
> Could these callers be changed to "debugOnlyClearLastStackSlot()", so the
> weirdness only happens in one place?

To rely on JVM cleverness minimally, I changed the method names to debugOnly… but kept the current call sites. (The JVM is more likely to be clever than not, but I don’t want to find out.)

Comment 6

[Henri Sivonen (:hsivonen)]

Attachment: Include assertions, address Jesse's comments, right patch version

Operating mq properly this time hopefully...

Comment 7

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

Comment on attachment 665821 [details] [diff] [review]
Include assertions, address Jesse's comments, right patch version


>-    public HtmlAttributes cloneAttributes(Interner interner) throws SAXException {
>-        assert (length == 0 && xmlnsLength == 0) || mode == 0 || mode == 3;
>+    public HtmlAttributes cloneAttributes(Interner interner)
>+            throws SAXException {
>+        assert (length == 0
>+        // [NOCPP[
>+                && xmlnsLength == 0
>+                // ]NOCPP]
The alignment of [NOCPP[ is odd.
I'd put it below length.
Also, add a comment why // [NOCPP[ // ]NOCPP]


>         for (int i = 0; i < length; i++) {
>-            clone.addAttribute(names[i].cloneAttributeName(interner), Portability.newStringFromString(values[i])
>-            // [NOCPP[
>-                   , XmlViolationPolicy.ALLOW
>+            clone.addAttribute(names[i].cloneAttributeName(interner),
>+                    Portability.newStringFromString(values[i])
>+                    // [NOCPP[
>+                    , XmlViolationPolicy.ALLOW
>             // ]NOCPP]
>             );
indentation is odd also here.


>-                        assert (index > 0);
>+                        assert index > 0;
I'd prefer assert(expr), but don't really care.

Comment 8

[Henri Sivonen (:hsivonen)]

(In reply to Olli Pettay [:smaug] from comment #7)

> The alignment of [NOCPP[ is odd.

Fixed, though this was the output of Eclipse’s formatter and generally it’s not worth the trouble to fight the formatter.

> Also, add a comment why // [NOCPP[ // ]NOCPP]

Sorry, I missed this before landing. The xmlnsLength field itself doesn’t exist in C++ and the other NOCPP sections about it don’t have comments either, so perhaps not worth changing.

Thanks. Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/1caa3c482541

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1caa3c482541

Comment 10

[Jesse Ruderman]

And the first victim is <drumroll>...</drumroll>

Bug 797009!