As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 503190 - [HTML5] Translation from Java to C++ loses assertions
: [HTML5] Translation from Java to C++ loses assertions
Status: RESOLVED FIXED
[sg:want P4]
: sec-want
Product: Core
Classification: Components
Component: HTML: Parser (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: mozilla18
Assigned To: Henri Sivonen (:hsivonen)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-07-08 15:07 PDT by Jesse Ruderman
Modified: 2012-10-02 10:40 PDT (History)
4 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Include assertions (33.15 KB, patch)
2012-09-27 04:25 PDT, Henri Sivonen (:hsivonen)
no flags Details | Diff | Splinter Review
Include assertions, address Jesse's comments (33.15 KB, patch)
2012-09-28 00:25 PDT, Henri Sivonen (:hsivonen)
no flags Details | Diff | Splinter Review
Include assertions, address Jesse's comments, right patch version (38.62 KB, patch)
2012-09-28 00:54 PDT, Henri Sivonen (:hsivonen)
bugs: review+
Details | Diff | Splinter Review

Description User image Jesse Ruderman 2009-07-08 15:07:27 PDT
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 User image Henri Sivonen (:hsivonen) 2012-09-27 04:25:00 PDT
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."
Comment 2 User image Henri Sivonen (:hsivonen) 2012-09-27 04:26:21 PDT
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 User image Henri Sivonen (:hsivonen) 2012-09-27 04:30:05 PDT
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 User image Jesse Ruderman 2012-09-27 13:43:21 PDT
> 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 User image Henri Sivonen (:hsivonen) 2012-09-28 00:25:40 PDT
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.)
Comment 6 User image Henri Sivonen (:hsivonen) 2012-09-28 00:54:49 PDT
Created attachment 665821 [details] [diff] [review]
Include assertions, address Jesse's comments, right patch version

Operating mq properly this time hopefully...
Comment 7 User image Olli Pettay [:smaug] (review queue closed until backlog cleared) 2012-09-30 14:09:04 PDT
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 User image Henri Sivonen (:hsivonen) 2012-10-01 01:58:58 PDT
(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 User image Ryan VanderMeulen [:RyanVM] 2012-10-01 19:03:06 PDT
https://hg.mozilla.org/mozilla-central/rev/1caa3c482541
Comment 10 User image Jesse Ruderman 2012-10-02 10:40:09 PDT
And the first victim is <drumroll>...</drumroll>

Bug 797009!

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