698928 - Assert that about:memory numbers are nonnegative (and <= 100%)

Status: RESOLVED FIXED

(Toolkit :: about:memory, enhancement)

Description

[Jesse Ruderman]

My fuzzer opens about:memory occasionally. This has already caught a few bugs (bug 678820 and bug 682646), but might find more if about:memory reported obvious bogosity as assertion failures.

(How do you assert from JS in a way that the fuzzer can detect the bug?)

Comment 1

[Justin Lebar (not reading bugmail)]

> (How do you assert from JS in a way that the fuzzer can detect the bug?)

You tell us.  :)

Can we put an element with a magic ID which you look for in the document?

Comment 2

[Justin Lebar (not reading bugmail)]

We could do something like

  <span class='bad-value'>-400MB</span>  heap-unclassified

and then visually highlight the bad value, too.

Comment 3

[Nicholas Nethercote [inactive]]

aboutMemory.js already has this:

 function assert(aCond, aMsg)
 {
   if (!aCond) {
     throw("assertion failed: " + aMsg);
   }
 }

It's used for checking that reporters are valid (certain combinations of fields are illegal) and also some sanity checking.

We probably shouldn't use this mechanism -- it prevents the page being rendered, and in negative cases we want to see the page.  But I thought it worth mentioning.

Comment 4

[Jesse Ruderman]

You could dump() a short, unique message.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsIDebug.idl#53

Comment 6

[Nicholas Nethercote [inactive]]

Jesse, I assume browser.dom.window.dump.enabled is on in your fuzzers?  Any preferences for what the dumped message should look like?

Comment 7

[Nicholas Nethercote [inactive]]

Now that I think about it, my not-quite-right patch in bug 698930 prints a message that's visible on the about:memory page.  Maybe that's enough, or is the dump() message needed too?

Comment 8

[Jesse Ruderman]

For fuzzing and other automated testing, a dump() or nsIDebug.assertion() is easier to check than something visual. As long as the message always starts the same way, I won't have any trouble detecting it.

Comment 9

[Nicholas Nethercote [inactive]]

Jesse, I think it's easier at this point if you take this bug.  That'll let you fiddle with the exact assertion mechanism and the message.  I'm happy to review.

Probably the best place to trigger the assertion is within appendWarningElements() in aboutMemory.js -- look for the string "WARNING: the following values are negative or unreasonably large."

Comment 10

[Jesse Ruderman]

Attachment: patch

* Add messages for the assert() calls that were missing messages
* Make assert() call nsIDebug.assertion

* Change the "Invalid value" test to not rely on division
* Make the "Invalid value" test call nsIDebug.assertion (but not through assert(), because assert() throws)


With the patch, a debug build hitting an assertion prints messages like this:

###!!! ASSERTION: reporters in the tree must have KIND_HEAP or KIND_NONHEAP: 'false', file aboutMemory.js, line 0

JavaScript error: , line 0: uncaught exception: aboutMemory.js assertion failed: reporters in the tree must have KIND_HEAP or KIND_NONHEAP


https://tbpl.mozilla.org/?tree=Try&rev=d28482f6dd85

Comment 11

[Nicholas Nethercote [inactive]]

Comment on attachment 598761 [details] [diff] [review]
patch

Review of attachment 598761 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!  Thanks for adding the missing messages.

Bug 702300 is almost finished and it has a 9-patch stack that moves about tons of code in aboutMemory.js.  This patch will probably cause horrible conflicts if it lands first.  I'm happy to un-bitrot this one and land it for you when bug 702300 is done.  Sound ok?

Comment 12

[Jesse Ruderman]

> I'm happy to un-bitrot this one and land it for you when bug 702300 is done.

Sounds good, thanks!

Comment 13

[Jesse Ruderman]

Attachment: patch v2

Comment 14

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/49140e63962f

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/49140e63962f