688891 - Change Sprinter to js::CStringBuilder backed by js::Vector storage

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Leary [:cdleary] (not checking bugmail)]

The only user of unaligned realloc-like LifoAlloc functionality (referenced in bug 684039) is the decompiler and its friends, Sprinter and JSPrinter. This part of the code does some pretty terrible things with ptrdiff offset return values and lack of error checking, so it generally needs an overhaul.

This is a simple cleanup bug that would let us get rid of the silly realloc stuff in the LifoAlloc code.

Comment 1

[Adam]

Attachment: sprinter rewrite

Comment 2

[Nicholas Nethercote [inactive]]

Wow!  Nice work, Adam.  I've assigned the bug to you :)

Comment 3

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 586847 [details] [diff] [review]
sprinter rewrite

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

Looks great! Just a few small comments in this feedback. Next step is to check over the patch, make sure it conforms to the C++ coding style guidelines ( https://wiki.mozilla.org/JavaScript:SpiderMonkey:C%2B%2B_Coding_Style ), and put it up for review!

We should also file a followup cleanup bug to get rid of the wrapping Sprinter functions and call the methods directly, now that we're going to have this nice new Sprinter interface.

::: js/src/jsopcode.cpp
@@ +765,5 @@
> +}
> +
> +Sprinter::Sprinter(JSContext *cx) : context(cx)
> +{
> +    base = (char *) js_malloc(default_size);

We usually use slightly more higher-level allocators that tell us about the memory reporting. I think cx->malloc/free/realloc are probably what we're going to want to use here, since they have memory accounting built in.

@@ +858,4 @@
>  }
>  
>  ptrdiff_t
>  SprintPut(Sprinter *sp, const char *s, size_t len)

I think it might be nice for the abstraction to put the meat of this function into a Sprinter method -- it plays pretty fast and loose with the internals / buffer management.

Comment 4

[Adam]

Attachment: sprinter rewrite #2

Comment 5

[Chris Leary [:cdleary] (not checking bugmail)]

Sorry Adam, not going to get to this today, but I'll review it for you tomorrow!

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 587470 [details] [diff] [review]
sprinter rewrite #2

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

Very nice, this is a great start! Rest assured that the rest of our code base is way nicer than this stuff -- this is just the area where the cleanups can really pay off. :-)

I have a lot of small nits and a few high-level points, but that's typical for a first patch, and especially common for this hairy part of the code base. I'm going to cancel review on this one and we can do another round of review once these comments are addressed.

Generally, this code scares the bejesus out of me in terms of invariants, so I'm asking for a lot more assertions / invariant checks for sanity checking purposes. This code is not hot-path code, so in this patch simple and correct will beat fancy / potentially incorrect hands down.

One overarching request is just for the variable names to go from things like deflated_chars => deflatedChars and old_offset => oldOffset. I didn't note all of those since it would be too noisy.

After we get this patch landed I think we should make a few follow up patches to prevent the clients from messing with the internals of the Sprinter (like by setting the offset or depending on the client to set the NULL byte) unnecessarily. That's for other bugs, though, since we try to do one logical patch per bug.

I'll be on IRC to chat more and clarify anything!

