Last Comment Bug 698928 - Assert that about:memory numbers are nonnegative (and <= 100%)
: Assert that about:memory numbers are nonnegative (and <= 100%)
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Toolkit
Classification: Components
Component: about:memory (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla13
Assigned To: Jesse Ruderman
:
Mentors:
Depends on: 698930 751509
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-01 15:57 PDT by Jesse Ruderman
Modified: 2012-05-03 03:01 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.25 KB, patch)
2012-02-19 20:44 PST, Jesse Ruderman
n.nethercote: review+
Details | Diff | Review
patch v2 (3.12 KB, patch)
2012-02-19 21:19 PST, Jesse Ruderman
jruderman: review+
Details | Diff | Review

Description Jesse Ruderman 2011-11-01 15:57:33 PDT
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) 2011-11-01 15:59:22 PDT
> (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) 2011-11-01 16:00:43 PDT
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 [:njn] 2011-11-01 17:18:37 PDT
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 2011-11-01 21:41:42 PDT
You could dump() a short, unique message.
Comment 5 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-11-02 04:59:35 PDT
http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsIDebug.idl#53
Comment 6 Nicholas Nethercote [:njn] 2012-01-22 01:51:54 PST
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 [:njn] 2012-01-22 02:01:51 PST
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 2012-01-22 02:09:02 PST
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 [:njn] 2012-02-06 17:55:45 PST
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 2012-02-19 20:44:06 PST
Created attachment 598761 [details] [diff] [review]
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 [:njn] 2012-02-19 21:08:18 PST
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 2012-02-19 21:14:37 PST
> 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 2012-02-19 21:19:45 PST
Created attachment 598764 [details] [diff] [review]
patch v2
Comment 14 Nicholas Nethercote [:njn] 2012-02-20 18:59:04 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/49140e63962f
Comment 15 Ed Morley [:emorley] 2012-02-22 04:48:27 PST
https://hg.mozilla.org/mozilla-central/rev/49140e63962f

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