Closed Bug 1061909 Opened 7 years ago Closed 6 years ago

[jsdbg2] Debugger.Memory.prototype.takeCensus should be able to categorize by allocation stack

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: fitzgen, Assigned: jimb)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 5 obsolete files)

5.13 KB, patch
fitzgen
: review+
fitzgen
: feedback+
Details | Diff | Splinter Review
10.96 KB, patch
Details | Diff | Splinter Review
4.82 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
55.49 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
6.18 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
11.75 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
5.81 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
We should be able to count+categorize things on the heap by allocation site.

Debugger.Memory.prototype.takeCensus will need to take an object of the form { byClass, byAllocationSite } where both properties are bools and you must choose one or the other (for now).
Assignee: nobody → jimb
OS: Mac OS X → All
Hardware: x86 → All
Summary: [jsdbg2] Create an allocation site assorter → [jsdbg2] Debugger.Memory.prototype.takeCensus should be able to categorize by allocation stack
Attachment #8612595 - Flags: feedback?(nfitzgerald)
So, this works pretty nicely! No tests yet; that's the next step.

$ cat ~/moz/allocStack.js
var g = newGlobal();
var dbg = new Debugger(g);

dbg.memory.trackingAllocationSites = true;

g.eval(`function list(n) {
  var l;
  while (n--)
    l = { link: l }
  return l;
}

var l = list(21);
`);

dbg.memory.takeCensus({ breakdown: { by: "allocationStack",
                                     then: { by: "objectClass" }}})
  .forEach((v, k) => {
    print("" + k.toString())
    print(JSON.stringify(v, null, 2));
    print();
});
$ obj~/js/src/js -f ~/moz/allocStack.js
list@/home/jimb/moz/allocStack.js line 6 > eval:4:5
@/home/jimb/moz/allocStack.js line 6 > eval:8:9
@/home/jimb/moz/allocStack.js:6:1

{
  "Object": {
    "count": 22
  }
}

@/home/jimb/moz/allocStack.js:6:1

{
  "ScriptSource": {
    "count": 1
  },
  "StaticEval": {
    "count": 1
  },
  "Function": {
    "count": 1
  }
}

@/home/jimb/moz/allocStack.js line 6 > eval:8:9
@/home/jimb/moz/allocStack.js:6:1

{
  "Object": {
    "count": 1
  }
}

noStack
{
  "count": 1693
}

$
Comment on attachment 8612593 [details] [diff] [review]
Add documentation for Debugger.Memory.prototype.takeCensus's 'breakdown' parameter.

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

This is great! Very detailed!

My only concern is that some people's eyes might glaze over at this much text without some simple examples. I think the usability factor of these docs would greatly increase if we had a series of example breakdowns (and an example of the output it would generate) starting with just counting, moving to combining two simple breakdowns, and then finally a complex example with many breakdowns every which way.

::: js/src/doc/Debugger/Debugger.Memory.md
@@ +280,5 @@
> +        Further categorize all the items allocated at each distinct stack using
> +        <i>breakdown</i>.
> +
> +        In the result of the census, this breakdown produces a JavaScript `Map`
> +        value whose keys are `SavedFrame` values, and whose values are whatever

Hrm, so are we going to need a way to create arbitrary SavedFrame instances with heap snapshots?

@@ +335,5 @@
> +        where each <i>result</i> is a value of whatever sort the corresponding
> +        breakdown value produces. All breakdown values are optional, and default
> +        to `{ type: "count" }`.
> +
> +    <code>{ by: "internalTypeName", then:<i>breakdown</i> }</code>

Can you give some kind of example here? Is this C++ class name? JSGCTraceKind? I think just having examples would help.
Attachment #8612593 - Flags: feedback?(nfitzgerald) → feedback+
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #6)
> My only concern is that some people's eyes might glaze over at this much
> text without some simple examples. I think the usability factor of these
> docs would greatly increase if we had a series of example breakdowns (and an
> example of the output it would generate) starting with just counting, moving
> to combining two simple breakdowns, and then finally a complex example with
> many breakdowns every which way.

