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

RESOLVED FIXED in mozilla13

Status

()

Toolkit
about:memory
--
enhancement
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: Jesse Ruderman)

Tracking

Trunk
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment, 1 obsolete attachment)

3.12 KB, patch
Jesse Ruderman
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 4

6 years ago
You could dump() a short, unique message.
http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsIDebug.idl#53
Assignee: nobody → nnethercote
Whiteboard: [MemShrink] → [MemShrink:P2]
Depends on: 698930
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?
(Assignee)

Comment 8

6 years ago
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
(Assignee)

Comment 10

5 years ago
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 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?
Attachment #598761 - Flags: review+
(Assignee)

Comment 12

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

Sounds good, thanks!
(Assignee)

Comment 13

5 years ago
Created attachment 598764 [details] [diff] [review]
patch v2
Attachment #598764 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #598761 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/49140e63962f
https://hg.mozilla.org/mozilla-central/rev/49140e63962f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
(Assignee)

Updated

5 years ago
Depends on: 751509
You need to log in before you can comment on or make changes to this bug.