578233 - GC warning during startup

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Gregor Wagner [:gwagner]]

A browser startup from the current tip with --enable-gczeal in valgrind gives the following warning:

==65063== Thread 1:
==65063== Invalid read of size 8
==65063==    at 0x1047A54A3: js::ConservativeGCStackMarker::markRange(unsigned long*, unsigned long*) (in /Users/mozilla/gregor/ws1/Debug/js/src/libmozjs.dylib)
==65063==    by 0x1047A5563: js::ConservativeGCStackMarker::markRoots() (in /Users/mozilla/gregor/ws1/Debug/js/src/libmozjs.dylib)
==65063==    by 0x1047A7574: js_TraceRuntime(JSTracer*) (in /Users/mozilla/gregor/ws1/Debug/js/src/libmozjs.dylib)
==65063==    by 0x1047A76B7: GC(JSContext*) (in /Users/mozilla/gregor/ws1/Debug/js/src/libmozjs.dylib)
==65063==    by 0x1047A7AD1: GCUntilDone(JSContext*, JSGCInvocationKind) (in /Users/mozilla/gregor/ws1/Debug/js/src/libmozjs.dylib)
==65063==    by 0x1047A7C1B: js_GC(JSContext*, JSGCInvocationKind) (in /Users/mozilla/gregor/ws1/Debug/js/src/libmozjs.dylib)
==65063==    by 0x1047331E9: JS_GC (in /Users/mozilla/gregor/ws1/Debug/js/src/libmozjs.dylib)
==65063==    by 0x1008F0204: MaybeGC(JSContext*) (in /Users/mozilla/gregor/ws1/Debug/toolkit/library/XUL)
==65063==    by 0x1008F3823: nsJSContext::ScriptEvaluated(int) (in /Users/mozilla/gregor/ws1/Debug/toolkit/library/XUL)
==65063==    by 0x1005BC248: nsCxPusher::Pop() (in /Users/mozilla/gregor/ws1/Debug/toolkit/library/XUL)
==65063==    by 0x1005BC284: nsCxPusher::~nsCxPusher() (in /Users/mozilla/gregor/ws1/Debug/toolkit/library/XUL)
==65063==    by 0x10057195E: nsTextBoxFrame::UpdateAccesskey(nsWeakFrame&) (in /Users/mozilla/gregor/ws1/Debug/toolkit/library/XUL)
==65063==  Address 0x7fff5fbf4370 is not stack'd, malloc'd or (recently) free'd

Comment 1

[Igor Bukanov]

It seems that defence from the bug 572678 is not enough as the scanne read the memory that is outside of any current stack. 

Nicholas, Julian, could comment on the best way to silence the invalid read warning?

Comment 2

[Julian Seward [:jseward]]

I wouldn't assume that this is a false positive -- it might or might
not be.  Can you figure out what the complained about address
(0x7fff5fbf4370) is, and in particular what it is relative to the
program's %RSP ?

Comment 3

[Gregor Wagner [:gwagner]]

I also think the technique from bug 572678 works pretty well and for a browser start with gczeal I only get a warning for this particular call stack coming from nsCxPusher::Pop(). All the other GCs have no warnings at all. Igor do you know if there is something special about this particular control flow?

Comment 4

[Gregor Wagner [:gwagner]]

And furthermore the warning is in markRange and not in markWord. So the question is if the stack boundaries are set correctly.

Comment 5

[Igor Bukanov]

(In reply to comment #4)
> And furthermore the warning is in markRange and not in markWord. So the
> question is if the stack boundaries are set correctly.

The stack boundaries may not be set correctly as the GC uses the bounds set by the last call to JS_SuspendRequest. That could be outside the current stack bounds like in the following example:

indirect_call_to_JS_SuspendRequest(); // [1]
JS_GC();

If [1] leads to a few extra stack frames before the actual call to SuspendRequest, than the stack pointer captured inside that call be outside the stack bounds during the call to JS_GC.

This could be the case here. Consider nsCxPusher::Pop() implementation,  http://hg.mozilla.org/tracemonkey/file/49abcb57270a/content/base/src/nsContentUtils.cpp#l2925 :

2939   JSContext *unused;
2940   stack->Pop(&unused);
2941 
2942   NS_ASSERTION(unused == mPushedContext, "Unexpected context popped");
2943 
2944   if (!mScriptIsRunning && mScx) {
2945     // No JS is running in the context, but executing the event handler might have
2946     // caused some JS to run. Tell the script context that it's done.
2947 
2948     mScx->ScriptEvaluated(PR_TRUE);
2949   }

Here JS_SuspendRequest is called indirectly via stack->Pop(&unused) so there would be stack frames for XPCJSContextStack::Pop, JS_SuspendRequest and ConservativeGCThreadData::enable before SP is captured. That could take more stack space in a debug build than the stack from the comment 0. I will mitigate this by making sure that the GC thread stack is never scanned beyond the current pointer.

Of cause, this would not help if the problem comes from a suspend request for non-GC threads but on a browser startup this is not likely.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Still a valid non-TM bug?

Comment 7

[Igor Bukanov]

(In reply to Ryan VanderMeulen from comment #6)
> Still a valid non-TM bug?

Yes. I suppose it becomes works-for-me after we switch to single-threaded JS runtime.