732842 - Add assertions for memory reports

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Nicholas Nethercote [inactive]]

We should detect illegal combinations of values (kind, units, etc) in memory reports within aboutMemory.js.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: patch

The much-anticipated patch :)  If you're wondering why I changed the
other6-danger<script>window.alert(1)</script> reports in the tests, it's for 
two reasons:

- The '/' in the name was triggering an assertion, because we don't allow 
  slashes in "other" reports.
  
- Now that aboutMemory.js doesn't use innerHTML to create its DOM, the 
  security aspect isn't relevant any more.

Comment 2

[Justin Lebar (not reading bugmail)]

\o/

>+  function handleException(aKind, aE)
>+  {
>+    // There are two exception cases that must be distinguished here.
>+    //
>+    // - We want to halt proceedings on exceptions thrown within this file
>+    //   (i.e. assertion failures in handleReport);  such exceptions contain
>+    //   gAssertionFailureMsgPrefix in their string representation.
>+    //
>+    // - We want to continue on when faced with exceptions thrown outside this
>+    //   file (i.e. in collectReports).
>+
>+    let str = aE.toString();
>+    if (str.search(gAssertionFailureMsgPrefix) >= 0) {
>+      throw(aE); 
>+    } else {
>+      debug("Bad memory " + aKind + " '" + name + "': " + aE);
>+    }
>+  }

So this error message will look like
  
   Bad memory 2 'heap-allocated': bad amount

?  I guess that's OK, but I'm not a fan of "Bad memory".  Perhaps "Error" or
"Bad memory reporter"?

>+   *   Reporters in this category must have kind HEAP or NONHEAP, units BYTES,
>+   *   and a description this is a sentence (i.e. starts with a capital letter
>+   *   and ends with a period, or similar).

a description /that/ is a sentence.

>+   *   contain '/' separators, kind OTHER, and a description this is a
>+   *   sentence.

/that/

Comment 3

[Nicholas Nethercote [inactive]]

> >+      debug("Bad memory " + aKind + " '" + name + "': " + aE);
> 
> So this error message will look like
>   
>    Bad memory 2 'heap-allocated': bad amount
> 
> ?  I guess that's OK, but I'm not a fan of "Bad memory".  Perhaps "Error" or
> "Bad memory reporter"?

If you look at the calling context, |aKind| is either "reporter" or "multi-reporter".  Overloading "kind" like this is confusing... I'll change the variable name to something else.

Comment 4

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d90b5920d076

Comment 5

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/d90b5920d076