Closed Bug 969786 Opened 6 years ago Closed 6 years ago

[jsdbg2] implement Debugger.Source.prototype.introductionScript

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: jimb, Assigned: jimb)

References

Details

Attachments

(3 files, 2 obsolete files)

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.
Assignee: nobody → jimb
Status: NEW → ASSIGNED
Attachment #8372788 - Flags: review?(sphink)
Attachment #8372789 - Flags: review?(sphink)
Not requesting review on this one yet, as the tests still need work.
Try push for the first two patches only:
https://tbpl.mozilla.org/?tree=Try&rev=7eaa149c7760
https://hg.mozilla.org/integration/mozilla-inbound/rev/66a052d05ddd
Flags: in-testsuite+
Target Milestone: --- → mozilla30
Ah, crap, that inbound push comment was intended for bug 979764. Marking 'leave-open'...
Whiteboard: [leave open]
Flags: in-testsuite+
Target Milestone: mozilla30 → ---
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 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?
Attachment #8372788 - Flags: review?(sphink) → review+
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.
(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.
(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.
(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
Revised per comments.
Attachment #8372789 - Attachment is obsolete: true
Attachment #8372789 - Flags: review?(sphink)
Attachment #8374618 - Flags: review?(sphink)
Now, with MOAR TESTS
Attachment #8372790 - Attachment is obsolete: true
Attachment #8374619 - Flags: review?(sphink)
What a pristine try push! Surely you must r+ this patch after that! :D
Attachment #8374618 - Flags: review?(sphink) → review+
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.)
Attachment #8374619 - Flags: review?(sphink) → review+
(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.
(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."
Blocks: 977255
(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.
Whiteboard: [leave open]
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...
Flags: needinfo?(jimb)
Depends on: 981097
You need to log in before you can comment on or make changes to this bug.