993085 - Add |Debugger.Memory.prototype.trackAllocationSites| getter/setter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Once bug 961288 lands, we will have an easy way to track all new allocation sites and we should expose this through the Debugger.Memory API so that it is easy to toggle it on/off for all your debuggees.

It should initially be false, and if you set it to true, then we would start tracking allocation sites.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: allocation-sites.patch

This should hopefully be a quick and easy review :)

https://tbpl.mozilla.org/?tree=Try&rev=eb5510a46e1f

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: debugger-memory-allocation-sites-getter-setter.patch

This should (hopefully) fix those try failures.

https://tbpl.mozilla.org/?tree=Try&rev=4091b448ed77

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: debugger-memory-allocation-sites-getter-setter.patch

Sigh... Debugging compiler errors via try push is sad. Not sure why clang doesn't complain.

Anyways, this time we really should have a green push.

https://tbpl.mozilla.org/?tree=Try&rev=6347031feca3

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Down to one compilation error on try... Woo?

https://tbpl.mozilla.org/php/getParsedLog.php?id=39826629&tree=Try#error0

I don't understand why it says that js::Debugger::fromJSObject(JSObject *) is undefined, since it clearly is in Debugger.h, and I include that header in DebuggerMemory.cpp. Help?

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: debugger-memory-allocation-sites-getter-setter.patch

Thanks to shu for pointing out that I needed to move Debugger::fromJSObject into Debugger-inl.h.

Fingers crossed for this push: https://tbpl.mozilla.org/?tree=Try&rev=58b30eaabb73

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Screen shot of profile

Modified https://github.com/jimblandy/benchmark-debugger to include tracking allocations sites and here are the results:

--ion,    debugger, trackingAllocationSites
warmup
measure
[Stats total: 1656.0305524902335s, mean: 3.312061104980467s, stddev: 5%]

--no-ion, debugger, trackingAllocationSites
warmup
measure
[Stats total: 1629.0351877441415s, mean: 3.258070375488283s, stddev: 6%]

--no-ion, debugger, onEnterFrame
warmup
measure
[Stats total: 229.4468962402345s, mean: 0.458893792480469s, stddev: 4%]

--ion,    debugger, onEnterFrame
warmup
measure
[Stats total: 231.84370166015609s, mean: 0.46368740332031216s, stddev: 5%]

--ion,    debugger, no hooks
warmup
measure
[Stats total: 108.59888964843753s, mean: 0.21719777929687506s, stddev: 7%]

--no-ion, debugger, no hooks
warmup
measure
[Stats total: 113.08044140624993s, mean: 0.22616088281249985s, stddev: 9%]

--ion
warmup
measure
[Stats total: 12.108040771484378s, mean: 0.024216081542968754s, stddev: 8%]

--no-ion
warmup
measure
[Stats total: 57.041055908203106s, mean: 0.1140821118164062s, stddev: 4%]

-------------------------------------------------------------------------

Way slower than I'd like!

Attached is a screenshot of profiling.

Things to investigate:

* Suspect we might be saving stacks for each SavedFrame object in the stacks we are saving!

* Determine how much slowdown we have just from enabling an empty metadata callback and not saving stacks or anything.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Screen shot of profile

Now with --enable-profiling!

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: debugger-memory-allocation-sites-getter-setter.patch

Try push that should fix the header include ordering and the s/Debugger/js::Debugger/ issue: https://tbpl.mozilla.org/?tree=Try&rev=1121f258e4da

I think we can land this now and continue to work on performance issues in bug 1004110.

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Looks like the try is green :)

Will add a second patch with documentation when I get a chance.

Comment 10

[Jim Blandy :jimb]

Comment on attachment 8425771 [details] [diff] [review]
debugger-memory-allocation-sites-getter-setter.patch

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

We should test that multiple Debuggers behave reasonably. Since we're not keeping a per-compartment count of how many Debuggers have requested allocation tracking, we need to test that attempts to request allocation tracking from multiple debuggers throws.

We should test that removeDebuggee clears the allocation tracking hook, if one was set.

We should test that addDebuggee sets the alloction tracking hook, if one is needed.

We should test that if a Debugger is unable to set the hook on *all* its debuggees, then assigning a true value to .trackingAllocationSites has no effect.

The docs need to be part of the patch. Be sure they document exceptions, too; I would hate to see JSMSG_OBJECT_METADATA_CALLBACK_ALREADY_SET appearing before users because we forgot to check for it in the server...

::: js/src/jit-test/tests/debug/Memory-trackingAllocationSites-02.js
@@ +1,1 @@
> +// Test that we don't get allocation sites when we aren't tracking them.

I think it's definitely worth verifying that we don't track allocation sites when nobody has asked for them, but keep in mind that, in the presence of multiple Debuggers for a single compartment, you may get metadata even when *you* didn't ask for it, if the other Debugger did.

I guess all I'm saying is, the comment should say:

// Test that we don't get allocation sites when nobody has asked for them.

