1203696 - Crash [@ js::ScriptSourceObject::source] with Debugger

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision a6786bf8d71d (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --target=i686-pc-linux-gnu --disable-tests --enable-debug, run with --fuzzing-safe --thread-count=2 --ion-extra-checks):

var g = newGlobal();
g.eval(`function f() { return function() {
  function g() {}
}; }`);
new Debugger(g).findObjects();


Backtrace:

Program terminated with signal SIGSEGV, Segmentation fault.
#0  js::ScriptSourceObject::source (this=0x0) at js/src/jsobj.h:122
#1  0x087ed696 in scriptSource (this=<optimized out>) at js/src/jsscript.h:2159
#2  JSFunction::createScriptForLazilyInterpretedFunction (cx=cx@entry=0xf71033d0, fun=fun@entry=...) at js/src/jsfun.cpp:1392
#3  0x080eeac2 in JSFunction::getOrCreateScript (this=0xf47ab240, cx=0xf71033d0) at js/src/shell/../jsfun.h:378
#4  0x082f6eb1 in EnsureFunctionHasScript (fun=..., cx=0xf71033d0) at js/src/vm/Debugger.cpp:116
#5  js::Debugger::wrapDebuggeeValue (this=this@entry=0xf49e0800, cx=cx@entry=0xf71033d0, vp=vp@entry=...) at js/src/vm/Debugger.cpp:812
#6  0x082fc2d2 in js::Debugger::findObjects (cx=0xf71033d0, argc=0, vp=0xf4fb8150) at js/src/vm/Debugger.cpp:4082
#7  0x0832bd2a in js::CallJSNative (cx=0xf71033d0, native=0x82fbd20 <js::Debugger::findObjects(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
#8  0x0831b697 in js::Invoke (cx=0xf71033d0, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:763
#9  0x0831447c in Interpret (cx=cx@entry=0xf71033d0, state=...) at js/src/vm/Interpreter.cpp:3067
#10 0x0831ad61 in js::RunScript (cx=cx@entry=0xf71033d0, state=...) at js/src/vm/Interpreter.cpp:704
#11 0x08325dce in js::ExecuteKernel (cx=cx@entry=0xf71033d0, script=..., script@entry=..., scopeChainArg=..., thisv=..., newTargetValue=..., type=type@entry=js::EXECUTE_DIRECT_EVAL, evalInFrame=evalInFrame@entry=..., result=result@entry=0xffa6da68) at js/src/vm/Interpreter.cpp:978
#12 0x08214b50 in js::DirectEvalStringFromIon (cx=0xf71033d0, scopeobj=..., callerScript=..., thisValue=..., newTargetValue=..., str=..., pc=0xf7142914 "{", vp=...) at js/src/builtin/Eval.cpp:449
#13 0xf76eefe6 in ?? ()
eax	0x0	0
ebx	0x97b142c	159061036
ecx	0x1	1
edx	0xf47ac010	-193282032
esi	0x0	0
edi	0x10	16
ebp	0xffa6ce58	4289121880
esp	0xffa6ce40	4289121856
eip	0x832a61a <js::ScriptSourceObject::source() const+10>
=> 0x832a61a <js::ScriptSourceObject::source() const+10>:	mov    (%esi),%eax
   0x832a61c <js::ScriptSourceObject::source() const+12>:	mov    (%eax),%eax

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/827e17b69b0f
user:        Jim Blandy
date:        Thu Jun 04 14:08:20 2015 -0700
summary:     Bug 1163520: Don't hand out internal function objects via Debugger.Environment.prototype.callee. r=shu

This iteration took 278.159 seconds to run.

Comment 2

[Jim Blandy :jimb]

Shu has explained to me that this is probably due to js::Debugger::ObjectQuery not calling JSCompartment::ensureDelazifyScriptsForDebugger.

Comment 3

[Jim Blandy :jimb]

Attachment: Don't collect allocation metadata when lazily creating standard prototypes.

Requesting feedback only since this has no test.

Comment 4

[Jim Blandy :jimb]

Try push for the above: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7bfe731863f1

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8703995 [details] [diff] [review]
Don't collect allocation metadata when lazily creating standard prototypes.

Review of attachment 8703995 [details] [diff] [review]:
-----------------------------------------------------------------

Does this depend on some other bug? AutoSuppressObjectMetadataCallback doesn't seem to be in the tree yet. That said, we've discussed this general approach in person, and I think it is the right way to move forward.

Comment 6

[Jim Blandy :jimb]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #5)
> Comment on attachment 8703995 [details] [diff] [review]
> Don't collect allocation metadata when lazily creating standard prototypes.
> 
> Review of attachment 8703995 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Does this depend on some other bug? AutoSuppressObjectMetadataCallback
> doesn't seem to be in the tree yet. That said, we've discussed this general
> approach in person, and I think it is the right way to move forward.

I just totally attached the patch to the wrong bug. It belonged on bug 1221378.

Comment 7

[Jan de Mooij [:jandem]]

What's the status of this? The fuzzers hit this crash a lot.

Comment 8

[Jim Blandy :jimb]

Sorry, I did drop this. Shu and I discussed this, and the quick fix expected in comment 2 is not correct. I'm debugging now.

Comment 9

[Jim Blandy :jimb]

Attachment: Improve comments about lazily-initialized members of js::LazyScript.

Comment 10

[Jim Blandy :jimb]

Attachment: Make js::IsInternalFunctionObject consider non-lambdas internal.

I think this change should be okay; I can't see any justification for testing the LAMBDA flag when deciding whether a function object is internal or not; function declarations produce internal function objects too, right?

I looked in the history, and the definition using LAMBDA originates in bug 518103. Perhaps it was being used for something different at that point.

I looked at the present uses of js::IsInternalFunctionObject, and I didn't see any subtleties to suggest that only lambdas could be internal.

Comment 11

[Jim Blandy :jimb]

In any case, the analysis of the bug:

Debugger.prototype.findObjects walks the GC arenas looking for objects that meet certain criteria, and returns an array of them. The devtools use this to find promises to display in the promises debugger. (Such traversals are often a source of difficulty; we're not adding any more of them, and looking for opportunities to remove the ones we have.)

As a consequence, it can come across internal function objects, which should never be exposed to JS. We do try to filter out these objects, but because of js::IsInternalFunctionObject's odd definition, we're letting some through. That's the bug; the crash occurs when we try to de-lazify an (internal) function object whose parent script hasn't been byte-compiled: the `LazyScript` hasn't had its `setParent` method called yet, so its `sourceObject_` field is still uninitialized.

Comment 12

[Jim Blandy :jimb]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c511cd4d24bc

Comment 13

[Brian Hackett [Laid off!]]

Comment on attachment 8713437 [details] [diff] [review]
Make js::IsInternalFunctionObject consider non-lambdas internal.

Review of attachment 8713437 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsobjinlines.h
@@ +590,5 @@
>  IsInternalFunctionObject(JSObject& funobj)
>  {
>      JSFunction& fun = funobj.as<JSFunction>();
>      MOZ_ASSERT_IF(fun.isLambda(),
>                    fun.isInterpreted() || fun.isAsmJSNative());

I don't think this assert is necessary anymore.

Comment 14

[Jim Blandy :jimb]

Try push looks *excellent*.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e04da99412e7
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6fd06aa2782

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e04da99412e7
https://hg.mozilla.org/mozilla-central/rev/a6fd06aa2782