743311 - Expose a way to list all global objects and be notified when a new global is created

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Content debugging can just surf the frame tree, but chrome debugging needs all the globals. So far we've been kind of limping along with just .jsm globals, and it's not really enough.

Comment 1

[Rob Campbell [:rc] (:robcee)]

was looking in aboutMemory.js this weekend and they have some methods for enumerating all of the compartments that have been registered via the registerReporter mechanism in nsIMemoryReporterManager. Not really ideal, and no way to get at the underlying global without some additional XPConnect goop, afaict. Also, I'm not sure this will get everything. There may be globals hiding under the covers that don't register themselves as a reporter.

Nevertheless, an interesting experiment.

Comment 2

[Nicholas Nethercote [inactive]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #1)
> was looking in aboutMemory.js this weekend and they have some methods for
> enumerating all of the compartments that have been registered via the
> registerReporter mechanism in nsIMemoryReporterManager. Not really ideal,
> and no way to get at the underlying global without some additional XPConnect
> goop, afaict. Also, I'm not sure this will get everything. There may be
> globals hiding under the covers that don't register themselves as a reporter.

That's not quite right.

The JS multi-reporter produces info about every compartment, using CollectRuntimeStats(), which uses IterateCompartmentsArenasCells().  There's also JS_IterateCompartments().  The latter two functions will traverse every live compartment.  These functions are all C++, which it sounds like you don't want, but I think JS_IterateCompartments() is the most likely foundation for what you want to do.

Comment 3

[Jim Blandy :jimb]

First draft of new global creation tracking here:
https://github.com/jimblandy/DebuggerDocs/compare/onNewGlobal

Comment 4

[Jim Blandy :jimb]

Missing is a way to discover existing globals. That's next.

Comment 5

[Jim Blandy :jimb]

Updated doc branch now includes enumeration, new global creation tracking, and annotations.

Comment 6

[Jim Blandy :jimb]

Attachment: Add option to shell 'evaluate' to catch termination, for tests.

Comment 7

[Jim Blandy :jimb]

Attachment: Implement Debugger.prototype.onNewGlobalObject.

Comment 8

[Jim Blandy :jimb]

Comments on design are always welcome as well.

Comment 9

[Jim Blandy :jimb]

Note that this does not implement:
- Debugger.prototype.findAllGlobals
- Debugger.Object.prototype.hostAnnotations
- any actual interesting host object annotations.

Upcoming.

Comment 10

[Jim Blandy :jimb]

Try push for those first two patches: https://tbpl.mozilla.org/?tree=Try&rev=b54b80a45662

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 663883 [details] [diff] [review]
Add option to shell 'evaluate' to catch termination, for tests.

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

::: js/src/jit-test/tests/basic/evaluate-catchTermination.js
@@ +1,5 @@
> +// Test evaluate's catchTermination option.
> +
> +var x = 0;
> +assertEq(evaluate('x = 1; terminate(); x = 2;', { catchTermination: true }),
> +         "terminated");

Looks like you probably intended to assertEq(x, 1) afterwards.

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 663884 [details] [diff] [review]
Implement Debugger.prototype.onNewGlobalObject.

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

Good surface tests, but more of the actual functionality needs tests:

- Exactly 1 argument is passed to the hook, and it is a Debugger.Object
  wrapping the new global:
    var dbg = new Debugger;
    var saved;
    dbg.onNewGlobalObject = function (x) {
        assertEq(arguments.length, 1);
        saved = x;
    };
    var g = newGlobal();
    assertEq(dbg.addDebuggee(g), saved);

- 'this' is the Debugger object.

- The onNewGlobalObject hook can add the new global as a debuggee.
  It would be nice to check that debugging really works.

- The onNewGlobalObject hook can run code in the new global.  The new
  global's proto chain and lazy properties work as expected.  (I mean:
  by the time the hook is called, the new global is ready to go.)

::: js/src/jit-test/tests/debug/Debugger-onNewGlobalObject-09.js
@@ +13,5 @@
> +// For onNewGlobalObject, { return: V } resumption values are treated like
> +// 'undefined': the new global is still returned.
> +dbg.onNewGlobalObject = function (g) { log += 'n'; return { return: "snoo" }; };
> +log = '';
> +assertEq(typeof newGlobal(), "object");

This is kind of a weird thing to let pass silently, but ok.

::: js/src/jsapi.cpp
@@ +3373,4 @@
>      cx->setCompartment(saved);
>  
> +    if (!Debugger::onNewGlobalObject(cx, global))
> +        return NULL;

This should only happen if global is non-null (i.e. on success).

Comment 13

[Jim Blandy :jimb]

Thanks for the review. I'll address these comments.

I was about to write the more thorough functionality tests when I came across bug 794726; I'll post a fix for that, and then add the tests.

> This is kind of a weird thing to let pass silently, but ok.

Are you okay with it? It seems to me JS_NewGlobalObject's callers shouldn't be expected to cope with non-GlobalObject return values. And I couldn't imagine a circumstance when it would be appropriate to substitute a different global.

> This should only happen if global is non-null (i.e. on success).

Duh. Thanks!

Comment 14

[Jim Blandy :jimb]

Attachment: [revised per comments] Implement Debugger.prototype.onNewGlobalObject.

I think this addresses the comments.

Comment 15

[Jim Blandy :jimb]

Fresh try:
https://tbpl.mozilla.org/?tree=Try&rev=479b37b2134c

Comment 16

[Jim Blandy :jimb]

Attachment: Implement Debugger.prototype.findAllGlobals.

Comment 17

[Jim Blandy :jimb]

Try for findAllGlobals:
https://tbpl.mozilla.org/?tree=Try&rev=40b0519addac

