Closed
Bug 698928
Opened 13 years ago
Closed 13 years ago
Assert that about:memory numbers are nonnegative (and <= 100%)
Categories
(Toolkit :: about:memory, enhancement)
Toolkit
about:memory
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: jruderman, Assigned: jruderman)
References
Details
(Whiteboard: [MemShrink:P2])
Attachments
(1 file, 1 obsolete file)
3.12 KB,
patch
|
jruderman
:
review+
|
Details | Diff | Splinter Review |
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?)
Updated•13 years ago
|
Whiteboard: [MemShrink]
Comment 1•13 years ago
|
||
> (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•13 years ago
|
||
We could do something like <span class='bad-value'>-400MB</span> heap-unclassified and then visually highlight the bad value, too.
Comment 3•13 years ago
|
||
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•13 years ago
|
||
You could dump() a short, unique message.
Updated•13 years ago
|
Assignee: nobody → nnethercote
Whiteboard: [MemShrink] → [MemShrink:P2]
Comment 6•13 years ago
|
||
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•13 years ago
|
||
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•13 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.
Comment 9•13 years ago
|
||
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•13 years ago
|
||
* 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•13 years ago
|
||
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?
Updated•13 years ago
|
Attachment #598761 -
Flags: review+
Assignee | ||
Comment 12•13 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•13 years ago
|
||
Attachment #598764 -
Flags: review+
Assignee | ||
Updated•13 years ago
|
Attachment #598761 -
Attachment is obsolete: true
Comment 15•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/49140e63962f
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in
before you can comment on or make changes to this bug.
Description
•