Okay. I've taken a shot at this. Will re-post when done.


> ::: js/src/doc/Debugger/Debugger.Memory.md
> @@ +280,5 @@
> > +        Further categorize all the items allocated at each distinct stack using
> > +        <i>breakdown</i>.
> > +
> > +        In the result of the census, this breakdown produces a JavaScript `Map`
> > +        value whose keys are `SavedFrame` values, and whose values are whatever
> 
> Hrm, so are we going to need a way to create arbitrary SavedFrame instances
> with heap snapshots?

Yes. I think we should have a ubi::Node specialization for SavedFrame with accessors. This might be a chance to explore the possibility of having subclasses of Node and Base for such type-specific accessors.



> 
> @@ +335,5 @@
> > +        where each <i>result</i> is a value of whatever sort the corresponding
> > +        breakdown value produces. All breakdown values are optional, and default
> > +        to `{ type: "count" }`.
> > +
> > +    <code>{ by: "internalTypeName", then:<i>breakdown</i> }</code>
> 
> Can you give some kind of example here? Is this C++ class name?
> JSGCTraceKind? I think just having examples would help.
Attachment #8613667 - Flags: feedback?(nfitzgerald)
(In reply to Jim Blandy :jimb from comment #7)
> > ::: js/src/doc/Debugger/Debugger.Memory.md
> > @@ +280,5 @@
> > > +        Further categorize all the items allocated at each distinct stack using
> > > +        <i>breakdown</i>.
> > > +
> > > +        In the result of the census, this breakdown produces a JavaScript `Map`
> > > +        value whose keys are `SavedFrame` values, and whose values are whatever
> > 
> > Hrm, so are we going to need a way to create arbitrary SavedFrame instances
> > with heap snapshots?
> 
> Yes. I think we should have a ubi::Node specialization for SavedFrame with
> accessors. This might be a chance to explore the possibility of having
> subclasses of Node and Base for such type-specific accessors.

Yeah, then we could move objectClassName and objectConstructorName. Not sure how this would fit with the protobuf message format in heap snapshots.

Can you file the appropriate bug(s) and block bug 1139476 when appropriate?

To make sure we are on the same page: we wouldn't actually recreate SavedFrame objects but just use the ubi::Node subclass representing the idea of a stack that is backed by deserialized protobuf messages rather than a SavedFrame object. Correct?

> > @@ +335,5 @@
> > > +        where each <i>result</i> is a value of whatever sort the corresponding
> > > +        breakdown value produces. All breakdown values are optional, and default
> > > +        to `{ type: "count" }`.
> > > +
> > > +    <code>{ by: "internalTypeName", then:<i>breakdown</i> }</code>
> > 
> > Can you give some kind of example here? Is this C++ class name?
> > JSGCTraceKind? I think just having examples would help.

(Did you intend to leave a comment here?)
Attachment #8613671 - Flags: feedback?(nfitzgerald)
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #9)
> Can you file the appropriate bug(s) and block bug 1139476 when appropriate?

Roger.

> To make sure we are on the same page: we wouldn't actually recreate
> SavedFrame objects but just use the ubi::Node subclass representing the idea
> of a stack that is backed by deserialized protobuf messages rather than a
> SavedFrame object. Correct?

Precixactly.

> (Did you intend to leave a comment here?)

Hurr. No.
Comment on attachment 8613671 [details] [diff] [review]
Revise per Fitzgen's feedback.

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

This is _great_! The examples are very nice! Thanks!

::: js/src/doc/Debugger/Debugger.Memory.md
@@ +311,5 @@
>          <i>count</i> and <i>bytes</i> properties on the breakdown.
>  
> +        Note that the census can produce byte sizes only for the most common
> +        types. When the census cannot find the byte size for a given type, it
> +        returns zero.

Is zero better than one? There are no zero-sized types in C++, so everything in Gecko/SM will have a size of at least one byte.
Attachment #8613671 - Flags: review+
Attachment #8613671 - Flags: feedback?(nfitzgerald)
Attachment #8613671 - Flags: feedback+
Comment on attachment 8612594 [details] [diff] [review]
Add breakdown argument to Debugger.Memory.prototype.takeCensus, covering all existing count types.

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

Yay! Comments below are pretty much all nits.

::: js/src/vm/DebuggerMemory.cpp
@@ +399,5 @@
> +//    the heap, counting each node we find, expanding our tree of counts as we
> +//    go.
> +//
> +// 3) We walk the tree of counts and produce JavaScript objects reporting the
> +//    accumulated results.

Maybe mention what implementation methods match these phases? Eg, CountType::report overrides for #3, etc.

@@ +500,5 @@
> +    if (!ptr)
> +        return;
> +
> +    // Downcast to our true type and destruct, as guided by our CountType
> +    // pointer.

I don't think this comment is needed, since it's basically a dupe of the one above `destruct`.

@@ +594,5 @@
> +        CountBasePtr stringsCount(strings->makeCount());
> +        CountBasePtr otherCount(other->makeCount());
> +
> +        if (!objectsCount || !scriptsCount || !stringsCount || !otherCount)
> +            return CountBasePtr(nullptr);

Does it really have to call the ctor here? I was under the impression that `return nullptr;` would implicitly construct a UniquePtr. Is the typedef messing this up?

Not that this matters that much, I'm just surprised...

@@ +1103,5 @@
>  
> +    CountBasePtr rootCount(rootType->makeCount());
> +    if (!rootCount)
> +        return false;
> +    dbg::CensusHandler handler(census, rootCount);

After re-reading a bunch of code to verify that this line shouldn't actually be `Move(rootCount)`, I've come to appreciate Rust's explicit reference passing (for which I didn't really feel passionately about either way before).

