The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla18

Status

()

Core
HTML: Parser
P3
normal
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: hsivonen)

Tracking

({sec-want})

Trunk
mozilla18
sec-want
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:want P4])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

8 years ago
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 ;)
(Assignee)

Updated

7 years ago
Priority: -- → P3
(Reporter)

Updated

7 years ago
Whiteboard: [sg:want P4]
Keywords: sec-want
(Assignee)

Updated

5 years ago
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
Created attachment 665351 [details] [diff] [review]
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."
Attachment #665351 - Flags: review?(bugs)
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
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.
(Reporter)

Comment 4

5 years ago
> 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?
(Assignee)

Comment 5

5 years ago
Created attachment 665814 [details] [diff] [review]
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.)
Attachment #665351 - Attachment is obsolete: true
Attachment #665351 - Flags: review?(bugs)
Attachment #665814 - Flags: review?(bugs)
(Assignee)

Comment 6

5 years ago
Created attachment 665821 [details] [diff] [review]
Include assertions, address Jesse's comments, right patch version

Operating mq properly this time hopefully...
Attachment #665814 - Attachment is obsolete: true
Attachment #665814 - Flags: review?(bugs)
Attachment #665821 - Flags: review?(bugs)

Comment 7

5 years ago
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.
Attachment #665821 - Flags: review?(bugs) → review+
(Assignee)

Comment 8

5 years ago
(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
https://hg.mozilla.org/mozilla-central/rev/1caa3c482541
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(Reporter)

Comment 10

5 years ago
And the first victim is <drumroll>...</drumroll>

Bug 797009!
You need to log in before you can comment on or make changes to this bug.