Closed Bug 698928 Opened 8 years ago Closed 8 years ago

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

Categories

(Toolkit :: about:memory, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: jruderman, Assigned: jruderman)

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 file, 1 obsolete file)

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?)
Whiteboard: [MemShrink]
> (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?
We could do something like

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

and then visually highlight the bad value, too.
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.
You could dump() a short, unique message.
Assignee: nobody → nnethercote
Whiteboard: [MemShrink] → [MemShrink:P2]
Jesse, I assume browser.dom.window.dump.enabled is on in your fuzzers?  Any preferences for what the dumped message should look like?
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?
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.
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."
Assignee: n.nethercote → jruderman
Attached patch patch (obsolete) — Splinter Review
* 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 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?
> I'm happy to un-bitrot this one and land it for you when bug 702300 is done.

Sounds good, thanks!
Attached patch patch v2Splinter Review
Attachment #598764 - Flags: review+
Attachment #598761 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/49140e63962f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Depends on: 751509
You need to log in before you can comment on or make changes to this bug.