It looks like nothing else uses the `rootCount` below this, it is just used in the handler via this reference in the `report` method. Given that, it seems like changing the handler to take ownership of the ptr (by using && and Move) would make things line up better with what's "really" going on, as well as prevent anyone else from mutating it while the handler expects to be the sole mutator.

::: js/src/vm/Xdr.h
@@ +28,5 @@
>   * this wiki page:
>   *
>   *  https://developer.mozilla.org/en-US/docs/SpiderMonkey/Internals/Bytecode
>   */
> +static const uint32_t XDR_BYTECODE_VERSION_SUBTRAHEND = 286;

No patch is considered Fun without incrementing the subtrahend!
Attachment #8612594 - Flags: feedback?(nfitzgerald) → feedback+
Comment on attachment 8612595 [details] [diff] [review]
Arrange for trees of census counts to be traced by the GC.

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

And this needs to happen because `report` can gc?
Attachment #8612595 - Flags: feedback?(nfitzgerald) → feedback+
Comment on attachment 8612596 [details] [diff] [review]
Implement 'allocationStack' breakdown for Debugger.Memory.prototype.takeCensus.

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

So straightforward! Great :D

::: js/src/vm/DebuggerMemory.cpp
@@ +1052,5 @@
> +        // Now build the result by iterating over the sorted vector.
> +        Rooted<MapObject*> map(cx, MapObject::create(cx));
> +        if (!map)
> +            return false;
> +        for (Entry** entryPtr = entries.begin(); entryPtr < entries.end(); entryPtr++) {

Nitpick: I think VectorBase::Range / VectorBase::ConstRange are preferred to pointer arithmetic. If not, maybe you can tell me when one would prefer one or the other. Or maybe it doesn't really matter.
Attachment #8612596 - Flags: feedback?(nfitzgerald) → feedback+
Attachment #8613667 - Flags: feedback?(nfitzgerald) → feedback+
Blocks: 1060567
Comment on attachment 8613667 [details] [diff] [review]
Debugger.Memory.prototype.takeCensus: provide byte counts on request.

This belongs on bug 1060567. Moved, and marked that bug as blocked on this one.
Attachment #8613667 - Attachment is obsolete: true
No longer blocks: 1060567
Depends on: 1060567
Blocks: 1060567
No longer depends on: 1060567
Revised for minor changes to takeCensus API.
Attachment #8612593 - Attachment is obsolete: true
Attachment #8612595 - Attachment is obsolete: true
Attachment #8627087 - Flags: review?(nfitzgerald)
Attachment #8612596 - Attachment is obsolete: true
Attachment #8627088 - Flags: review?(nfitzgerald)
Comment on attachment 8627085 [details] [diff] [review]
Define a testing function to introduce easily traceable objects.

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

Yes!

::: js/src/builtin/TestingFunctions.cpp
@@ +2921,5 @@
> +"allocationMarker()",
> +"  Return a freshly allocated object whose [[Class]] name is\n"
> +"  \"AllocationMarker\". Such objects are allocated only by calls\n"
> +"  to this function, never implicitly by the system, making them\n"
> +"  suitable for use in allocation tooling tests.\n"),

