Closed
Bug 969786
Opened 11 years ago
Closed 11 years ago
[jsdbg2] implement Debugger.Source.prototype.introductionScript
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: jimb, Assigned: jimb)
References
Details
Attachments
(3 files, 2 obsolete files)
(deleted),
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•11 years ago
|
||
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #8372789 -
Flags: review?(sphink)
Assignee | ||
Comment 3•11 years ago
|
||
Not requesting review on this one yet, as the tests still need work.
Assignee | ||
Comment 4•11 years ago
|
||
Try push for the first two patches only:
https://tbpl.mozilla.org/?tree=Try&rev=7eaa149c7760
Assignee | ||
Comment 5•11 years ago
|
||
Flags: in-testsuite+
Target Milestone: --- → mozilla30
Assignee | ||
Comment 6•11 years ago
|
||
Ah, crap, that inbound push comment was intended for bug 979764. Marking 'leave-open'...
Whiteboard: [leave open]
Assignee | ||
Updated•11 years ago
|
Flags: in-testsuite+
Target Milestone: mozilla30 → ---
Comment 7•11 years ago
|
||
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•11 years ago
|
||
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 9•11 years ago
|
||
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.
Assignee | ||
Comment 10•11 years ago
|
||
(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.
Assignee | ||
Comment 11•11 years ago
|
||
(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.
Assignee | ||
Comment 12•11 years ago
|
||
(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
Assignee | ||
Comment 13•11 years ago
|
||
Revised per comments.
Attachment #8372789 -
Attachment is obsolete: true
Attachment #8372789 -
Flags: review?(sphink)
Attachment #8374618 -
Flags: review?(sphink)
Assignee | ||
Comment 14•11 years ago
|
||
Now, with MOAR TESTS
Attachment #8372790 -
Attachment is obsolete: true
Attachment #8374619 -
Flags: review?(sphink)
Assignee | ||
Comment 15•11 years ago
|
||
Assignee | ||
Comment 16•11 years ago
|
||
What a pristine try push! Surely you must r+ this patch after that! :D
Updated•11 years ago
|
Attachment #8374618 -
Flags: review?(sphink) → review+
Comment 17•11 years ago
|
||
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+
Comment 18•11 years ago
|
||
(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•11 years ago
|
||
(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."
Assignee | ||
Comment 20•11 years ago
|
||
(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.
Assignee | ||
Comment 21•11 years ago
|
||
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
Flags: in-testsuite+
Target Milestone: --- → mozilla30
Assignee | ||
Updated•11 years ago
|
Whiteboard: [leave open]
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
Flags: needinfo?(jimb)
Assignee | ||
Comment 23•11 years ago
|
||
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)
Assignee | ||
Comment 24•11 years ago
|
||
Comment 25•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/38e421359f5e
https://hg.mozilla.org/mozilla-central/rev/6f174fcd2b20
https://hg.mozilla.org/mozilla-central/rev/6e3b9da3d83f
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•