Last Comment Bug 703474 - Setting gczeal to 4 and back to 0 causes everything to leak
: Setting gczeal to 4 and back to 0 causes everything to leak
: mlk, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Mac OS X
-- normal with 1 vote (vote)
: mozilla14
Assigned To: Bill McCloskey (:billm)
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: 641027
  Show dependency treegraph
Reported: 2011-11-17 17:53 PST by Jesse Ruderman
Modified: 2012-03-24 13:40 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

browser testcase (requires extension) (73 bytes, text/html)
2011-11-17 17:53 PST, Jesse Ruderman
no flags Details
shell valgrind output (73 bytes, text/plain)
2011-11-17 17:54 PST, Jesse Ruderman
no flags Details
fix (1.88 KB, patch)
2012-03-22 18:18 PDT, Bill McCloskey (:billm)
igor: review+
Details | Diff | Splinter Review

Description User image Jesse Ruderman 2011-11-17 17:53:30 PST
Created attachment 575350 [details]
browser testcase (requires extension)

To reproduce in a debug browser build:

  1. Install 
  2. XPCOM_MEM_LEAK_LOG=2 ./firefox v.html

  --> trace-refcnt reports that nsGlobalWindow and more leak

To reproduce in the shell using valgrind:

  1. Build a spidermonkey shell:
    a. Create a new directory
    b. js/src/configure --enable-debug --disable-optimize --enable-valgrind
    c. make
  2. valgrind --leak-check=full --dsymutil=yes ./js -e "gczeal(4); gczeal(0);"

  --> valgrind reports a significant leak rooted in js::gc::StartVerifyBarriers
Comment 1 User image Jesse Ruderman 2011-11-17 17:54:42 PST
Created attachment 575351 [details]
shell valgrind output
Comment 2 User image Jesse Ruderman 2012-02-23 19:09:17 PST
It worries me that turning on GC debugging makes the GC stop working ;)
Comment 3 User image Bill McCloskey (:billm) 2012-03-22 18:18:01 PDT
Created attachment 608552 [details] [diff] [review]

I remember fixing the shell problem a while ago, although I can't find the bug. It no longer reproduces.

The problem in the browser case was that, even though we set the gczeal back to 0, we were still calling Start/EndVerifyBarriers repeatedly. I had thought that the verifier would stop at the next GC. However, if a GC happens during a verification, it starts a new one after the GC is done.

The current patch adds a few places where we EndVerifyBarriers if the zeal is 0. I considered just calling EndVerifyBarriers from SetGCZeal, but I'd rather have this happen from places where we already know that GC is safe.

Also, I made sure not to start a verification if we do a CC_FORCED GC, since it relies on the mark bits being correct afterwards.
Comment 4 User image Igor Bukanov 2012-03-23 02:43:37 PDT
Comment on attachment 608552 [details] [diff] [review]

Review of attachment 608552 [details] [diff] [review]:

::: js/src/jsgc.cpp
@@ +3693,5 @@
>          }
> +        ~AutoVerifyBarriers() {
> +            if (inVerify && reason != gcreason::CC_FORCED &&
> +                cx->runtime->gcZeal() == ZealVerifierValue)
> +            {

There is no reason to store inVerify and reason as they stay the same. Instead store the result of inVerify && reason != gcreason::CC_FORCED in a flag. Also comment here why it is necessary to protect StartVerifyBarriers(cx) with the extra checks.
Comment 6 User image Ed Morley [:emorley] 2012-03-24 13:40:44 PDT

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