This will be helpful! I've been doing the ugliest hack arounds...
Attachment #8627085 - Flags: review?(nfitzgerald) → review+
Comment on attachment 8627086 [details] [diff] [review]
Add breakdown argument to Debugger.Memory.prototype.takeCensus, covering all existing count types.

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

::: js/src/jit-test/tests/debug/Memory-takeCensus-07.js
@@ +7,5 @@
> +var dbg = new Debugger(g);
> +
> +assertThrowsValue(() => {
> +  dbg.memory.takeCensus({
> +    breakdown: { get by() { throw "exception value" } }

If you're trying to make sure that you get your exception value, it might make sense to do

  const exceptionValue = Symbol("exception value");

at the top, and then throw exceptionValue in all of these getters.

It tightens up the test a little, but in practice I doubt that any random API will ever happen to throw the string "exception value". I leave it up to you to decide whether this is worth the change or not.

::: js/src/jit-test/tests/debug/Memory-takeCensus-08.js
@@ +39,5 @@
> +       shape.factor = 3;
> +       `);
> +
> +let baseline = 0;
> +function countIncreasedByAtLeast(n) {

For future debuggability, and because assertion failures don't print a full stack (which sucks), it probably makes sense to take a "test name" parameter here and print it out before asserting. Or just print once before each call to countIncreasedByAtLeast below.

@@ +71,5 @@
> +
> +g.add(1000, g.shape);
> +assertEq(countIncreasedByAtLeast(g.shape.factor * 1000), true);
> +
> +// print(uneval(dbg.memory.takeCensus({ breakdown: { by: 'internalTypeName' } })));

?

::: js/src/tests/js1_8_5/reflect-parse/Match.js
@@ +42,5 @@
>      Pattern.ANY = new Pattern;
>      Pattern.ANY.template = Pattern.ANY;
>  
> +    Pattern.NUMBER = new Pattern;
> +    Pattern.NUMBER.match = function (act) { typeof act === "number" };

Are we consciously considering NaN and +/- Infinity numbers? That's ok, but seems like something we might want to have a more restrictive Pattern.IS_REALLY_A_NUMBER_ACTUALLY for when we don't want to match those values (especially since we shouldn't get those values in census reports!)
Attachment #8627086 - Flags: review?(nfitzgerald) → review+
Comment on attachment 8627087 [details] [diff] [review]
Arrange for trees of census counts to be traced by the GC.

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

::: js/src/vm/DebuggerMemory.cpp
@@ +557,5 @@
>      CountBasePtr makeCount() override {
>          return CountBasePtr(census.new_<Count>(*this));
>      }
>  
> +    void traceCount(CountBase& countBase, JSTracer* trc) override { }

AFAICT, none of these CountTypes subclasses are intended to be further subclassed, right? Not sure if it is worth it, but I believe some compilers (gcc at least) can do additional optimizations if you add "final" to overridden virtual methods (or final subclasses) and remove the vtable lookup in some cases.
Attachment #8627087 - Flags: review?(nfitzgerald) → review+
Attachment #8627088 - Flags: review?(nfitzgerald) → review+
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #24)
> Comment on attachment 8627086 [details] [diff] [review]
> Add breakdown argument to Debugger.Memory.prototype.takeCensus, covering all
> existing count types.
> 
> Review of attachment 8627086 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit-test/tests/debug/Memory-takeCensus-07.js
> @@ +7,5 @@
> > +var dbg = new Debugger(g);
> > +
> > +assertThrowsValue(() => {
> > +  dbg.memory.takeCensus({
> > +    breakdown: { get by() { throw "exception value" } }
> 
> If you're trying to make sure that you get your exception value, it might
> make sense to do
> 
>   const exceptionValue = Symbol("exception value");
> 
> at the top, and then throw exceptionValue in all of these getters.
> 
> It tightens up the test a little, but in practice I doubt that any random
> API will ever happen to throw the string "exception value". I leave it up to
> you to decide whether this is worth the change or not.

I could use more distinctive text.


> ::: js/src/jit-test/tests/debug/Memory-takeCensus-08.js
> @@ +39,5 @@
> > +       shape.factor = 3;
> > +       `);
> > +
> > +let baseline = 0;
> > +function countIncreasedByAtLeast(n) {
> 
> For future debuggability, and because assertion failures don't print a full
> stack (which sucks), it probably makes sense to take a "test name" parameter
> here and print it out before asserting. Or just print once before each call
> to countIncreasedByAtLeast below.

Actually, I thought I did this right: countIncreasedByAtLeast returns a boolean value, and each call site asserts that it returned true. So failures should always be readily distinguished by the line number printed by the message. Isn't that what you want?


> @@ +71,5 @@
> > +
> > +g.add(1000, g.shape);
> > +assertEq(countIncreasedByAtLeast(g.shape.factor * 1000), true);
> > +
> > +// print(uneval(dbg.memory.takeCensus({ breakdown: { by: 'internalTypeName' } })));
> 
> ?

Oops. Had been using that for debugging. ("This failed? Why???")


> ::: js/src/tests/js1_8_5/reflect-parse/Match.js
> @@ +42,5 @@
> >      Pattern.ANY = new Pattern;
> >      Pattern.ANY.template = Pattern.ANY;
> >  
> > +    Pattern.NUMBER = new Pattern;
> > +    Pattern.NUMBER.match = function (act) { typeof act === "number" };
> 
> Are we consciously considering NaN and +/- Infinity numbers? That's ok, but
> seems like something we might want to have a more restrictive
> Pattern.IS_REALLY_A_NUMBER_ACTUALLY for when we don't want to match those
> values (especially since we shouldn't get those values in census reports!)

ARGH, my definition for Pattern.NUMBER.match is doubly wrong: it's missing a 'return', and patterns are executed for effect anyway: they're supposed to throw an exception. I've fixed this, added a Pattern.NATURAL, and made the test use that.
Even better try push (trying to keep this upbeat):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5361cd111ed
The stack-keyed hash table rekeying was wrong: you can't use e.front() after e.rekey(). New try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5361cd111ed
I have one remaining bug, caught by the CGC builds: js/src/jit-tests/tests/debug/Memory-takeCensus-09.js crashes when run with JS_GC_ZEAL=14,1. When building the report, it seems to be fetching a bad StackFrame object from the hash table.

#0  0x000000000044a928 in JSObject::compartment (this=(const JSObject * const) 0x7ffff7eaf180 [object J\ufffd\ufffd]) at /home/jimb/moz/dbg/js/src/shell/../jsobj.h:158
#1  0x00000000005cc058 in JSObject::global (this=(const JSObject * const) 0x7ffff7eaf180 [object J\ufffd\ufffd]) at /home/jimb/moz/dbg/js/src/jsobjinlines.h:376
#2  0x0000000000c78193 in JSCompartment::wrap (this=0x7ffff6159800, cx=0x7ffff611b330, obj=(JSObject *) 0x7ffff7eaf180 [object J\ufffd\ufffd], existingArg=0x0) at /home/jimb/moz/dbg/js/src/jscompartment.cpp:383
#3  0x000000000044d5ff in JSCompartment::wrap (this=0x7ffff6159800, cx=0x7ffff611b330, vp=$jsval((JSObject *) 0x7ffff7eaf180 [object J\ufffd\ufffd]), existing=0x0) at /home/jimb/moz/dbg/js/src/shell/../jscompartmentinlines.h:117
#4  0x0000000000725993 in js::dbg::ByAllocationStack::report (this=0x7fffed274280, countBase=..., report=JSVAL_VOID) at /home/jimb/moz/dbg/js/src/vm/DebuggerMemory.cpp:1117
#5  0x0000000000722819 in js::dbg::CountBase::report (this=0x7ffff61162e0, report=JSVAL_VOID) at /home/jimb/moz/dbg/js/src/vm/DebuggerMemory.cpp:498
#6  0x0000000000724441 in js::dbg::ByObjectClass::report (this=0x7fffed2742c0, countBase=..., report=$jsval((JSObject *) 0x7ffff7e7c100 [object Function "takeCensus"])) at /home/jimb/moz/dbg/js/src/vm/DebuggerMemory.cpp:863
#7  0x0000000000722819 in js::dbg::CountBase::report (this=0x7ffff6115e80, report=$jsval((JSObject *) 0x7ffff7e7c100 [object Function "takeCensus"])) at /home/jimb/moz/dbg/js/src/vm/DebuggerMemory.cpp:498
#8  0x0000000000725db0 in js::dbg::CensusHandler::report (this=0x7fffffffb5c0, report=$jsval((JSObject *) 0x7ffff7e7c100 [object Function "takeCensus"])) at /home/jimb/moz/dbg/js/src/vm/DebuggerMemory.cpp:1157
#9  0x00000000006d7d2e in js::DebuggerMemory::takeCensus (cx=0x7ffff611b330, argc=1, vp=0x7fffed1100b8) at /home/jimb/moz/dbg/js/src/vm/DebuggerMemory.cpp:1435
I can't believe the "I'm just gonna stare at this code" method actually worked.

The ByAllocationStack::report traversal is bogus. We create a bunch of pointers into the hash table, and then do allocations which can cause GC which can cause the hash table to be traced which can cause rehashes which invalidate the pointers into the hash table.
Here's the fix. It's kind of weird, so I wanted feedback. Big comment in the code.

The alternative is to have ByAllocationStack::report simply copy the entire hash table into some kind of vector. It is pretty weird to not re-key the table. But when you can fix a bug by *removing* complex code, it seems like one ought to at least try it out.
Attachment #8628279 - Flags: review?(nfitzgerald)
Huh, unused variables are now fatal build errors. I guess that's good. New try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ca098551406
(In reply to Jim Blandy :jimb from comment #31)
> I can't believe the "I'm just gonna stare at this code" method actually
> worked.
> 
> The ByAllocationStack::report traversal is bogus. We create a bunch of
> pointers into the hash table, and then do allocations which can cause GC
> which can cause the hash table to be traced which can cause rehashes which
> invalidate the pointers into the hash table.

Did the hazard analysis not warn about this? That certainly seems to fall well within its responsibilities. If it didn't, then we should probably ping sfink and see if this is a known issue or on his radar at all.
Comment on attachment 8628279 [details] [diff] [review]
Fix Debugger.Memory.prototype.takeCensus handling of by: allocationStack reports.

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

Nice comment, that explains what's going on quite well.
Attachment #8628279 - Flags: review?(nfitzgerald) → review+
The last commit here was missing an 'override' annotation on two count() implementations (in subclasses of CountType). The missing 'override' causes clang 3.6 & newer to spam a "-Winconsistent-missing-override" build warning, which busts --enable-warnings-as-errors builds (with clang 3.6+).

I'm pushing a followup (posted here via pulsebot shortly) to add that keyword, with blanket r+ that ehsan
granted me for fixes of this sort over in bug 1126447 comment 2.
You need to log in before you can comment on or make changes to this bug.