Last Comment Bug 743311 - Expose a way to list all global objects and be notified when a new global is created
: Expose a way to list all global objects and be notified when a new global is ...
Status: RESOLVED FIXED
[chrome-debug]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla19
Assigned To: Jim Blandy :jimb
:
Mentors:
: 740547 (view as bug list)
Depends on: 799272
Blocks: 740551
  Show dependency treegraph
 
Reported: 2012-04-06 11:50 PDT by Jason Orendorff [:jorendorff]
Modified: 2012-12-04 12:58 PST (History)
8 users (show)
jimb: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add option to shell 'evaluate' to catch termination, for tests. (2.48 KB, patch)
2012-09-23 18:03 PDT, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Splinter Review
Implement Debugger.prototype.onNewGlobalObject. (23.91 KB, patch)
2012-09-23 18:04 PDT, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Splinter Review
[revised per comments] Implement Debugger.prototype.onNewGlobalObject. (25.56 KB, patch)
2012-09-27 10:00 PDT, Jim Blandy :jimb
no flags Details | Diff | Splinter Review
Implement Debugger.prototype.findAllGlobals. (4.65 KB, patch)
2012-09-27 10:45 PDT, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2012-04-06 11:50:52 PDT
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) 2012-09-01 12:35:27 PDT
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 [:njn] 2012-09-02 20:20:38 PDT
(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 2012-09-18 16:47:08 PDT
First draft of new global creation tracking here:
https://github.com/jimblandy/DebuggerDocs/compare/onNewGlobal
Comment 4 Jim Blandy :jimb 2012-09-18 16:47:45 PDT
Missing is a way to discover existing globals. That's next.
Comment 5 Jim Blandy :jimb 2012-09-23 18:01:02 PDT
Updated doc branch now includes enumeration, new global creation tracking, and annotations.
Comment 6 Jim Blandy :jimb 2012-09-23 18:03:36 PDT
Created attachment 663883 [details] [diff] [review]
Add option to shell 'evaluate' to catch termination, for tests.
Comment 7 Jim Blandy :jimb 2012-09-23 18:04:41 PDT
Created attachment 663884 [details] [diff] [review]
Implement Debugger.prototype.onNewGlobalObject.
Comment 8 Jim Blandy :jimb 2012-09-23 18:05:06 PDT
Comments on design are always welcome as well.
Comment 9 Jim Blandy :jimb 2012-09-23 18:06:28 PDT
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 2012-09-23 18:13:17 PDT
Try push for those first two patches: https://tbpl.mozilla.org/?tree=Try&rev=b54b80a45662
Comment 11 Jason Orendorff [:jorendorff] 2012-09-25 11:44:21 PDT
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] 2012-09-26 11:36:12 PDT
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 2012-09-26 20:26:48 PDT
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 2012-09-27 10:00:26 PDT
Created attachment 665519 [details] [diff] [review]
[revised per comments] Implement Debugger.prototype.onNewGlobalObject.

I think this addresses the comments.
Comment 15 Jim Blandy :jimb 2012-09-27 10:07:31 PDT
Fresh try:
https://tbpl.mozilla.org/?tree=Try&rev=479b37b2134c
Comment 16 Jim Blandy :jimb 2012-09-27 10:45:29 PDT
Created attachment 665536 [details] [diff] [review]
Implement Debugger.prototype.findAllGlobals.
Comment 17 Jim Blandy :jimb 2012-09-27 18:00:49 PDT
Try for findAllGlobals:
https://tbpl.mozilla.org/?tree=Try&rev=40b0519addac
Comment 18 Jim Blandy :jimb 2012-09-28 10:00:32 PDT
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] 2012-10-01 16:48:16 PDT
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] 2012-10-01 16:59:29 PDT
(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 2012-10-04 18:12:55 PDT
(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 2012-10-09 14:24:44 PDT
(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 2012-10-09 14:35:59 PDT
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 2012-10-09 14:56:38 PDT
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 2012-10-09 14:59:24 PDT
(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 2012-10-09 16:08:17 PDT
I'm going to file a separate bug for hostAnnotations.
Comment 27 Jim Blandy :jimb 2012-10-12 13:04:09 PDT
(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 30 Jim Blandy :jimb 2012-12-04 12:58:07 PST
*** Bug 740547 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.