Comment 18

[Jim Blandy :jimb]

These patches depend on the patches for bug 794726 and bug 795119; there are a bunch of fixes for minor bugs elsewhere in Debugger that the tests depend on.

Comment 19

[Jason Orendorff [:jorendorff]]

Comment on attachment 665536 [details] [diff] [review]
Implement Debugger.prototype.findAllGlobals.

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

r=me when the tests pass; they depend on other patches that didn't get review yet. :-\

::: js/src/jit-test/tests/debug/Debugger-findAllGlobals-01.js
@@ +18,5 @@
> +                       TypeError);
> +var a = dbg.findAllGlobals();
> +assertEq(a instanceof Array, true);
> +assertEq(a.length > 0, true);
> +for each (g in a) {

for (g of a)

::: js/src/jit-test/tests/debug/Debugger-findAllGlobals-02.js
@@ +10,5 @@
> +
> +var a = dbg.findAllGlobals();
> +
> +var g1w = dbg.addDebuggee(g1);
> +var g3w = dbg.addDebuggee(g3);

Did you mean for g1 and g3 to be debuggees at the time findAllGlobals was called?

@@ +17,5 @@
> +// from their own compartments; this is the sort that findAllGlobals and
> +// addDebuggee return.
> +var g2w = g1w.makeDebuggeeValue(g2).makeDebuggeeValue(g2);
> +var g4w = g1w.makeDebuggeeValue(g4).makeDebuggeeValue(g4);
> +var thisw = g2w.makeDebuggeeValue(this).makeDebuggeeValue(this);

I imagine these being written like this:
  var g2w = g1w.makeDebuggeeValue(g2).unwrapCrossCompartmentWrapper();
makeDebuggeeValue returns an object whose referent is a cross-comparment wrapper in g1's compartment of g2.

Comment 20

[Jason Orendorff [:jorendorff]]

(In reply to Jim Blandy :jimb from comment #13)
> > This is kind of a weird thing to let pass silently, but ok.
> 
> Are you okay with it?

Yes.

> It seems to me JS_NewGlobalObject's callers shouldn't
> be expected to cope with non-GlobalObject return values. And I couldn't
> imagine a circumstance when it would be appropriate to substitute a
> different global.

Yes, certainly; I just meant that I would have expected it to throw, triggering uncaughtExceptionHook etc. That's what happens when the resumption value is something crazy. I would expect the same thing if you returned {yield: 0} when the topmost frame isn't in a generator.

Comment 21

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #20)
> Yes, certainly; I just meant that I would have expected it to throw,
> triggering uncaughtExceptionHook etc. That's what happens when the
> resumption value is something crazy. I would expect the same thing if you
> returned {yield: 0} when the topmost frame isn't in a generator.

That's not a bad idea.

Comment 22

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #21)
> (In reply to Jason Orendorff [:jorendorff] from comment #20)
> > Yes, certainly; I just meant that I would have expected it to throw,
> > triggering uncaughtExceptionHook etc. That's what happens when the
> > resumption value is something crazy. I would expect the same thing if you
> > returned {yield: 0} when the topmost frame isn't in a generator.
> 
> That's not a bad idea.

I hope it's not just fatigue, but I'm going to leave this unchanged. I'd need to add a third boolean flag to parseResumptionValue and handleUncaughtException that they'd propagate through, to make sure that neither the hook nor the uncaught exception handler tried to force a return. It doesn't seem worth it for a minor gain in error reporting.

Comment 23

[Jim Blandy :jimb]

The new functions on which these tests now depend are in bug 799272.

We decided not to make the change in bug 795119 (although it lives on as an error handling bug).

Bug 794726 is now resolved as invalid; these tests no longer use those testing functions, or depend on the change made there.

Comment 24

[Jim Blandy :jimb]

New try: https://tbpl.mozilla.org/?tree=Try&rev=4591cd0305cc

Will land if this looks good, and the new prerequisites are approved.

Comment 25

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #19)
> > +for each (g in a) {
> 
> for (g of a)

Fixed, thanks.

> ::: js/src/jit-test/tests/debug/Debugger-findAllGlobals-02.js
> @@ +10,5 @@
> > +
> > +var a = dbg.findAllGlobals();
> > +
> > +var g1w = dbg.addDebuggee(g1);
> > +var g3w = dbg.addDebuggee(g3);
> 
> Did you mean for g1 and g3 to be debuggees at the time findAllGlobals was
> called?

Yes, I did --- thanks.

> @@ +17,5 @@
> > +// from their own compartments; this is the sort that findAllGlobals and
> > +// addDebuggee return.
> > +var g2w = g1w.makeDebuggeeValue(g2).makeDebuggeeValue(g2);
> > +var g4w = g1w.makeDebuggeeValue(g4).makeDebuggeeValue(g4);
> > +var thisw = g2w.makeDebuggeeValue(this).makeDebuggeeValue(this);
> 
> I imagine these being written like this:
>   var g2w = g1w.makeDebuggeeValue(g2).unwrapCrossCompartmentWrapper();
> makeDebuggeeValue returns an object whose referent is a cross-comparment
> wrapper in g1's compartment of g2.

Indeed, these now use '.unwrap()'.

Comment 26

[Jim Blandy :jimb]

I'm going to file a separate bug for hostAnnotations.

Comment 27

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #22)
> It doesn't seem worth it for a minor gain in error reporting.

I just know I'm going to regret this...

Comment 28

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10f0632888bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/e44a8b7579fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/801ed9c31fd5

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/10f0632888bb
https://hg.mozilla.org/mozilla-central/rev/e44a8b7579fb
https://hg.mozilla.org/mozilla-central/rev/801ed9c31fd5