::: js/src/jsopcode.cpp
@@ +750,4 @@
>  
>  /************************************************************************/
>  
> +namespace js {

In .cpp files we use "using namespace js;" instead of a namespace scope, so please remove this. The only thing it bites you on are non-member functions in the js scope, where you have to say |js::TopLevelFunction() { ... }|.

@@ +753,5 @@
> +namespace js {
> +
> +const size_t Sprinter::DefaultSize = 64;
> +
> +bool Sprinter::realloc_(size_t sz)

Personal nit: s/sz/newSize :-) Should we also assert that newSize is >= offset, for sanity?

@@ +758,2 @@
>  {
> +    char *new_buf = (char *) context->realloc_(base, sz);

Style nit: s/new_buf/newBuf

@@ +765,5 @@
> +}
> +
> +Sprinter::Sprinter(JSContext *cx) : context(cx)
> +{
> +    base = (char *) context->malloc_(DefaultSize);

So this is subtly incorrect -- we only put infallible stuff into constructors and use |bool init()| methods for the fallible things (like allocating a buffer). The reason for this is that a |false| return value indicates that an exception value has been set on the |JSContext|, so we have to bubble the error upwards.

So I think you want to initialize base/size/offset to zero here, which we usually do in the initializer list, and then allocate the buffer / initialize the real size in a |bool init()| that you'll have to call after construction.

@@ +779,5 @@
> +
> +Sprinter::~Sprinter()
> +{
> +    if (base)
> +        context->free_(base);

I'm pretty sure free works on NULL values, so you don't need the conditional here.

@@ +782,5 @@
> +    if (base)
> +        context->free_(base);
> +}
> +
> +const char *Sprinter::string() const

Style nit: newline after the return type. Similar for the below methods.

const char *
Sprinter::string() const
{
    ...
}

@@ +795,5 @@
> +
> +char *Sprinter::string_at(ptrdiff_t off) const
> +{
> +    JS_ASSERT(off >= 0 && (size_t) off <= size);
> +    return off >= 0 && (size_t) off <= size ? base + off : base + size;

Assertions are fatal in the JS engine, so you shouldn't test at runtime -- the assertion must hold. You can just turn this into |return base + off;|

It's annoying that the client code requests things that are > base + offset at all, since that's not going to have a null terminating byte. Would it be hard to transform the code so that the assertion is |size_t(off) <= offset|? Not sure how much client code would have to change to make that work, but it'd be a much safer-feeling assertion. :-)

@@ +802,5 @@
> +char *Sprinter::reserve(size_t len)
> +{
> +    while (len + 1 > size - offset) /* Include trailing \0 */
> +        if (!realloc_(size * 2))
> +            return NULL;

Style nit: brackets on the while loop body, since it's multi-line. The if is okay without brackets, though, because it's only one line.

@@ +805,5 @@
> +        if (!realloc_(size * 2))
> +            return NULL;
> +
> +    char *sb = base + offset;
> +    offset += len;

Should we set the byte at offset to the NULL byte, just to be safe?

@@ +829,5 @@
> +        if (string() != old_base)
> +            s = string_at(s - old_base);  /* this is where it lives now */
> +        memmove(bp, s, len);
> +    } else {
> +        memcpy(bp, s, len);

Nice. Can we add an assertion that |s| doesn't fall within the (current) buffer in this block, just for sanity checking?

@@ +847,5 @@
> +        va_start(va, fmt);
> +        int i = vsnprintf(base + offset, size - offset, fmt, va);
> +        va_end(va);
> +
> +        if (i > -1 && (size_t) i < size - offset) {

We should be able to assert if |i >= 0| that |size_t(i) < size - offset|, right?

@@ +861,3 @@
>  {
> +    JS_ASSERT(end >= base && end <= base + size);
> +    if (end >= base && end <= base + size)

Assertions are fatal, so you can get rid of the conditional.

@@ +866,5 @@
> +
> +void Sprinter::set_offset(ptrdiff_t off)
> +{
> +    JS_ASSERT(off >= 0 && (size_t) off <= size);
> +    if (off >= 0 && (size_t) off <= size)

Ditto.

@@ +882,4 @@
>  }
>  
>  ptrdiff_t
>  SprintPut(Sprinter *sp, const char *s, size_t len)

We should remove uses of this in a future cleanup patch.

@@ -840,5 @@
>          return -1;
>  
> -    ptrdiff_t offset = sp->offset;
> -    sp->offset += size;
> -    DeflateStringToBuffer(sp->context, chars, length, sp->base + offset, &size);

I'm curious why you wanted to get rid of the print-directly-to-buffer code in favor of DeflateString. DeflateString creates an intermediate string on the heap, (which isn't so bad on this path, because it's fairly cold). Maybe you were trying to avoid playing with the internals of the sprinter, but I think we can take this as a method of the sprinter to make it less encapsulation-violating? Or, this works too, just wondering. :-)

@@ +911,5 @@
> +
> +    if (i == -1)
> +        return -1;
> +
> +    return old_offset;

Nit: return (i == -1) ? -1 : oldOffset;

@@ -913,5 @@
> -            return NULL;
> -
> -        /* Advance sp->offset and copy s into sp's buffer. */
> -        char *bp = sp->base + sp->offset;
> -        sp->offset += len;

Nice simplification here.

@@ -1060,5 @@
>  js_GetPrinterOutput(JSPrinter *jp)
>  {
>      JSContext *cx = jp->sprinter.context;
> -    if (!jp->sprinter.base)
> -        return cx->runtime->emptyString;

Can we preserve this conditional (empty string optimization may be useful), but turn it into a call to sprinter.empty(), which would tell us if the offset was zero?

@@ +1223,5 @@
>      uintN       inArrayInit;    /* array initialiser/comprehension level */
>      JSBool      inGenExp;       /* in generator expression */
>      JSPrinter   *printer;       /* permanent output goes here */
> +
> +    SprintStack(JSContext *cx) : sprinter(cx)

'explicit' keyword on the constructor, and can we use the initializer list for the NULLs/0s/false too?

@@ -1321,5 @@
>          }
> -        if (!ss->sprinter.base && SprintPut(&ss->sprinter, "", 0) >= 0) {
> -            memset(ss->sprinter.base, 0, ss->sprinter.offset);
> -            ss->offsets[i] = -1;
> -        }

This really scares me. Somehow this path expects to get a sprinter with no backing buffer but a non-zero offset. Let's throw a call to |ss->sprinter.checkInvariants()| here to make ourselves feel better.

@@ +1414,5 @@
>  static void
>  AddParenSlop(SprintStack *ss)
>  {
> +    char *sp = ss->sprinter.reserve(PAREN_SLOP);
> +    memset(sp, 0, PAREN_SLOP);

Let's make a reserveAndClear(size) helper method that does this two-hit combo for us.

@@ +1694,5 @@
>                                         (JSVAL_IS_STRING(key) ? '"' : 0));
>                      if (!rval)
>                          return JS_FALSE;
>                  }
> +                ss->sprinter.set_offset(rval);

Let's make a retract helper that asserts the offset you're setting is lower than the current offset -- that helps you reason about the behavior of the Sprinter client code.

@@ +1774,4 @@
>              if (!rval)
>                  return NULL;
>  
> +            ss->sprinter.set_offset(rval);

Ditto -- everywhere we used RETRACE let's try to use sprinter->retract(offset)

@@ +2125,4 @@
>  
>              /* Distinguish object from array by opcode or source note. */
>              if (sn && SN_TYPE(sn) == SRC_INITPROP) {
> +                *ss->sprinter.string_at(head) = '{';

Let's make an char &operator[](size_t) that we can use to bounds check these one-off character mutations -- maybe we can check that the NULL character isn't being mutated in that helper as well (unless you're mutating the character *to* null at an offset, in which case we can do a |setNull(size_t)|).

@@ +2146,4 @@
>            {
>              JSAtom *atom;
>              LOAD_ATOM(0);
> +            *ss->sprinter.string_at(head) = '{';

Ditto.

@@ +2179,5 @@
>          if (nameoff >= 0) {
>              ptrdiff_t offset, initlen;
>  
> +            offset = ss->sprinter.get_offset();
> +            LOCAL_ASSERT(*ss->sprinter.string_at(offset) == '\0');

Same thing for these NULL checks -- we can make a |const char &operator[] const| to check bounds. That one should be able to get at the NULL char as well, unlike the mutable case.

@@ +2198,4 @@
>                  if (!strncmp(name + namelen, ": ", 2) &&
>                      !strncmp(name, name + namelen + 2, namelen)) {
>                      offset -= namelen + 2;
> +                    *ss->sprinter.string_at(offset) = '\0';

And then all of these can change.

@@ -2275,4 @@
>  {
>      for (size_t i = 0; i < atoms.length(); i++) {
>          const char *name = QuoteString(&ss->sprinter, atoms[i], 0);
> -        if (!name || !PushOff(ss, STR2OFF(&ss->sprinter, name), JSOP_ENTERBLOCK))

Let's change STR2OFF to sprinter->offsetOf(const char *) so we can bounds check the string (in this case name) lives inside the sprinter buffer.

@@ +2910,5 @@
>                      if (!jp2)
>                          return NULL;
>                      ok = js_DecompileFunction(jp2);
> +                    if (ok && *jp2->sprinter.string())
> +                        js_puts(jp, jp2->sprinter.string());

Let's turn the |sprinter.base| checks into |!sprinter.empty()|.

::: js/src/jsopcode.h
@@ +459,4 @@
>  /*
>   * Sprintf, but with unlimited and automatically allocated buffering.
>   */
> +class Sprinter

One general request: since the client code is so hairy, I'd like to make a |void checkInvariants() const| method that checks the invariant properties of the sprinter data structure. Usually those go like this:

    void checkInvariants() const {
#ifdef DEBUG
        if (!base)
            return;

        JS_ASSERT(...);
        JS_ASSERT(...);
#endif
    }

Just to enforce all of the invariants that this data structure must maintain, or document what can't be enforced as an invariant and why. Then, once that's added, you have to call it from mutating operations. You can do that using an RAII guard (to check both before and after the methods executes) if you like.

@@ +465,5 @@
> +    JSContext               *context;       /* context executing the decompiler */
> +
> +  private:
> +    static const size_t     DefaultSize;
> +    char                    *base;          /* base address of buffer in pool */

Since the buffer is no longer in the pool, I guess this comment is something like "malloc'd buffer address" now.

@@ +467,5 @@
> +  private:
> +    static const size_t     DefaultSize;
> +    char                    *base;          /* base address of buffer in pool */
> +    size_t                  size;           /* size of buffer allocated at base */
> +    ptrdiff_t               offset;         /* offset of next free char in buffer */

Maybe we want a comment WRT the null termination invariant -- is offset always pointing at a NULL byte?

@@ +472,5 @@
> +
> +    bool realloc_(size_t sz);
> +
> +  public:
> +    Sprinter(JSContext *cx);

We typically put the |explicit| keyword on all 1-parameter constructors to avoid annoying mishaps down the road.

@@ +477,5 @@
> +    ~Sprinter();
> +
> +    const char *string() const;
> +    const char *string_end() const;
> +    char *string_at(ptrdiff_t off) const;

Style nit:
s/string_at/stringAt
s/string_end/stringEnd

Similar naming for the methods at the end of this class, please! :-)

If you prefer, you can also turn these small-and-simple methods into inline functions, like:

const char *string() const {
    return base;
}
const char *stringEnd() const {
    return base + offset;
}

We typically line up simple functions all in a row like that in new code that I've seen.

@@ +480,5 @@
> +    const char *string_end() const;
> +    char *string_at(ptrdiff_t off) const;
> +
> +    /*
> +     * Attempt to reserve len space in sp (including a trailing NULL byte). If the

While we're moving this comment, let's clear it up a little bit: is it that the value of |len| has to account for a trailing NULL byte or that |reserve| adds to the |len| value to account for the NULL byte? (It looks like the latter.)

@@ +487,5 @@
> +     * (including the space for the trailing NULL byte) on success.
> +     */
> +    char *reserve(size_t len);
> +
> +    ptrdiff_t put(const char *s, size_t len);

Can we make a comment as to what the return value is giving us? Something like "Put |len| characters of |s| at the current position and return ..."

At a glance it looks like the offset at which the string was placed (on success, >= 0) and -1 on failure?

@@ +495,5 @@
> +    void set_offset(const char *end);
> +    void set_offset(ptrdiff_t off);
> +
> +    ptrdiff_t get_offset() const;
> +    size_t get_size() const;

Personal nit: One-liner comments like "offset within the allocated buffer" and "size (in bytes) of the allocated buffer" are always appreciated. :-)

::: js/src/shell/js.cpp
@@ +1962,3 @@
>  Notes(JSContext *cx, uintN argc, jsval *vp)
>  {
>      LifoAllocScope las(&cx->tempLifoAlloc());

I think we can ditch the LifoAllocScope here now that you've removed the dependency on tempLifoAlloc.

@@ +2114,3 @@
>          return false;
>  
>      LifoAllocScope las(&cx->tempLifoAlloc());

Ditto, and for a few below here.

Comment 7

[Nicholas Nethercote [inactive]]

> If you prefer, you can also turn these small-and-simple methods into inline
> functions, like:
> 
> const char *string() const {
>     return base;
> }
> const char *stringEnd() const {
>     return base + offset;
> }
> 
> We typically line up simple functions all in a row like that in new code
> that I've seen.

There's precedent for putting such short functions on a single line, e.g.:

  const char *string() const { return base; }

Comment 8

[Adam]

Attachment: sprinter rewrite #3

Comment 9

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 588468 [details] [diff] [review]
sprinter rewrite #3

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

Great! I'm going to r+ this on the condition that these few nits are addressed and that we add the RAII invariant checker to the mutating operations that I mentioned in the last review / we chatted about on IRC. Adam, when you post the next patch I'll just glance that over and carry the r+.

Thanks for doing this! A difficult first bug, you'll be well prepared for bug #2. :-)

::: js/src/jsanalyze.cpp
@@ +57,4 @@
>  PrintBytecode(JSContext *cx, JSScript *script, jsbytecode *pc)
>  {
>      printf("#%u:", script->id());
> +    Sprinter sprinter(cx);

Missing initialization?

::: js/src/jsdbgapi.cpp
@@ +1630,4 @@
>  JS_DumpBytecode(JSContext *cx, JSScript *script)
>  {
>  #if defined(DEBUG)
> +    Sprinter sprinter(cx);

Missing initialization? The outer-most points should do the call to init() and the methods that take a sprinter, like js_Disassemble, should assume that their sprinter argument is initialized.

@@ +1645,4 @@
>  #if defined(DEBUG)
>      JS_ASSERT(script->pcCounters);
>  
> +    Sprinter sprinter(cx);

Ditto?

::: js/src/jsopcode.cpp
@@ +757,5 @@
>  /************************************************************************/
>  
> +const size_t js::Sprinter::DefaultSize = 64;
> +
> +bool js::Sprinter::realloc_(size_t newSize)

Style nit: newline after return type. Also, since we're |using namespace js;| there's no need for the |js::| prefix on Sprinter methods -- the compiler will automatically find the Sprinter class in the js namespace without the prefix.

@@ +783,5 @@
> +js::Sprinter::init()
> +{
> +    base = (char *) context->malloc_(DefaultSize);
> +    if (base)
> +    {

Style nit: brace on same line as conditional.

@@ +838,5 @@
> +char *
> +js::Sprinter::reserve(size_t len)
> +{
> +    while (len + 1 > size - offset) /* Include trailing \0 */
> +    {

Style nit: brace on same line as while head.

@@ -950,5 @@
> -     * the OFF2STR below gives a valid result.
> -     */
> -    if (off == sp->offset && Sprint(sp, "") < 0)
> -        return NULL;
> -    return OFF2STR(sp, off);

How great is it that we don't have to do stuff like this anymore! Huzzah for sensible invariants!

@@ +1270,5 @@
> +    explicit SprintStack(JSContext *cx) : sprinter(cx), offsets(NULL),
> +        opcodes(NULL), bytecodes(NULL), top(0), inArrayInit(0),
> +        inGenExp(JS_FALSE), printer(NULL)
> +    {
> +    }

Style nit: we typically write this like so (if you overflow the first line you put a newline and half-indent the colon for the initializer list):

explicit SprintStack(JSContext *cx)
  : sprinter(cx), offsets(NULL),
    opcodes(NULL), bytecodes(NULL), top(0), inArrayInit(0),
    inGenExp(JS_FALSE), printer(NULL)
{ }

@@ -1334,5 @@
> -
> -    /*
> -     * Must call GetOff before using ss->sprinter.base, since it may be null
> -     * until bootstrapped by GetOff.
> -     */

Hah! More grossness elimination.

@@ +4752,4 @@
>                  rval = QuoteString(&ss->sprinter, atom, inXML ? DONT_ESCAPE : '"');
>                  if (!rval)
>                      return NULL;
> +                todo = rval - ss->sprinter.string();

Can this be made into ss->sprinter.getOffsetOf(rval)? If not, can we add a small comment as to why?

::: js/src/jsopcode.h
@@ +467,5 @@
> +  private:
> +    static const size_t     DefaultSize;
> +    char                    *base;          /* malloc'd buffer address */
> +    size_t                  size;           /* size of buffer allocated at base */
> +    ptrdiff_t               offset;         /* offset of next free char in buffer */

Could you a field that's:

#ifdef DEBUG
    bool initialized;
#endif

(And set it appropriately in the constructor / assert on it being false in |init()|.)

I'd like to assert that we only initialize once, just because the sprint stack code is a little bit hairy.

@@ +482,5 @@
> +    void checkInvariants() const;
> +
> +    /* Returns base */
> +    const char *string() const;
> +    /* Returns base + offset */

We talked on IRC about the fact I over-generalized the comments-on-small-methods recommendation. That was my bad. :-) The overtly obvious / just-describing-the-code ones can be removed in the revised patch.

@@ +496,5 @@
> +     * Attempt to reserve len + 1 space in sp (for a trailing NULL byte). If the
> +     * attempt succeeds, return a pointer to the start of that space and adjust the
> +     * length of sp's contents. The caller *must* completely fill this space on success.
> +     */
> +    char *reserve(size_t len);

Nit: s/in sp// and s/adjust the length of sp's contents/adjust the internal offset/ since the API exposes offset as a concept.

@@ +497,5 @@
> +     * attempt succeeds, return a pointer to the start of that space and adjust the
> +     * length of sp's contents. The caller *must* completely fill this space on success.
> +     */
> +    char *reserve(size_t len);
> +    /* Like reserve, but emoy is initialized to 0 */

Nit: emoy?

::: js/src/methodjit/Compiler.cpp
@@ +1410,4 @@
>  #define SPEW_OPCODE()                                                         \
>      JS_BEGIN_MACRO                                                            \
>          if (IsJaegerSpewChannelActive(JSpew_JSOps)) {                         \
> +            Sprinter sprinter(cx);                                            \

Missing initialization?

::: js/src/shell/js.cpp
@@ -1917,2 @@
>                  PutEscapedString(buf, len, atom, 0);
> -                buf[len] = '\0';

Can we assert this in the new version?

Comment 10

[Adam]

Attachment: sprinter rewrite #4

Comment 11

[Adam]

Attachment: remove-unaligned.diff

Comment 12

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 589304 [details] [diff] [review]
remove-unaligned.diff

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

Huzzah! Nice work.

Comment 13

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 588561 [details] [diff] [review]
sprinter rewrite #4

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

With a few minor changes on push that we discussed on IRC -- I'm pushing the modified patch to try in the next few minutes and will post the link.

Comment 14

[Chris Leary [:cdleary] (not checking bugmail)]

Adam said he's going to post another patch.

Comment 15

[Adam]

Attachment: sprinter rewrite #5

Comment 16

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 589393 [details] [diff] [review]
sprinter rewrite #5

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

Pushed to try! https://tbpl.mozilla.org/?tree=Try&rev=2fcb340cf597

::: js/src/jsopcode.cpp
@@ +762,5 @@
> +Sprinter::init()
> +{
> +#ifdef DEBUG
> +    JS_ASSERT(!initialized);
> +#endif

Nit: JS_ASSERT is known to only evaluate the expression in DEBUG, so we don't wrap it in #ifdef DEBUG.

@@ +781,5 @@
> +{
> +#ifdef DEBUG
> +    JS_ASSERT((size_t) offset < size);
> +    JS_ASSERT(base[size - 1] == 0);
> +#endif

Same deal.

Comment 17

[Chris Leary [:cdleary] (not checking bugmail)]

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

Comment 18

[Chris Leary [:cdleary] (not checking bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/24dac171a324
https://hg.mozilla.org/integration/mozilla-inbound/rev/7026011a83df

Nice work, Adam. That was a very involved first bug. :-)

CCing Gary, Jesse, and Christian -- can we do some decompiler fuzzing to see if we hit any of the bountiful assertions we've added?

Comment 19

[Christian Holler (:decoder)]

I'm on it :) I'll file bugs specific to those two inbound revisions as blocking to this one, if any pop up.

Comment 21

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/24dac171a324
https://hg.mozilla.org/mozilla-central/rev/7026011a83df

Comment 22

[Christian Holler (:decoder)]

Stopped testing with LangFuzz for now (found bug 720680) but gkw might want to try jsfunfuzz as well :)

Comment 23

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to Christian Holler (:decoder) from comment #22)
> Stopped testing with LangFuzz for now (found bug 720680)

Got that one fixed, can we deploy some more fuzz now? :-)

Comment 24

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> Got that one fixed, can we deploy some more fuzz now? :-)

Yup, I saw that these 2 mentioned bugs are on m-c so am fuzzing m-c with jsfunfuzz too.

Comment 25

[Christian Holler (:decoder)]

(In reply to Chris Leary [:cdleary] from comment #23)

> Got that one fixed, can we deploy some more fuzz now? :-)

Since this is all on m-c now, there is permanent fuzzing on this now by default from both of our JS fuzzers :) We'll see soon if we can find more bugs here :)