774471 - Move source maps from being associated with JSScripts to ScriptSource

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Right now, only top level JSScripts even have source map urls, and it would be a lot easier (and logical) if the source map url information were stored on ScriptSource.

Comment 1

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

Attachment: v1

Comment 2

[:Benjamin Peterson]

Comment on attachment 642843 [details] [diff] [review]
v1

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

::: js/src/frontend/TokenStream.h
@@ +681,2 @@
>       */
> +    jschar *copySourceMap(JSContext *cx) {

This function doesn't seem to provide a way distinguish OOM from not having a sourceMap. It should.

@@ +681,5 @@
>       */
> +    jschar *copySourceMap(JSContext *cx) {
> +        if (!sourceMap)
> +            return NULL;
> +        jschar *sm = (jschar *) cx->malloc_((sourceMapLength + 1) * sizeof(jschar));

Use static_cast<jschar *>.

::: js/src/jsscript.h
@@ +961,5 @@
>      static bool performXDR(XDRState<mode> *xdr, ScriptSource **ss);
>  
> +    // Source maps
> +    bool setSourceMap(JSContext *cx, jschar *sm);
> +    jschar *getSourceMap();

This should probably be named sourceMap() or maybeSourceMap().

@@ +963,5 @@
> +    // Source maps
> +    bool setSourceMap(JSContext *cx, jschar *sm);
> +    jschar *getSourceMap();
> +    jschar *releaseSourceMap();
> +    void destroySourceMap(JSRuntime *rt);

Does anyone actually need to call these two? (destroySource could be inlined in ScriptSource::destroy.)

@@ +964,5 @@
> +    bool setSourceMap(JSContext *cx, jschar *sm);
> +    jschar *getSourceMap();
> +    jschar *releaseSourceMap();
> +    void destroySourceMap(JSRuntime *rt);
> +    bool hasSourceMap:1;

Isn't this redundant with sourceMap != NULL?

Comment 3

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

Attachment: v2

Updated patch based on feedback.

Comment 4

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

> This should probably be named sourceMap() or maybeSourceMap().

My impression was that getters that could possibly fail should be called "getX" where as ones that can't fail should just be called "X". If I'm wrong, I'll be happy to change that.

Comment 5

[:Benjamin Peterson]

Comment on attachment 644422 [details] [diff] [review]
v2

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

::: js/src/frontend/TokenStream.cpp
@@ +1257,4 @@
>          for (int i = 0; i < len; i++)
>              sourceMap[i] = tokenbuf[i];
>          sourceMap[len] = '\0';
> +        sourceMapLength = len;

Suggestion: s/len/sourceMapLength/ above.

::: js/src/frontend/TokenStream.h
@@ +685,4 @@
>       */
> +    jschar *copySourceMap(JSContext *cx) {
> +        JS_ASSERT(hasSourceMap());
> +        jschar *sm = static_cast<jschar *>

Line length is 100, btw. I can't tell, but maybe you don't need to wrap this?

::: js/src/jsscript.cpp
@@ +1199,4 @@
>      JS_ASSERT(onRuntime());
>      rt->free_(data.compressed);
>      onRuntime_ = marked = false;
> +    if (hasSourceMap()) {

free_ should be able to handle NULL pointers, so you probably don't need this check.

@@ +1310,5 @@
> +ScriptSource::setSourceMap(JSContext *cx, jschar *sm) {
> +    JS_ASSERT(!hasSourceMap());
> +
> +    if (!sm)
> +        return false;

Is being able to pass NULL here useful? It seems to me this function should just be void.

@@ +1723,5 @@
>      script->nslots = script->nfixed + bce->maxStackDepth;
>  
> +    if (bce->parser->tokenStream.hasSourceMap()) {
> +        jschar *sourceMap = static_cast<jschar *>
> +            (bce->parser->tokenStream.copySourceMap(cx));

No cast needed, I think.

::: js/src/jsscript.h
@@ +961,5 @@
>      static bool performXDR(XDRState<mode> *xdr, ScriptSource **ss);
>  
> +    // Source maps
> +    bool setSourceMap(JSContext *cx, jschar *sm);
> +    jschar *getSourceMap();

Since it's infallible now, it can be called sourceMap(). Also, it can be const.

@@ +962,5 @@
>  
> +    // Source maps
> +    bool setSourceMap(JSContext *cx, jschar *sm);
> +    jschar *getSourceMap();
> +    bool hasSourceMap();

This can be const, too.

Comment 6

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

Attachment: v3

Updated based on feedback.

Comment 7

[:Benjamin Peterson]

Comment on attachment 644516 [details] [diff] [review]
v3

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

::: js/src/frontend/TokenStream.cpp
@@ +1250,4 @@
>  
>          if (sourceMap)
>              cx->free_(sourceMap);
> +        sourceMap = (jschar *) cx->malloc_(sizeof(jschar) * (sourceMapLength + 1));

Use static_cast.

@@ +1254,4 @@
>          if (!sourceMap)
>              return false;
>  
> +        for (int i = 0; i < sourceMapLength; i++)

This whole loop:
PodCopy(sourceMap, tokenbuf.begin(), sourceMapLength);

::: js/src/frontend/TokenStream.h
@@ +685,2 @@
>       */
> +    jschar *copySourceMap(JSContext *cx) {

Why do we have to always copy it instead of releasing it to the script source?

Comment 8

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

> Why do we have to always copy it instead of releasing it to the script
> source?

I made it this way based on a comment jimb made in IRC about how he felt that he wouldn't expect any mutations to the parser as a side effect of JSScript::fullyInitFromEmitter.

Comment 9

[Jim Blandy :jimb]

(In reply to Nick Fitzgerald :fitzgen from comment #8)
> > Why do we have to always copy it instead of releasing it to the script
> > source?
> 
> I made it this way based on a comment jimb made in IRC about how he felt
> that he wouldn't expect any mutations to the parser as a side effect of
> JSScript::fullyInitFromEmitter.

What I meant was, I expect a function like fullyInitFromEmitter to mutate the script it's initializing, but not its arguments.

Instead, I would expect the ScriptSource to take ownership of the source map data when the ScriptSource itself is constructed. Isn't it possible for ScriptSource::createFromSource to take the source maps as arguments, and then pass them the appropriate source maps in frontend::CompileScript and frontend::CompileFunctionBody?

Hmm. Since I last looked at the ScriptSource stuff, it seems that ScriptSources are now absent on JSScripts produced by XDR, and only provided on demand via JSRuntime::sourceHook. If ScriptSources are going to own source map URLs, how will we store source map URLs on JSScripts that don't have ScriptSources yet?

I see two possibilities:

- JSRuntime::sourceHook could furnish source map URLs as well as source text. This doesn't quite seem right: its obvious that a sourceHook can find the sources, but not so obvious that it can find associated source maps.

- JSScripts could always have ScriptSources, but ScriptSources might be empty, and need to have sourceHook applied to them to be "filled in". Then every script would always have a place to store source map URLs. (Perhaps we could move JSScript::filename in there, too, and save a pointer.)

Comment 10

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

Attachment: v4

So I made a constructor for |ScriptSource| which optionally takes a source map as the only argument. It creates an empty skeleton of a script source, so if you want to fill it in you have to call |setSource| which is pretty much the old |createFromSource|. Inside |JSScript::Create|, if |ss| is NULL, then an empty skeleton script source is created and saved, so a script should always have a script source. Lastly, the script source is no longer given the source map inside of |JSScript::fullyInitFromEmitter|, it happens in the same scope that a script source is created (if applicable).

So currently I am never actually passing a source map to the script source constructor because we create the script source before we create the script before we parse. We don't have access to the source map until after we parse, so I can't really change that without changing a whole lot of other stuff, it seems to me. However, once I update the patch for bug 765993, it will be possible to have a source map before we parse and I will be able to take advantage of the constructor.

Feedback appreciated.

Comment 11

[:Benjamin Peterson]

Comment on attachment 645472 [details] [diff] [review]
v4

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

Good start.

::: js/src/frontend/BytecodeCompiler.cpp
@@ +85,4 @@
>      SourceCompressionToken sct(cx->runtime);
>      ScriptSource *ss = NULL;
>      if (!cx->hasRunOption(JSOPTION_ONLY_CNG_SOURCE) || compileAndGo) {
> +        ss = cx->new_<ScriptSource>();

Don't you need to always create it in case the condition on line 215 is triggered?

@@ +95,1 @@
>          attacher.ss = ss;

Move this above the setSource call, and you won't have to do cx->free_(ss) in the setSource error case.

@@ +212,4 @@
>          parser.freeTree(pn);
>      }
>  
> +    if (tokenStream.hasSourceMap()) {

Drop braces.

@@ +373,4 @@
>          pn = fn->pn_body;
>      }
>  
> +    if (parser.tokenStream.hasSourceMap()) {

Ditto on braces.

::: js/src/frontend/TokenStream.h
@@ +677,4 @@
>       */
>      size_t endOffset(const Token &tok);
>  
> +    bool hasSourceMap() {

bool hasSourceMap() const {

::: js/src/jsscript.cpp
@@ +1152,3 @@
>       * scripts pointing to the source may no longer be reachable.
>       */
>      if (cx->runtime->gcIncrementalState == MARK && cx->runtime->gcIsFull)

This whole block needs to go in the constructor.

@@ +1220,1 @@
>      JS_ASSERT(onRuntime());

The assertion above this will fail when a empty ScriptSource is GCed. The ready_ flag should be set to true in the constructor and to false before compression starts. It should probably also be renamed.

@@ +1325,5 @@
>      return true;
>  }
>  
> +void
> +ScriptSource::setSourceMap(jschar *sm) {

Brace on the next line for these functions per style.

@@ +1561,4 @@
>      }
>      script->staticLevel = uint16_t(staticLevel);
>  
> +    if (ss) {

If scripts are going to always have sources, I'd prefer we force callers to pass in a non-null one.

::: js/src/jsscript.h
@@ +977,4 @@
>  #ifdef DEBUG
>      bool ready_:1;
>  #endif
> +    jschar *sourceMap_;

Put this before the bitfield.

@@ +1000,5 @@
> +                   uint32_t length,
> +                   bool argumentsNotIncluded = false,
> +                   SourceCompressionToken *tok = NULL,
> +                   bool ownSource = false);
> +    bool hasSource();

Make this const.

@@ +1023,5 @@
>  
> +    // Source maps
> +    void setSourceMap(jschar *sm);
> +    const jschar *sourceMap();
> +    bool hasSourceMap();

bool hasSourceMap() const

@@ +1029,2 @@
>    private:
>      bool compressed() { return !!compressedLength; }

Accesors like this and length()/argumentsIncluded()... above that are only meaningful with a source should assert hasSource().

Comment 12

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

Attachment: v5

Updated based on feedback.

|ready_| is set to false before compression inside |setSource|, however it also appears that compression happens from the |SourceCompressor| and I wasn't sure where/when to set |ready_| to false for that.

Comment 13

[:Benjamin Peterson]

Comment on attachment 645871 [details] [diff] [review]
v5

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

::: js/src/frontend/BytecodeCompiler.cpp
@@ +86,5 @@
> +    ScriptSource *ss = cx->new_<ScriptSource>(cx);
> +    if (!ss)
> +        return NULL;
> +    attacher.ss = ss;
> +    if (!cx->hasRunOption(JSOPTION_ONLY_CNG_SOURCE) || compileAndGo)

Need braces around this one now.

::: js/src/jsscript.cpp
@@ +565,4 @@
>          JSVersion version_ = JSVersion(version & JS_BITMASK(16));
>          JS_ASSERT((version_ & VersionFlags::FULL_MASK) == unsigned(version_));
>  
> +         ScriptSource *ss = cx->new_<ScriptSource>(cx);

I know I said that the ScriptSource argument to JSScript::Create should never to be NULL, but I think we can make an exception here, since it's set several lines father down. Let's set it then. That will prevent this one from leaking if we replace it from an XDRed one.

@@ +565,5 @@
>          JSVersion version_ = JSVersion(version & JS_BITMASK(16));
>          JS_ASSERT((version_ & VersionFlags::FULL_MASK) == unsigned(version_));
>  
> +         ScriptSource *ss = cx->new_<ScriptSource>(cx);
> +        if (!ss)

Indentation looks wrong.

@@ +1114,5 @@
> +#endif
> +}
> +
> +bool
> +ScriptSource::hasSource() const

Might as well just inline this on the class definition.

@@ +1168,5 @@
> +    next = NULL;
> +    length_ = length;
> +    compressedLength = 0;
> +    marked = onRuntime_ = false;
> +    argumentsNotIncluded_ = argumentsNotIncluded;

Don't need to initialize the members that are handled by the constructor.

@@ +1173,2 @@
>  #ifdef DEBUG
> +    ready_ = false;

Move this block in the same condition as line 1186 and SourceCompressorThread::compress.

@@ +1354,5 @@
> +    return sourceMap_;
> +}
> +
> +bool
> +ScriptSource::hasSourceMap() const

You could put these in the class definition, too, if you want.

::: js/src/jsscript.h
@@ +996,5 @@
> +        JS_ASSERT(hasSource());
> +        return length_;
> +    }
> +    bool onRuntime() const {
> +        JS_ASSERT(hasSource());

onRuntime() doesn't require hasSource().

Comment 14

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

Attachment: v6

Updated based on feedback.

> Move this block in the same condition as line 1186 and SourceCompressorThread::compress.

Sorry I made other changes before I got to this one and I am not sure where you want me to move this anymore. Can you tell me  again?

Comment 15

[:Benjamin Peterson]

Comment on attachment 645934 [details] [diff] [review]
v6

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

::: js/src/jsscript.cpp
@@ +1158,5 @@
>      const size_t memlen = length * sizeof(jschar);
> +    data.compressed = static_cast<unsigned char *>(cx->malloc_(memlen));
> +    if (!data.compressed)
> +        return false;
> +    argumentsNotIncluded_ = argumentsNotIncluded;

You still need to set length_.

@@ +1169,5 @@
>          tok->chars = src;
>          cx->runtime->sourceCompressorThread.compress(tok);
>      } else
>  #endif
> +        considerCompressing(cx->runtime, src, ownSource);

Put ss->ready_ = false here.

@@ +1295,3 @@
>          cleanup.protect(ss);
>  #ifdef JSGC_INCREMENTAL
> +        // See comment in ScriptSource constructor.

Since this is in the constructor now, the whole block can perish.

Comment 16

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

Attachment: v7

Update based on feedback.

Comment 17

[Jim Blandy :jimb]

Comment on attachment 645934 [details] [diff] [review]
v6

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

::: js/src/frontend/BytecodeCompiler.cpp
@@ +85,5 @@
>      SourceCompressionToken sct(cx->runtime);
> +    ScriptSource *ss = cx->new_<ScriptSource>(cx);
> +    if (!ss)
> +        return NULL;
> +    attacher.ss = ss;

Since we're always creating a ScriptSource now, it would be kind of nice if AutoAttachToRuntime's constructor just took a ScriptSource * as an argument. Then you could simply move the declaration of attacher below the ScriptSource allocation check, initialize it, and forget about it.

Comment 18

[:Benjamin Peterson]

Comment on attachment 646751 [details] [diff] [review]
v7

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

Almost there. XDR just needs to be fixed.

::: js/src/jsscript.cpp
@@ +568,4 @@
>          JSVersion version_ = JSVersion(version & JS_BITMASK(16));
>          JS_ASSERT((version_ & VersionFlags::FULL_MASK) == unsigned(version_));
>  
> +        ScriptSource *ss = cx->new_<ScriptSource>(cx);

This still leaks latter in this function when script->source is actually assigned.

@@ +569,5 @@
>          JS_ASSERT((version_ & VersionFlags::FULL_MASK) == unsigned(version_));
>  
> +        ScriptSource *ss = cx->new_<ScriptSource>(cx);
> +        if (!ss)
> +            return JS_FALSE;

This can just be "return false";

@@ +1109,5 @@
> +     * During the IGC we need to ensure that source is marked whenever it is
> +     * accessed even if the name was already in the table. At this point old
> +     * scripts pointing to the source may no longer be reachable.
> +     */
> +    if (cx->runtime->gcIncrementalState == MARK && cx->runtime->gcIsFull)

When you rebase, kill this whole block.

::: js/src/jsscript.h
@@ +979,4 @@
>  #endif
>  
>    public:
> +    ScriptSource(JSContext *cx,

This should be marked explicit. I also think you can put the whole parameter list on one line.

Comment 19

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

Attachment: v8

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

There's a couple other commits stuck in there. One from earlier in my patch queue that I already know doesn't cause problems based on other try pushes, and the other is something from fx-team, I think. Hopefully it doesn't cause any issues.

Comment 20

[:Benjamin Peterson]

Comment on attachment 647331 [details] [diff] [review]
v8

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

Okay. :)

::: js/src/jsscript.cpp
@@ +1151,4 @@
>      if (!ownSource) {
>          const size_t memlen = length * sizeof(jschar);
> +        data.compressed = static_cast<unsigned char *>(cx->malloc_(memlen));
> +        if (!data.compressed) {

Drop braces.

Comment 21

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

Attachment: v9

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

Currently, the only orange is also orange on the try push I also did for the "always create ScriptSources" bug (https://tbpl.mozilla.org/?tree=Try&rev=7f1bc75b583c) so that I could tell what was and wasn't my fault.

Comment 22

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

Comment on attachment 647729 [details] [diff] [review]
v9

Fresh try push now that bug 779017 has landed: https://tbpl.mozilla.org/?tree=Try&rev=348cb00ae93f

Comment 23

[:Benjamin Peterson]

Comment on attachment 647729 [details] [diff] [review]
v9

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

Another nit.

::: js/src/frontend/TokenStream.h
@@ -677,5 @@
>      size_t endOffset(const Token &tok);
>  
> -    /*
> -     * Give up responsibility for managing the sourceMap filename's memory.
> -     */

This comment can stay.

Comment 24

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

Attachment: v9.1

Just added the comment back in and changed the order of initializing fields in ScriptSource's constructor to suppress a compiler warning. Shouldn't need another try push.

Comment 25

[Jim Blandy :jimb]

Comment on attachment 648510 [details] [diff] [review]
v9.1

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

This still needs XDR support, with tests. Marking feedback+, because that's got to be in before this lands.

::: js/src/frontend/TokenStream.cpp
@@ +1255,5 @@
>          if (!sourceMap)
>              return false;
>  
> +        PodCopy(sourceMap, tokenbuf.begin(), sourceMapLength);
> +        sourceMap[sourceMapLength] = '\0';

*Almost* another instance of js_strdup... except the input isn't null-terminated. Oh well.

::: js/src/frontend/TokenStream.h
@@ -680,5 @@
>      size_t endOffset(const Token &tok);
>  
> -    /*
> -     * Give up responsibility for managing the sourceMap filename's memory.
> -     */

Nit: I agree with benjamin that this comment should be retained.

Comment 26

[Jim Blandy :jimb]

Oh, and: this is looking great. Looking forward to having it land. I've got some thoughts on how to make Debugger.Source really support alternative sources nicely...

Comment 27

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

Attachment: v10

Add support for XDR. That darn comment is in there, too (if it isn't, then I am going crazy).

Added a test for source maps and XDR as well.

Comment 28

[:Benjamin Peterson]

Comment on attachment 648893 [details] [diff] [review]
v10

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

::: js/src/jsscript.cpp
@@ +1367,5 @@
> +{
> +    JS_ASSERT(!hasSourceMap());
> +    JS_ASSERT(sm);
> +    sourceMap_ = sm;
> +    sourceMapLength = js_strlen(sm);

It seems you only need this for XDR. Why not just compute it then on encoding?

::: js/src/jsscript.h
@@ +977,5 @@
>      } data;
>      uint32_t length_;
>      uint32_t compressedLength_;
> +    jschar *sourceMap_;
> +    uint32_t sourceMapLength;

This really out to be called sourceMapLength_.

Comment 29

[:Benjamin Peterson]

Comment on attachment 648893 [details] [diff] [review]
v10

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

::: js/src/jsscript.cpp
@@ +1342,5 @@
> +            sourceMap_ = static_cast<jschar *>(xdr->cx()->malloc_(byteLen));
> +            if (!sourceMap_)
> +                return false;
> +        }
> +        if (!xdr->codeBytes(sourceMap_, byteLen)) {

Use codeChars.

Comment 30

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

Attachment: v11

Removed the |sourceMapLength| member from |ScriptSource|. Used |xdr->codeChars| instead of |xdr->codeBytes|.

Comment 31

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

Attachment: v11.1

Minor tweak in the conditional setting of |sourceMapLen| to make it more clear.

Comment 32

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

Attachment: v11.2

Rebased on M-C.

Comment 33

[Jim Blandy :jimb]

Comment on attachment 649793 [details] [diff] [review]
v11.2

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

Since you've changed the XDR format, you need to update XDR_BYTECODE_VERSION in js/src/vm/Xdr.h, as described in the comments.

Other than that and moving sourceMapLength, this looks ready to go.

::: js/src/frontend/TokenStream.cpp
@@ +1255,5 @@
>          if (!sourceMap)
>              return false;
>  
> +        PodCopy(sourceMap, tokenbuf.begin(), sourceMapLength);
> +        sourceMap[sourceMapLength] = '\0';

As far as I can tell, TokenStream::sourceMapLength's value is dead after this point: it'll never be used again, because it's only used here, and we set it from tokenbuf.length() before doing so. So it should be a variable local to this block, not a member of TokenStream.

::: js/src/jsscript.cpp
@@ +1301,5 @@
> +        uint32_t sourceMapLen = (mode == XDR_DECODE) ? 0 : js_strlen(sourceMap_);
> +        if (!xdr->codeUint32(&sourceMapLen))
> +            return false;
> +
> +        size_t byteLen = (sourceMapLen + 1) * sizeof(jschar);

Isn't byteLen used only inside the 'if decode' block? The declaration could be moved inside there if you think it's an improvement.

Comment 34

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

Attachment: v11.3

Updated based on feedback.

Can you commit this Jim? I don't have push access. Thanks!

Comment 35

[Jim Blandy :jimb]

Since we've added the XDR support, and tests for that, I've re-pushed to try. I'll take care of landing it as soon as it's done, assuming all goes greenly.

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

Comment 36

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b24f11ad62b3

Comment 37

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b24f11ad62b3