969786 - [jsdbg2] implement Debugger.Source.prototype.introductionScript

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

If S is a Debugger.Source instance, S.introductionScript and S.introductionOffset are the script and bytecode offset of the operation that introduced S into the system --- the call to 'eval', say.

Comment 1

[Jim Blandy :jimb]

Attachment: Add an 'introduction script' compilation option to ReadOnlyCompileOptions, OwningCompileOptions, and CompileOptions.

Comment 2

[Jim Blandy :jimb]

Attachment: Record the introduction script in ScriptSourceObjects.

Comment 3

[Jim Blandy :jimb]

Attachment: Implement Debugger.Source.prototype.introductionScript.

Not requesting review on this one yet, as the tests still need work.

Comment 4

[Jim Blandy :jimb]

Try push for the first two patches only:
https://tbpl.mozilla.org/?tree=Try&rev=7eaa149c7760

Comment 5

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/66a052d05ddd

Comment 6

[Jim Blandy :jimb]

Ah, crap, that inbound push comment was intended for bug 979764. Marking 'leave-open'...

Comment 7

[Jason Orendorff [:jorendorff]]

The only things that occur to me to test are:

- cross-compartment eval and Debugger.evalInGlobal (that is, check that the introductionScript and such are as-documented when the code was introduced from the debugger itself)

- check that introductionScript is still correct after the introducing direct eval call returns (you kind of covered this with Function, but direct eval is weird)

plus the stuff you said, particularly the stuff about running code from an empty stack.

Comment 8

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8372788 [details] [diff] [review]
Add an 'introduction script' compilation option to ReadOnlyCompileOptions, OwningCompileOptions, and CompileOptions.

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

::: js/src/builtin/Eval.cpp
@@ +391,5 @@
>                 .setForEval(true)
>                 .setNoScriptRval(false)
>                 .setPrincipals(principals)
>                 .setOriginPrincipals(originPrincipals)
> +               .setIntroductionInfo(introducerFilename, "eval", lineno, script, pcOffset);

I guess it would be too annoying to distinguish direct eval from indirect eval? Perhaps it's enough to be able to introspect the scope from Debugger?

::: js/src/jsapi.cpp
@@ +4438,5 @@
> +    // There is no equivalent of cross-compartment wrappers for scripts. If
> +    // the introduction script would be in a different compartment from the
> +    // compiled code, we would be creating a cross-compartment script
> +    // reference, which would be bogus. In that case, just don't bother to
> +    // retain the introduction script.