@@ +11,5 @@
> +root.eval("this.obj2 = {};");
> +
> +let wrappedObj = wrappedRoot.makeDebuggeeValue(root.obj);
> +let allocationSite = dbg.memory.getAllocationSiteOf(wrappedObj);
> +assertEq(allocationSite, null);

Would it be possible for this test to allocate one object with metadata collection turned on, and the other with it turned off?

::: js/src/js.msg
@@ +438,5 @@
>  MSG_DEF(JSMSG_TERMINATED,               384, 1, JSEXN_ERR, "Script terminated by timeout at:\n{0}")
>  MSG_DEF(JSMSG_NO_SUCH_SELF_HOSTED_PROP, 385, 1, JSEXN_ERR, "No such property on self-hosted object: {0}")
>  MSG_DEF(JSMSG_PROXY_EXTENSIBILITY,      386, 0, JSEXN_TYPEERR, "proxy must report same extensiblitity as target")
>  MSG_DEF(JSMSG_PROXY_CONSTRUCT_OBJECT,   387, 0, JSEXN_TYPEERR, "proxy [[Construct]] must return an object")
> +MSG_DEF(JSMSG_OBJECT_METADATA_CALLBACK_ALREADY_SET, 388, 0, JSEXN_ERR, "Cannot set the object metadata callback when it is already set.")

The object metadata callback is an implementation detail; I think this message should be something like:

Cannot track object allocation, because other tools are already doing so

You should use one of the extant JSMSG_UNUSED spots.

::: js/src/vm/Debugger.cpp
@@ +79,5 @@
>      JSSLOT_DEBUGSOURCE_OWNER,
>      JSSLOT_DEBUGSOURCE_COUNT
>  };
>  /*** Utils ***************************************************************************************/
> +bool

Removing the "static" keyword just dumps the function into the global namespace, which is shared with the embedding application. SpiderMonkey can't do that.

You should put it in the js:: namespace. Or better yet, make it a member function of CallArgs, which is already in 'js':

if (!args.requireAtLeast(2))
    return false;

@@ +118,1 @@
>  ReportObjectRequired(JSContext *cx)

Similarly: this can't be global. This seems less appropriate for CallArgs; js::?

@@ +1963,5 @@
> +    memoryValue = ObjectValue(*memory);
> +    dbg->object->setReservedSlot(JSSLOT_DEBUG_MEMORY_INSTANCE, memoryValue);
> +
> +    memory->setReservedSlot(DebuggerMemory::JSSLOT_DEBUGGER, ObjectValue(*dbg->object));
> +    memory->setReservedSlot(DebuggerMemory::JSSLOT_TRACKING_ALLOCATION_SITES, JS::FalseValue());

Given that it's no longer trivial, could we abstract out the D.M instance allocation and initialization into a DebuggerMemory static member function? Say, DebuggerMemory::create?

If you agree it'd be clearer, this could also be:

Value memoryValue = ...

if (!memoryValue.isObject()) {
    ... actually create it
}

args.rval().set...
return true;

That is, it's possible to share the successful return tail, instead of duplicating it.

::: js/src/vm/DebuggerMemory.cpp
@@ +89,5 @@
> +DebuggerMemory::getDebugger(JSContext *cx)
> +{
> +    const Value dbgVal = getReservedSlot(JSSLOT_DEBUGGER);
> +    RootedObject dbgObj(cx, &dbgVal.toObject());
> +    return Debugger::fromJSObject(dbgObj);

I think you can simply say:

return Debugger::fromJSObject(&getReservedSlot(JSSLOT_DEBUGGER).toObject());

@@ +119,5 @@
> +            if (compartment->hasObjectMetadataCallback()) {
> +                JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr,
> +                                     JSMSG_OBJECT_METADATA_CALLBACK_ALREADY_SET);
> +                return false;
> +            }

This means the function can fail after changing some of the compartments' callbacks, but not others. To avoid chaos, I think we have to check them all first, and then set them once we know it will succeed.

@@ +140,5 @@
> +    return true;
> +}
> +
> +/* static */ bool
> +DebuggerMemory::getAllocationSiteOf(JSContext *cx, unsigned argc, Value *vp)

In the end, I really think this should be an accessor on Debugger.Object.prototype.

