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

RESOLVED FIXED in mozilla19

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jorendorff, Assigned: jimb)

Tracking

Other Branch
mozilla19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [chrome-debug])

Attachments

(4 attachments)

(Reporter)

Description

5 years ago
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.
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.
(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.
(Assignee)

Comment 3

5 years ago
First draft of new global creation tracking here:
https://github.com/jimblandy/DebuggerDocs/compare/onNewGlobal
(Assignee)

Comment 4

5 years ago
Missing is a way to discover existing globals. That's next.
(Assignee)

Comment 5

5 years ago
Updated doc branch now includes enumeration, new global creation tracking, and annotations.
(Assignee)

Comment 6

5 years ago
Created attachment 663883 [details] [diff] [review]
Add option to shell 'evaluate' to catch termination, for tests.
Assignee: general → jimb
Status: NEW → ASSIGNED
Attachment #663883 - Flags: review?(jorendorff)
(Assignee)

Comment 7

5 years ago
Created attachment 663884 [details] [diff] [review]
Implement Debugger.prototype.onNewGlobalObject.
Attachment #663884 - Flags: review?(jorendorff)
(Assignee)

Comment 8

5 years ago
Comments on design are always welcome as well.
(Assignee)

Comment 9

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

Upcoming.
(Assignee)

Comment 10

5 years ago
Try push for those first two patches: https://tbpl.mozilla.org/?tree=Try&rev=b54b80a45662
(Reporter)

Comment 11

5 years ago
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.
Attachment #663883 - Flags: review?(jorendorff) → review+
(Reporter)

Comment 12

5 years ago
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).
Attachment #663884 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 13

5 years ago
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!
(Assignee)

Updated

5 years ago
Depends on: 794726
(Assignee)

Comment 14

5 years ago
Created attachment 665519 [details] [diff] [review]
[revised per comments] Implement Debugger.prototype.onNewGlobalObject.

I think this addresses the comments.
(Assignee)

Comment 15

5 years ago
Fresh try:
https://tbpl.mozilla.org/?tree=Try&rev=479b37b2134c
(Assignee)

Comment 16

5 years ago
Created attachment 665536 [details] [diff] [review]
Implement Debugger.prototype.findAllGlobals.
Attachment #665536 - Flags: review?(jorendorff)
(Assignee)

Comment 17

5 years ago
Try for findAllGlobals:
https://tbpl.mozilla.org/?tree=Try&rev=40b0519addac
Blocks: 740551
(Assignee)

Comment 18

5 years ago
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.
Depends on: 795119
(Reporter)

Comment 19

5 years ago
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.
Attachment #665536 - Flags: review?(jorendorff) → review+
(Reporter)

Comment 20

5 years ago
(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.
(Assignee)

Comment 21

5 years ago
(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.
(Assignee)

Comment 22

5 years ago
(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.
(Assignee)

Comment 23

5 years ago
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.
Depends on: 799272
No longer depends on: 795119, 794726
(Assignee)

Comment 24

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

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

Comment 25

5 years ago
(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()'.
(Assignee)

Comment 26

5 years ago
I'm going to file a separate bug for hostAnnotations.
(Assignee)

Comment 27

5 years ago
(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...
(Assignee)

Comment 28

5 years ago
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
Flags: in-testsuite+
Target Milestone: --- → mozilla19
https://hg.mozilla.org/mozilla-central/rev/10f0632888bb
https://hg.mozilla.org/mozilla-central/rev/e44a8b7579fb
https://hg.mozilla.org/mozilla-central/rev/801ed9c31fd5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Duplicate of this bug: 740547
You need to log in before you can comment on or make changes to this bug.