How common is this case? Would you fix this later? (If so, it'd be good to get a bug number into these 2 comments.)

I don't know what the "fix" is, and I certainly wouldn't hold up this patch for it. "Wrap" the script by creating a stub script? Ugh. Add an extra layer of indirection (a JSFunction or something that points to the script, only set in this case)? Or would you not want to hold the original script alive?

Comment 9

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8372789 [details] [diff] [review]
Record the introduction script in ScriptSourceObjects.

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

::: js/src/jsscript.cpp
@@ +1098,5 @@
> +
> +    /* This comes from a private pointer, so no barrier needed. */
> +    if (JSScript *script = sso->introductionScript()) {
> +        MarkCrossCompartmentScriptUnbarriered(trc, sso, &script,
> +                                              "ScriptSourceObject introductionScript");

Can you explainify this for me? You make sure introductionScript is never cross-compartment, then call MarkCrossCompartmentScriptUnbarriered on it? I must be missing something.

::: js/src/jsworkers.cpp
@@ +215,5 @@
>          // Initialize the ScriptSourceObject slots that we couldn't while the SSO
>          // was in the temporary compartment.
>          ScriptSourceObject &sso = script->sourceObject()->as<ScriptSourceObject>();
>          sso.initElement(optionsElement);
> +        sso.initIntroductionScript(optionsIntroductionScript);

Wow, this is a crazy shell game.

Comment 10

[Jim Blandy :jimb]

(In reply to Steve Fink [:sfink] from comment #8)
> Comment on attachment 8372788 [details] [diff] [review]
> Add an 'introduction script' compilation option to ReadOnlyCompileOptions,
> OwningCompileOptions, and CompileOptions.
> 
> Review of attachment 8372788 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/builtin/Eval.cpp
> @@ +391,5 @@
> >                 .setForEval(true)
> >                 .setNoScriptRval(false)
> >                 .setPrincipals(principals)
> >                 .setOriginPrincipals(originPrincipals)
> > +               .setIntroductionInfo(introducerFilename, "eval", lineno, script, pcOffset);
> 
> I guess it would be too annoying to distinguish direct eval from indirect
> eval? Perhaps it's enough to be able to introspect the scope from Debugger?

I think the 'type' values become less valuable the more fine-grained they get. For the most part we should be using duck typing on the Debugger.Source.

> ::: js/src/jsapi.cpp
> @@ +4438,5 @@
> > +    // There is no equivalent of cross-compartment wrappers for scripts. If
> > +    // the introduction script would be in a different compartment from the
> > +    // compiled code, we would be creating a cross-compartment script
> > +    // reference, which would be bogus. In that case, just don't bother to
> > +    // retain the introduction script.
> 
> How common is this case? Would you fix this later? (If so, it'd be good to
> get a bug number into these 2 comments.)

No, no intention of fixing it at the moment, because I think almost nobody will care.

The fix would be challenging, because GC depends on all cross-compartment references being registered in nice tables it can find. We have no such table for JSScript pointers. I guess we could create an object whose sole purpose in life was to refer to the script. But I don't think it's worth it.

Comment 11

[Jim Blandy :jimb]

(In reply to Steve Fink [:sfink] from comment #9)
> Comment on attachment 8372789 [details] [diff] [review]
> Record the introduction script in ScriptSourceObjects.
> 
> Review of attachment 8372789 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsscript.cpp
> @@ +1098,5 @@
> > +
> > +    /* This comes from a private pointer, so no barrier needed. */
> > +    if (JSScript *script = sso->introductionScript()) {
> > +        MarkCrossCompartmentScriptUnbarriered(trc, sso, &script,
> > +                                              "ScriptSourceObject introductionScript");
> 
> Can you explainify this for me? You make sure introductionScript is never
> cross-compartment, then call MarkCrossCompartmentScriptUnbarriered on it? I
> must be missing something.

Um, probably not; I just copied that code from DebuggerScript_trace, which traces Debugger.Script instances. Now that you point it out, it's almost certainly inappropriate, because Debugger.Script instances are always cross-compartment references to scripts (hey...) whereas ScriptSourceObjects are prevented from ever containing such references.

So this was complete cargo-cult baloney. Thanks for catching this.

> ::: js/src/jsworkers.cpp
> @@ +215,5 @@
> >          // Initialize the ScriptSourceObject slots that we couldn't while the SSO
> >          // was in the temporary compartment.
> >          ScriptSourceObject &sso = script->sourceObject()->as<ScriptSourceObject>();
> >          sso.initElement(optionsElement);
> > +        sso.initIntroductionScript(optionsIntroductionScript);
> 
> Wow, this is a crazy shell game.

Given the way off-thread compilation initially uses a separate compartment, this actually seems like the simplest approach.

We could create the ScriptSourceObject in the final compartment; but then all the JSScripts the compiler builds need to point to it, and those will all be cross-compartment references until the compartments get merged. That seems bad.

We could create the ScriptSourceObject after the compartment merge, but then we'd have to find all the JSScripts and make them refer to it. That seems ugly.

So filling in the ScriptSourceObject slots after the merge seems like the best option we have.

Comment 12

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #7)
> The only things that occur to me to test are:
> 
> - cross-compartment eval and Debugger.evalInGlobal (that is, check that the
> introductionScript and such are as-documented when the code was introduced
> from the debugger itself)

For now I'm just tossing all cross-compartment eval introduction information, on the grounds that nobody will notice and it's a pain. If we later decide we care, we can revisit. But yes, we should check that we do neatly drop the information in those cases.

> - check that introductionScript is still correct after the introducing
> direct eval call returns (you kind of covered this with Function, but direct
> eval is weird)

Hmm, that's true, the tests all run while the frame is still live. I'll do that.

> plus the stuff you said, particularly the stuff about running code from an
> empty stack.

The only yet-unimplemented case I mentioned in IRC was:

- bind eval to a string, and then call it from the event loop directly

Comment 13

[Jim Blandy :jimb]

Attachment: Record the introduction script in ScriptSourceObjects.

Revised per comments.

Comment 14

[Jim Blandy :jimb]

Attachment: Implement Debugger.Source.prototype.introductionScript.

Now, with MOAR TESTS

Comment 15

[Jim Blandy :jimb]

Try:
https://tbpl.mozilla.org/?tree=Try&rev=d6d28596c2bc

Comment 16

[Jim Blandy :jimb]

What a pristine try push! Surely you must r+ this patch after that! :D

Comment 17

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8374619 [details] [diff] [review]
Implement Debugger.Source.prototype.introductionScript.

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

::: toolkit/devtools/server/tests/mochitest/test_Debugger.Source.prototype.introductionScript.html
@@ +44,5 @@
> +  }
> +
> +  function debuggerHandler(frame) {
> +    // The top stack frame's source should have an undefined
> +    // introduction script and introduction offset.

Hm. Can you also test whether non-toplevel introduction scripts are defined? (Just to rule out the possibility that this test passes because code running in iframes never has an introduction script set. Because you botched the compartment handling, or something. I dunno.)

Comment 18

[Jason Orendorff [:jorendorff]]

(In reply to Jim Blandy :jimb from comment #10)
> > ::: js/src/jsapi.cpp
> > @@ +4438,5 @@
> > > +    // There is no equivalent of cross-compartment wrappers for scripts. If
> > > +    // the introduction script would be in a different compartment from the
> > > +    // compiled code, we would be creating a cross-compartment script
> > > +    // reference, which would be bogus. In that case, just don't bother to
> > > +    // retain the introduction script.
> > 
> > How common is this case? Would you fix this later? (If so, it'd be good to
> > get a bug number into these 2 comments.)
> 
> No, no intention of fixing it at the moment, because I think almost nobody
> will care.
> 
> The fix would be challenging, because GC depends on all cross-compartment
> references being registered in nice tables it can find. We have no such
> table for JSScript pointers.

Agreed.

Debugger.Script objects have a cross-compartment pointer to a JSScript. The table is Debugger::scripts, a WeakMap. So it's possible. But about as painful to maintain as you'd expect.

Comment 19

[Steve Fink [:sfink] [:s:]]

(In reply to Jason Orendorff [:jorendorff] from comment #18)
> (In reply to Jim Blandy :jimb from comment #10)
> > > ::: js/src/jsapi.cpp
> > > @@ +4438,5 @@
> > > > +    // There is no equivalent of cross-compartment wrappers for scripts. If
> > > > +    // the introduction script would be in a different compartment from the
> > > > +    // compiled code, we would be creating a cross-compartment script
> > > > +    // reference, which would be bogus. In that case, just don't bother to
> > > > +    // retain the introduction script.
> > > 
> > > How common is this case? Would you fix this later? (If so, it'd be good to
> > > get a bug number into these 2 comments.)
> > 
> > No, no intention of fixing it at the moment, because I think almost nobody
> > will care.
> > 
> > The fix would be challenging, because GC depends on all cross-compartment
> > references being registered in nice tables it can find. We have no such
> > table for JSScript pointers.
> 
> Agreed.
> 
> Debugger.Script objects have a cross-compartment pointer to a JSScript. The
> table is Debugger::scripts, a WeakMap. So it's possible. But about as
> painful to maintain as you'd expect.

I have no objection to leaving this as-is. I just wanted to know how common the case was. Sounds like it's rare enough to be a valid "don't care, go away."

Comment 20

[Jim Blandy :jimb]

(In reply to Steve Fink [:sfink] from comment #17)
> Hm. Can you also test whether non-toplevel introduction scripts are defined?
> (Just to rule out the possibility that this test passes because code running
> in iframes never has an introduction script set. Because you botched the
> compartment handling, or something. I dunno.)

Okay, I added a test that does:

    var script2 = doc.createElement('script');
    script2.text = "eval('debugger;');";
    script2DO = iframeDO.makeDebuggeeValue(script2);
    dbg.onDebuggerStatement = evalHandler;
    doc.body.appendChild(script2);

and then looks at the introduction information for the script containing the 'debugger;' statement, and the script that calls it.

Comment 21

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d0b03e13765
https://hg.mozilla.org/integration/mozilla-inbound/rev/198decf16acf
https://hg.mozilla.org/integration/mozilla-inbound/rev/a79a64806e6c

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/782acfe6edfd for m-oth test bustage:
https://tbpl.mozilla.org/php/getParsedLog.php?id=35315169&tree=Mozilla-Inbound

Comment 23

[Jim Blandy :jimb]

That's embarrassing. I had deliberately introduced a failure into the test, to ensure that it was indeed getting tested. And naturally, I forgot to un-break it before pushing...

Comment 24

[Jim Blandy :jimb]

Second attempt to land:

https://hg.mozilla.org/integration/mozilla-inbound/rev/38e421359f5e
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f174fcd2b20
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e3b9da3d83f

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/38e421359f5e
https://hg.mozilla.org/mozilla-central/rev/6f174fcd2b20
https://hg.mozilla.org/mozilla-central/rev/6e3b9da3d83f