@@ +142,5 @@
> +
> +/* static */ bool
> +DebuggerMemory::getAllocationSiteOf(JSContext *cx, unsigned argc, Value *vp)
> +{
> +    THIS_DEBUGGER_MEMORY(cx, argc, vp, "(set trackingAllocationSites)", args, memory);

Wrong function name.

::: js/src/vm/DebuggerMemory.h
@@ +21,1 @@
>  public:

I think we indent visibility markers like "public:" two spaces.

::: js/src/vm/SavedStacks.cpp
@@ +21,5 @@
>  namespace js {
>  /* static */ HashNumber
>  SavedFrame::HashPolicy::hash(const Lookup &lookup)
>  {
> +    return AddToHash(JSAtomPtrHasher::hash(lookup.source),

It seems like you might as well skip JSAtomPtrHasher::hash here, and pass the JSAtom * in directly. That will get you the AddToHash(uint32_t hash, A* a) overload, I would expect. You won't get the '>> 3' behavior that PointerHasher::hash provides, but I think it's okay.

However we hash it, lookup.source and lookup.functionDisplayName should both be handled the same way.

@@ -45,5 @@
>      if (existing->getPrincipals() != lookup.principals)
>          return false;
>      JSAtom *source = existing->getSource();
> -    if (source->length() != lookup.source->length())
> -        return false;

Yes, definitely. (Did I miss this in review??)

@@ +352,5 @@
>  SavedStacks::saveCurrentStack(JSContext *cx, MutableHandle<SavedFrame*> frame)
>  {
>      JS_ASSERT(initialized());
>      JS_ASSERT(&cx->compartment()->savedStacks() == this);
> +    isSavingStack_ = true;

Could we put JS_ASSERT(!isSavingStack_) above this?

::: js/src/vm/SavedStacks.h
@@ +104,5 @@
>  {
>      typedef SavedFrame::Lookup               Lookup;
>      typedef PointerHasher<SavedFrame *, 3>   SavedFramePtrHasher;
>      typedef PointerHasher<JSPrincipals *, 3> JSPrincipalsPtrHasher;
> +    typedef PointerHasher<JSAtom *, 3>       JSAtomPtrHasher;

This could go if we just use AddToHash's pointer overload.

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

> Could we put JS_ASSERT(!isSavingStack_) above this?

No because someone else might legitimately want to save a stack that's unrelated to how the metadata callback hook doesn't want to save stacks for its own SavedFrame instances.

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: debugger-memory-allocation-sites-getter-setter.patch

Adds docs and updated based on feedback.

Only thing that isn't clear to me is how to link from docs in the Debugger/ directory to the new docs in the SavedFrame/ directory.

https://tbpl.mozilla.org/?tree=Try&rev=daa3197e2041

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: debugger-memory-allocation-sites-getter-setter.patch

This should hopefully fix the hazards, but I'm not 100% sure that its the correct solution. Would love some attention on the new cleanupDebuggeeGlobalBeforeRemoval.

https://tbpl.mozilla.org/?tree=Try&rev=c4949609b206

Comment 14

[Jim Blandy :jimb]

(In reply to Nick Fitzgerald [:fitzgen] from comment #12)
> Only thing that isn't clear to me is how to link from docs in the Debugger/
> directory to the new docs in the SavedFrame/ directory.

Each subdirectory of js/src/doc is supposed to know where it's going to be published. Inter-subdir references would just use 'absolute-label' (see js/src/doc/Debugger/config.sh for an example).

But you haven't written a config.sh file for that directory, so it doesn't have a home on MDN yet. Once you've settled that, then you can define an absolute label in js/src/doc/Debugger/config.sh for references from that directory.

Comment 15

[Jim Blandy :jimb]

Comment on attachment 8436163 [details] [diff] [review]
debugger-memory-allocation-sites-getter-setter.patch

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

Review still in progress; just some doc comments for now.

::: js/src/doc/Debugger/Debugger.Memory.md
@@ +1,3 @@
> +# `Debugger.Memory`
> +
> +## Accessor Properties of the `Debugger.Memory.prototype` Object

This isn't really okay. You've got to have a little introductory text here to orient people. (If you hate writing docs and you'd like me to do it, let me know; I enjoy it.)

Since Debugger.Memory isn't really the same kind of thing as the other Debugger.Bleah types, you don't have to say much:

"If <i>dbg</i> is a `Debugger` instance, then <i>dbg.memory</i> is an instance of `Debugger.Memory` whose methods and accessors operate on <i>dbg</i>. This class exists only to hold member functions and accessors related to memory analysis, keeping them separate from other `Debugger` facilities."

Or something better. But you have to say something so that people arriving at the page won't go "wut?".

@@ +4,5 @@
> +
> +`trackingAllocationSites`: A boolean value indicating whether this
> +`Debugger.Memory` instance is instrumenting Object allocations in the debuggee
> +compartments to capture the JavaScript execution stack at the time of
> +allocation. The allocation site for a given object can be retrieved with

I don't think you have to talk about X instrumenting Y to do Z. You can just say X is doing Z, leaving the instrumentation as an implementation detail:

"A boolean value indicating whether this `Debugger` instance is capturing the JavaScript execution stack when each Object is allocated."

@@ +5,5 @@
> +`trackingAllocationSites`: A boolean value indicating whether this
> +`Debugger.Memory` instance is instrumenting Object allocations in the debuggee
> +compartments to capture the JavaScript execution stack at the time of
> +allocation. The allocation site for a given object can be retrieved with
> +`Debugger.Memory.prototype.getAllocationSiteOf`. This accessor property has both

This is out of date; it's an accessor on Debugger.Object.prototype now, right?

@@ +7,5 @@
> +compartments to capture the JavaScript execution stack at the time of
> +allocation. The allocation site for a given object can be retrieved with
> +`Debugger.Memory.prototype.getAllocationSiteOf`. This accessor property has both
> +a getter and setter: assigning to it enables or disables the allocation
> +instrumentation; reading it produces `true` if the instrumentation is active,

Again, "instrumentation" is unnecessarily indirect. "... reading it produces `true` if we are capturing stacks for Object allocations."

@@ +11,5 @@
> +instrumentation; reading it produces `true` if the instrumentation is active,
> +and `false` otherwise. The default value is `false` in a freshly created
> +`Debugger.Memory` instance.
> +
> +Assignment is fallible, and if the Debugger is unable to install the allocation

"Assignment is fallible: if ..."

Specify which type of error is thrown.

::: js/src/doc/Debugger/Debugger.Object.md
@@ +209,5 @@
> +<!-- TODO bug 1019972: write SavedFrame documentation and link to it -->
> +
> +`allocationSite`
> +:   If `Debugger.Memory.prototype.trackingAllocationSites` was enabled when this
> +    [`Debugger.Object`][object] instance allocated, return the

... when this Debugger.Object's referent was allocated, ...

::: js/src/doc/Debugger/Debugger.md
@@ +249,5 @@
>      debuggee globals: if a debuggee global is not otherwise reachable, then
>      it is dropped from the `Debugger`'s set of debuggees. (Naturally, the
>      [`Debugger.Object`][object] instance this method returns does hold a strong
>      reference to the added global.)
> +    If there is a Debugger.Memory instance for this Debugger instance that is

For some reason, your patch is showing that there are no blank lines between the paragraphs in this definition, and none before your new paragraph. Did something go wrong? Aren't those blank lines significant in Markdown?

@@ +251,5 @@
>      [`Debugger.Object`][object] instance this method returns does hold a strong
>      reference to the added global.)
> +    If there is a Debugger.Memory instance for this Debugger instance that is
> +    [tracking allocation sites][tracking-allocs] and the debugger is unable to
> +    install the allocation hooks for <i>global</i>, an error is thrown.

We generally say what type of error is thrown: "a TypeError is thrown" or whatever.

::: js/src/doc/SavedFrame/SavedFrame.md
@@ +1,1 @@
> +# `SavedFrame`

There's no config.sh file for this directory. Where on MDN do you want this page to appear?

@@ +1,5 @@
> +# `SavedFrame`
> +
> +A `SavedFrame` instance is a singly linked list of stack frames. It represents a
> +JavaScript call stack at a past moment of execution. Younger frames hold
> +references to older frames. The older tails are shared across many younger

"... hold references to the frames that invoked them." Not just any random older frame.

@@ +15,5 @@
> +
> +`functionDisplayName`: Either SpiderMonkey's inferred name for this stack
> +frame's function, or `null`.
> +
> +`parent`: Either this stack frames parent stack frame (the next oldest frame),

"frame's"; "next older"

Comment 16

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

> For some reason, your patch is showing that there are no blank lines between the paragraphs in this
> definition, and none before your new paragraph. Did something go wrong? Aren't those blank lines
> significant in Markdown?

Strange... there are newlines in my source files. Will investigate.

Comment 17

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

For posterity, here is an email Jim sent me after an IRC convo where I had to run and he couldn't find the bug number.

-------------------------------------------------------------------------------------

From irc:
> fitzgen: One thought: we could simply *not* call ReleaseAllJITCode from
>        setObjectMetadataCallback *if* we're clearing the callback.
>        NewObjectMetadata doesn't call it if it's not set, so it's harmless to
>        have jitted code try to invoke it when it's not there.

Basically, it seems to me that if we're accidentally causing re-jittings from something that's just cleaning up a dead global, then something is trying too hard to be magic. For example, if turning on origin tracking causes a GC, then that seems reasonable and expected. But cleaning up Debugger state as globals get finalized should be able to do everything it needs to do without invoking such heavy machinery; it obviously has no real need to be involved in this operation at all.

Comment 18

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: debugger-memory-allocation-sites-getter-setter.patch

Fixed up the docs based on comments.

Removed the UnderGC variant of cleanupDebuggeeGlobalBeforeRemoval, instead I added JSCompartment::forgetObjectMetadataCallback() and call it from cleanupDebuggeeGlobalBeforeRemoval instead. Note that we can't just check if callback == nullptr in setObjectMetadataCallback because the hazard analysis hasn't solved the halting problem yet. Still waiting on that.

https://tbpl.mozilla.org/?tree=Try&rev=5e466e2d7172

Comment 20

[Jim Blandy :jimb]

Comment on attachment 8437201 [details] [diff] [review]
debugger-memory-allocation-sites-getter-setter.patch

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

This looks great. Just a few things to address.

Re: the docs: I've noted a few places where I noticed that your patch doesn't seem to show newlines that are present in my copy of the Markdown files. I don't know what the cause is here, but it would be inept for us to land a patch that destroys our paragraph structure, so we need to sort out the problem before we land.

By the way: I have a hack that makes Emacs's markdown mode understand how to fill definition list paragraphs.

Thanks very much for the requireAtLeast changes. Don't bother in this bug, but in the future, please break out large mechanical changes into their own patch. Then I can review that quickly - just pattern matching - without having to check to make sure each patch hunk doesn't include something more interesting. And then it's also easier to find the interesting parts in the remaining patch.

::: js/src/doc/Debugger/Debugger.Memory.md
@@ +7,5 @@
> +
> +## Accessor Properties of the `Debugger.Memory.prototype` Object
> +
> +`trackingAllocationSites`: A boolean value indicating whether this
> +`Debugger.Memory` instance is capturing the JavaScript execution stack when each

Please use the same definition list syntax as the other Debugger.Blah.md files.

@@ +8,5 @@
> +## Accessor Properties of the `Debugger.Memory.prototype` Object
> +
> +`trackingAllocationSites`: A boolean value indicating whether this
> +`Debugger.Memory` instance is capturing the JavaScript execution stack when each
> +Object is allocated. The allocation site for a given object can be retrieved

Avoid the passive voice. "You" is okay to use in technical documentation:

"You can retrieve a given object's allocation site with the `Debugger.Object.prototype.allocationSite' accessor property.

@@ +10,5 @@
> +`trackingAllocationSites`: A boolean value indicating whether this
> +`Debugger.Memory` instance is capturing the JavaScript execution stack when each
> +Object is allocated. The allocation site for a given object can be retrieved
> +with the `Debugger.Object.prototype.allocationSite` accessor property. This
> +accessor property has both a getter and setter: assigning to it enables or

Because you just mentioned D.O.p.allocationSite, "This accessor property"'s antecedent is now ambiguous: do you mean allocationSite or trackingAllocationSites? Sure, people will figure it out, but it's a needless pause. Instead, move the 'allocationSite' sentence to its own paragraph, immediately following this one.

The reference to allocationSite should be a link.

@@ +11,5 @@
> +`Debugger.Memory` instance is capturing the JavaScript execution stack when each
> +Object is allocated. The allocation site for a given object can be retrieved
> +with the `Debugger.Object.prototype.allocationSite` accessor property. This
> +accessor property has both a getter and setter: assigning to it enables or
> +disables the allocation instrumentation; reading it produces `true` if the

"and reading it".  Your semicolon here is a 'serial comma' style semicolon.

@@ +13,5 @@
> +with the `Debugger.Object.prototype.allocationSite` accessor property. This
> +accessor property has both a getter and setter: assigning to it enables or
> +disables the allocation instrumentation; reading it produces `true` if the
> +Debugger is capturing stacks for Object allocations, and `false` otherwise. The
> +default value is `false` in a freshly created `Debugger.Memory` instance.

Don't describe the default value of the accessor; instead, describe the default state of the system: "Allocation site tracking is initially disabled in a new Debugger."

@@ +16,5 @@
> +Debugger is capturing stacks for Object allocations, and `false` otherwise. The
> +default value is `false` in a freshly created `Debugger.Memory` instance.
> +
> +Assignment is fallible: if the Debugger is unable to install the allocation
> +hooks an `Error` instance is thrown.

Passive voice again: "it throws an `Error` instance." You've already got yourself a real subject, so you have something reasonable to attribute the action to.

::: js/src/doc/Debugger/Debugger.Object.md
@@ +205,5 @@
>      { "type":"jsm", "uri":"resource:://gre/modules/XPCOMUtils.jsm" }
>      ```
>      Firefox provides [DebuggerHostAnnotationsForFirefox annotations] for its
>      host objects.
> +`allocationSite`

Again, something is weird with the newlines in your patch.  My copy has three blank lines before the "##" header. (I understand that the recent changeset 741953db7a6d affects this area, but I think it's true even before then.)

@@ +206,5 @@
>      ```
>      Firefox provides [DebuggerHostAnnotationsForFirefox annotations] for its
>      host objects.
> +`allocationSite`
> +:   If `Debugger.Memory.prototype.trackingAllocationSites` was enabled when this

It would be nicer to say:

  If [object allocation site tracking][tracking-allocs] was enabled ...

::: js/src/doc/Debugger/Debugger.md
@@ +249,5 @@
>      debuggee globals: if a debuggee global is not otherwise reachable, then
>      it is dropped from the `Debugger`'s set of debuggees. (Naturally, the
>      [`Debugger.Object`][object] instance this method returns does hold a strong
>      reference to the added global.)
> +    If there is a [`Debugger.Memory`][memory] instance for this Debugger

There should be newlines separating these paragraphs, too.

@@ +252,5 @@
>      reference to the added global.)
> +    If there is a [`Debugger.Memory`][memory] instance for this Debugger
> +    instance that is [tracking allocation sites][tracking-allocs] and the
> +    debugger is unable to install the allocation hooks for <i>global</i>, an
> +    error is thrown.

passive voice

::: js/src/doc/Debugger/config.sh
@@ +34,5 @@
>  markdown Debugger.Source.md Debugger-API/Debugger.Source
>    label 'source'                                "Debugger.Source"
> +markdown Debugger.Memory.md Debugger-API/Debugger.Memory
> +  label 'memory'                                "Debugger.Memory"
> +  label 'tracking-allocs' '#trackingallocationsites' "Debugger.Memory: trackingAllocationSites"

See the docs for Debugger.Frame.prototype.eval for an example of how to place a link anchor on a particular property.

::: js/src/doc/SavedFrame/SavedFrame.md
@@ +6,5 @@
> +many younger frames.
> +
> +## Accessor Properties of the `SavedFrame.prototype` Object
> +
> +`source`: The source url for this stack frame, as a string.

Use the proper definition list syntax. URL should be capitalized, when it's the noun (but not when it's the Debugger.Script accessor).

::: js/src/jit-test/tests/debug/Memory-trackingAllocationSites-03.js
@@ +27,5 @@
> +
> +dbg1.removeDebuggee(root1);
> +root1.eval("this.obj = {}");
> +wrappedObj = d1r1.makeDebuggeeValue(root1.obj);
> +assertEq(wrappedObj.allocationSite, null);

It might be nice to just have a function:

function isTrackingAllocation(global, DO) {
  var site = DO.makeDebuggeeValue(global.eval('({})')).allocationSite;
  if (site) {
    assertEq(typeof allocationSite, 'object');
    assertEq(allocationSite === null, false);
  }
  return !!site;
}

and then just assertEq(isTrackingAllocation(root1, d1r1), false), etc. in the three sites.

@@ +58,5 @@
> +assertEq(typeof allocationSite == "object" && allocationSite != null, true);
> +
> +
> +// Setting trackingAllocationSites to true should throw if the debugger cannot
> +// install the allocation hooks for *every* debuggee.

Another test: ... and when it throws, its trackingAllocationSites accessor should reflect that allocation site tracking is still disabled in that Debugger.

::: js/src/jsapi.cpp
@@ +116,5 @@
> +
> +    if (length() < required) {
> +        char s[2];
> +        s[0] = '0' + (required - 1);
> +        s[1] = '\0';

While we're here, why not:

     char requiredStr[40];
     JS_snprintf(requiredStr, sizeof requiredStr, "%u", required);

::: js/src/vm/Debugger-inl.h
@@ +20,5 @@
>      if (!cx->compartment()->getDebuggees().empty() || evalTraps)
>          ok = slowPathOnLeaveFrame(cx, frame, ok);
>      return ok;
>  }
> +inline js::Debugger *

Leave a blank line between function bodies.

We mark static member function definitions with /* static */ before the return type.

::: js/src/vm/Debugger.cpp
@@ +1936,5 @@
>  bool
>  Debugger::getMemory(JSContext *cx, unsigned argc, Value *vp)
>  {
>      THIS_DEBUGGER(cx, argc, vp, "get memory", args, dbg);
> +    Value memoryValue = dbg->object->getReservedSlot(JSSLOT_DEBUG_MEMORY_INSTANCE);

The rooting hazard analysis doesn't complain about memoryValue being live across a call to DebuggerMemory::create, which can GC? How can it know that the slot's value isn't a string? Anyway, let's use RootedValue here, or just say:

  if (!dbg->object->getReservedSlot(JSSLOT_DEBUG_MEMORY_INSTANCE).isObject()) {
     ...
  }

  args.rval().set(dbg->object->getReservedSlot(JSSLOT_DEBUG_MEMORY_INSTANCE).isObject());

(in other words, eliminate memoryValue altogether).

@@ +2257,5 @@
> +     */
> +    const Value &memoryValue = this->object->getReservedSlot(JSSLOT_DEBUG_MEMORY_INSTANCE);
> +    if (memoryValue.isObject()) {
> +        DebuggerMemory &memory = memoryValue.toObject().as<DebuggerMemory>();
> +        if (memory.trackingAllocationSites()) {

The trackingAllocationSites flag should probably be stored in the js::Debugger instance. Then the getReservedSlot, isObject, as<DebuggerMemory>() can all go away. In DebuggerMemory.cpp, you usually have the js::Debugger ready at hand anyway: you just write getDebugger()->trackingAllocationSites().

I think the reason it's simpler is that, given a js::Debugger one doesn't always have a Debugger.Memory instance, but given a Debugger.Memory instance, one always has a js::Debugger.

@@ +2264,5 @@
> +                                     JSMSG_OBJECT_METADATA_CALLBACK_ALREADY_SET);
> +                return false;
> +            }
> +
> +            debuggeeCompartment->setObjectMetadataCallback(SavedStacksMetadataCallback);

What happens if the code below this fails? That only happens under low-memory conditions, but we'll leave those metadata callbacks set, with nobody watching, which seems like an unfriendly thing to do in low memory.

Note the unusual error handling down there: there's a single path for failure. It probably wouldn't be too much code to clear the metadata callbacks.

@@ +3700,2 @@
>      THIS_DEBUGSCRIPT_SCRIPT(cx, argc, vp, "setBreakpoint", args, obj, script);
> +    if (!args.requireAtLeast(cx, "Debugger.Script.setBreakpoint", 1))

requireAtLeast(..., 2)

@@ +5197,5 @@
> +    }
> +
> +    if (!cx->compartment()->wrap(cx, &metadata))
> +        return false;
> +    args.rval().setObject(*metadata);

The debuggee can access these SavedFrame objects too, right? Or, even if saveStack isn't visible to content now, we'd like to eventually make it so. That means that we're handing out, here, objects that the debuggee can munge and decorate and monkey-patch directly to debugger code.

Do we have a follow-up bug to make SavedFrame objects safe to share between content and chrome?

::: js/src/vm/Debugger.h
@@ +760,5 @@
>  extern bool
>  EvaluateInEnv(JSContext *cx, Handle<Env*> env, HandleValue thisv, AbstractFramePtr frame,
>                ConstTwoByteChars chars, unsigned length, const char *filename, unsigned lineno,
>                MutableHandleValue rval);
> +bool ReportObjectRequired(JSContext *cx);

Blank link between function declarations, too.

Comment 21

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

The missing newlines all exist if you look at the patch file. I don't know what happened, but splinter doesn't pick them up for my patches anymore, even though it used to...

*shrug*

Comment 22

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Jim Blandy :jimb from comment #20)
> @@ +5197,5 @@
> > +    }
> > +
> > +    if (!cx->compartment()->wrap(cx, &metadata))
> > +        return false;
> > +    args.rval().setObject(*metadata);
> 
> The debuggee can access these SavedFrame objects too, right? Or, even if
> saveStack isn't visible to content now, we'd like to eventually make it so.
> That means that we're handing out, here, objects that the debuggee can munge
> and decorate and monkey-patch directly to debugger code.
> 
> Do we have a follow-up bug to make SavedFrame objects safe to share between
> content and chrome?

No the debuggee can't access these objects, unless we explicitly handed one over. The only places it is being exposed is behind an accessor property that stringifies it first (DOM exceptions).

I don't think we'd ever want to expose these to content directly. That would require w3c specs, intent-to-implements, etc.

Comment 23

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: debugger-memory-allocation-sites-getter-setter.patch

Updated based on review comments.

New try push: https://tbpl.mozilla.org/?tree=Try&rev=da1ad262d120

Comment 24

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: debugger-memory-allocation-sites-getter-setter.patch

I messed up my rebase and and the max frames flag was being ignored.

New try push: https://tbpl.mozilla.org/?tree=Try&rev=aef55a40e2a1

Comment 25

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Just talked with Jim and he says we should do the equivalent of Object.freeze on SavedFrame instances and the SavedFrame.prototype for this to land.

Comment 26

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 0: Freeze SavedFrame objects and SavedFrame.prototype

Comment 27

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 0: Freeze SavedFrame objects and SavedFrame.prototype

Woops, should be consistent and use JSObject::freeze at both places.

Comment 28

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 0: Freeze SavedFrame and SavedFrame.prototype

Fix style. Sorry for bug spam. This is the last one.

Comment 29

[Jim Blandy :jimb]

Comment on attachment 8456297 [details] [diff] [review]
debugger-memory-allocation-sites-getter-setter.patch

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

Almost there!

I don't think the SavedStacks.{cpp,h} changes should be in this patch.

::: js/src/doc/Debugger/Debugger.Memory.md
@@ +10,5 @@
> +<code id="trackingallocationsites">trackingAllocationSites</code>
> +:   A boolean value indicating whether this `Debugger.Memory` instance is
> +    capturing the JavaScript execution stack when each Object is allocated. This
> +    accessor property has both a getter and setter: assigning to it enables or
> +    disables the allocation instrumentation, Reading the accessor produces

I think you meant: s/instrumentation,/instrumentation./  I still think we should just talk about whether we track allocation sites or not, and leave implementation techniques like "instrumentation" out of the discussion.

::: js/src/doc/Debugger/Debugger.Object.md
@@ +206,5 @@
> +    { "type":"jsm", "uri":"resource:://gre/modules/XPCOMUtils.jsm" }
> +    ```
> +
> +    Firefox provides [DebuggerHostAnnotationsForFirefox annotations] for its
> +    host objects.

How did the 'hostAnnotations' docs get back in here? This needs to come out... :D

@@ +210,5 @@
> +    host objects.
> +
> +<code id="allocationsite">allocationSite</code>
> +:   If [object allocation site tracking][tracking-allocs] was enabled when this
> +    [`Debugger.Object`][object]'s referent was allocated, return the

Uses of "Debugger.Object" don't need to be links in Debugger.Object.md itself.

::: js/src/doc/Debugger/Debugger.md
@@ +250,5 @@
>      [`Debugger.Object`][object] instance this method returns does hold a strong
>      reference to the added global.)
>  
> +    If there is a [`Debugger.Memory`][memory] instance for this Debugger
> +    instance that is [tracking allocation sites][tracking-allocs] and the

You can just say, "If this debugger is [tracking allocation sites][tracking-allocs]..."

::: js/src/jit-test/tests/debug/Memory-trackingAllocationSites-03.js
@@ +39,5 @@
> +
> +// Adding root back as a debuggee in dbg1 should fail now because it will
> +// attempt to track allocations in root, but dbg2 is already doing that.
> +assertThrowsInstanceOf(() => dbg1.addDebuggee(root1),
> +                       Error);

Let's do: assertEq(dbg1.hasDebuggee(root1), false);

@@ +43,5 @@
> +                       Error);
> +
> +// Adding a new debuggee to a debugger that is tracking allocations should
> +// enable the hook for the new debuggee.
> +dbg2.removeDebuggee(root1);

... and this is unnecessary, then, right?

::: js/src/jsapi.cpp
@@ +118,5 @@
>  
> +bool
> +JS::CallArgs::requireAtLeast(JSContext *cx, const char *fnname, unsigned required) {
> +    JS_ASSERT(required > 0);
> +    JS_ASSERT(required <= 10);

You can drop this assertion now; that was the point of the JS_snprintf.

::: js/src/vm/Debugger.cpp
@@ +2004,5 @@
>      THIS_DEBUGGER(cx, argc, vp, "get memory", args, dbg);
> +    RootedValue memoryValue(cx, dbg->object->getReservedSlot(JSSLOT_DEBUG_MEMORY_INSTANCE));
> +
> +    if (!memoryValue.isObject()) {
> +        RootedObject memory(cx, DebuggerMemory::create(cx, dbg));

My earlier comment about the rooting hazard here was clueless: if the 'then' branch is taken, then memoryValue is *not* live. Sorry about that.

@@ +5272,5 @@
> +    RootedObject metadata(cx, obj->getMetadata());
> +    if (!metadata) {
> +        args.rval().setNull();
> +        return true;
> +    }

For what it's worth, you can pass a nullptr to JSCompartment::wrap, so this whole 'if' could be dropped.

::: js/src/vm/Debugger.h
@@ +182,5 @@
>          JSSLOT_DEBUG_PROTO_STOP,
>          JSSLOT_DEBUG_HOOK_START = JSSLOT_DEBUG_PROTO_STOP,
>          JSSLOT_DEBUG_HOOK_STOP = JSSLOT_DEBUG_HOOK_START + HookCount,
>          JSSLOT_DEBUG_MEMORY_INSTANCE = JSSLOT_DEBUG_HOOK_STOP,
> +        JSSLOT_DEBUG_TRACKING_ALLOCATIONS_SITES,

In my original review, I wrote, "The trackingAllocationSites flag should probably be stored in the js::Debugger instance." That is, as a 'bool' member of the C++ type js::Debugger, alongside js::Debugger::enabled. The rest of my paragraph made that clear. There's no reason to go to the trouble of storing it as a JSObject slot in the Debugger instance.

::: js/src/vm/DebuggerMemory.cpp
@@ +124,5 @@
> +    Debugger *dbg = memory->getDebugger();
> +    bool enabling = ToBoolean(args[0]);
> +
> +    if (enabling == dbg->trackingAllocationSites())
> +        // Nothing to do here...

... except args.rval().setUndefined(). I think it's the callee, if you don't set it.

@@ +156,5 @@
> +/* static */ bool
> +DebuggerMemory::getTrackingAllocationSites(JSContext *cx, unsigned argc, Value *vp)
> +{
> +    THIS_DEBUGGER_MEMORY(cx, argc, vp, "(get trackingAllocationSites)", args, memory);
> +    args.rval().set(BooleanValue(memory->getDebugger()->trackingAllocationSites()));

You can say args.rval().setBoolean(...) here.

Comment 30

[Jim Blandy :jimb]

Comment on attachment 8458938 [details] [diff] [review]
Part 0: Freeze SavedFrame and SavedFrame.prototype

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

Looks good to me, if you've run the jit-tests.

Comment 31

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Add the Debugger.Memory.prototype.trackingAllocationSites accessor property

Comment 32

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: interdiff for part 1

To help your review, here are the changes in this new version of part 1.

Comment 33

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

New try push: https://tbpl.mozilla.org/?tree=Try&rev=aba8d326b16d

Comment 34

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Add the Debugger.Memory.prototype.trackingAllocationSites accessor property

Woops last patch had terminal color escape codes.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=2c4cd0050425

Comment 35

[Jim Blandy :jimb]

Comment on attachment 8459020 [details] [diff] [review]
interdiff for part 1

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

Looks great. Thanks for the revisions.

Comment 36

[Jim Blandy :jimb]

(Just putting the review+ on the full patch, not the interdiff...)

Comment 37

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: debugger-memory-allocation-sites-getter-setter.patch

Rebased and (hopefully) fixed the hazard from the last try push: https://tbpl.mozilla.org/?tree=Try&rev=dc80fce719cc

Comment 38

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

No more hazard!

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc6148c47021
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5bb52570f8f

Comment 40

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fc6148c47021
https://hg.mozilla.org/mozilla-central/rev/c5bb52570f8f