Jason Orendorff [:jorendorff] Reporter
	Description • 10 years ago

ES6 distinguishes between let/const variables with the value `undefined` and those that have not been initialized at all.

    {
        let x;
        x;        // <==== ReferenceError: x is uninitialized
    }

This is a ReferenceError even in non-strict code.

	Jason Orendorff [:jorendorff] Reporter
	Comment 1 • 10 years ago

## Whaaaat? Where in the spec does it say this is a ReferenceError?

In the GetBindingValue method of declarative lexical environments <http://people.mozilla.org/~jorendorff/es6-draft-rev-22.html#sec-declarative-environment-records-getbindingvalue-n-s>:

> 3. If the binding for N in envRec is an uninitialised binding, then
>     a. If S is false, return the value undefined, otherwise throw a
>         ReferenceError exception.

Unfortunately, in the current draft, step 3.a. is incorrect; it states that the exception only happens in strict mode. According to Domenic, this is a spec bug. Step 3.a. should read simply "Throw a ReferenceError exception." Trying to confirm.

Anyway. How did we get to this method? Well, evaluating an identifier that refers to any kind of local binding produces a Reference:

- Evaluating an identifier calls ResolveBinding.
http://people.mozilla.org/~jorendorff/es6-draft-rev-22.html#sec-identifier-reference-runtime-semantics-evaluation

- ResolveBinding calls GetIdentifierReference.
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-resolvebinding

- GetIdentifierReference, when an enclosing LexicalEnvironment has a binding
(which is the case if the identifier refers to a let/const binding in scope),
returns a Reference whose base value is that LexicalEnvironment.

Then, right after evaluation, GetValue is called to turn the Reference into a value. This always happens. Whenever identifiers are evaluated, except as assignment targets, GetValue is the next step.

- In the code above, we have an ExpressionStatement, so we call GetValue from:
http://people.mozilla.org/~jorendorff/es6-draft-rev-22.html#sec-expression-statement-runtime-semantics-evaluation

- The last step of GetValue calls the GetBindingValue "concrete method":
http://people.mozilla.org/~jorendorff/es6-draft-rev-22.html#sec-getvalue

	André Bargull [:anba]
	Comment 2 • 10 years ago

(In reply to Jason Orendorff [:jorendorff] from comment #1)
> Unfortunately, in the current draft, step 3.a. is incorrect; it states that
> the exception only happens in strict mode. According to Domenic, this is a
> spec bug. Step 3.a. should read simply "Throw a ReferenceError exception."
> Trying to confirm.

Filed at https://bugs.ecmascript.org/show_bug.cgi?id=2709

	André Bargull [:anba]
	Comment 3 • 10 years ago

(In reply to Jason Orendorff [:jorendorff] from comment #0)
> ES6 distinguishes between let/const variables with the value `undefined` and
> those that have not been initialized at all.
> 
>     {
>         let x;
>         x;        // <==== ReferenceError: x is uninitialized
>     }
> 
> This is a ReferenceError even in non-strict code.

And I guess the example should actually be:
---
{
    x;        // <==== ReferenceError: x is uninitialized
    let x;    
}
---

because after "let x;", "x" is initialized to "undefined". [13.2.1.4 Runtime Semantics: Evaluation]

	Jason Orendorff [:jorendorff] Reporter
	Comment 4 • 10 years ago

André is right about everything as usual.

In the bug linked in comment 2, Allen confirms that an error should be thrown in both strict and non-strict code.

	Shu-yu Guo [:shu] Assignee
	Comment 5 • 10 years ago

Similarly, for assignments to uninitialized let/const declarations, SetMutableBinding is what throws the ReferenceError in step 8.1.1.1.5 (3): https://people.mozilla.org/~jorendorff/es6-draft.html#sec-declarative-environment-records-setmutablebinding-n-v-s

	Shu-yu Guo [:shu] Assignee
	Comment 6 • 10 years ago

If I'm reading the spec right, the following throws a ReferenceError instead of binding the outer 'x'. Is this right?

{
  let x;
  {
     x; // 'x' is defined in the current declarative env, but uninitialized
     let x;
  }
}

	Till Schneidereit [:till]
	Comment 7 • 10 years ago

(In reply to Shu-yu Guo [:shu] from comment #6)
> If I'm reading the spec right, the following throws a ReferenceError instead
> of binding the outer 'x'. Is this right?

You probably already got an answer to this, but yes, that's correct. The temporal dead zone wouldn't make much sense otherwise, I think.

	Shu-yu Guo [:shu] Assignee
	Comment 8 • 10 years ago

WIP of the approach so far. The frontend and interpreter parts.

The idea here is to:

 1) Introduce MagicValue(JS_UNINITIALIZED_LET), which will be used to
    initialize 'let' bindings and will throw ReferenceError on touch.

 2) Introduce a family of JSOps which check for the magic value for easy
    compilation in the JITs and as to not slowdown the existing LOCAL/ALIASEDVAR
    ops. The specific scenarios for which these new JSOps will be emitted are
    listed below.

    The new ops are:
     - JSOP_GETLET
     - JSOP_SETLET
     - JSOP_INITLET
     - JSOP_GETALIASEDLET
     - JSOP_SETALIASEDLET
     - JSOP_INITALIASEDLET

    The init variants are able to overwrite MagicValue(JS_UNINITIALIZED_LET).
    All other ops throw on touching the magic value.

    JSOP_NAME and JSOP_SETNAME, being deoptimized cases already, will always
    check for the magic value.

The assumption will be that most programs written with 'let' bindings will have
simple to compute (e.g., analyzable at parse time) dominance relation w.r.t.
their uses. So the patch only generates the LET ops when it cannot prove in a
straightforward fashion that a 'let' binding initialization dominates all its
uses. The scenarios the parser detects are:

 - A 'let' def that resolves existing lexdeps marks all of its lexdep's uses as
   hoisted uses, thus generating LET ops.

 - A free access to a 'let' in a body-level function declaration is marked as a
   hoisted let use if there are any uses of the function name before the
   observed declaration. This is due to its behavior of hoisting both its
   declaration and definition) 

 - When parsing an inner function lazily, free accesses are unknown at parse
   time to be 'let' or 'var' bindings. In this case, if any upvar use is
   hoisted at all---either the definition of the free variable in the outer
   parser context is a placeholder or the inner function is a body-level
   function declaration and has hoisted uses of its own name---we mark the
   LazyScript to convert any upvar let accesses during bytecode compilation to
   be one of the LET ops.

   This is a coarse solution. At the cost of memory (a boolean vector at the
   end of LazyScript), we could track hoisted uses on an upvar by upvar basis.

Body-level 'let's also have to now be distinguished from 'var's. This patch
puts all 'let' slots after all 'var' slots. That is, the fixed part of
execution frames have 'var' slots come first, then body-level 'let's, then
block-local 'let's.

	Luke Wagner [:luke]
	Comment 9 • 10 years ago

Comment on attachment 8459125 [details] [diff] [review]
WIP Part 1: Frontend and interpreter parts of let temporal dead zone.

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

Seems reasonable.  Unfortunate language design decision, though, imho.  The PND_LET-flag-on-uses is a useful trick but a bit dangerous as sometimes we fiddle with the def-use links in a way that could fail to set the PND_LET flags on uses.  I'd grep around for this.  I keep hoping someone will embark on making the parser two-pass...

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1462,5 @@
> +                    // For emitting lazy scripts, we need to check if we are
> +                    // emitting a use to a hoisted let. See
> +                    // Parser<T>::addFreeVariablesFromLazyFunction.
> +                    if (bce->lazyCheckHoistedLetUse &&
> +                        IsAliasedNameLet(script, pn->pn_atom->asPropertyName()))

It seems like you could fuse the loop in IsAliasedNameLet with the loop in LookupAliasedName and avoid any overhead.  LookupAliasedName could just set the PND_LET flag directly.  Given that, perhaps there is no need for the lazyCheckHoistedLetUse flag (which I assume is an optimization?

::: js/src/jsscript.cpp
@@ +86,5 @@
>      JS_ASSERT(numVars <= LOCALNO_LIMIT);
>      JS_ASSERT(numBlockScoped <= LOCALNO_LIMIT);
> +    JS_ASSERT(numBodyLevelLets <= LOCALNO_LIMIT);
> +    JS_ASSERT(numVars <= LOCALNO_LIMIT - numBodyLevelLets - numBlockScoped);
> +    JS_ASSERT(UINT32_MAX - numArgs >= numVars + numBodyLevelLets + numBlockScoped);

I think these asserts can now underflow.  How about make them using uint64_t's?

::: js/src/vm/Stack-inl.h
@@ +103,5 @@
> +InterpreterFrame::setLetsToThrowOnTouch()
> +{
> +    // 'let' declaration throw ReferenceErrors if they are used before
> +    // initialization. See ES6 8.1.1.1.6.
> +    for (size_t slot = script()->nfixedvars(); slot < script()->nfixed(); slot++)

Could you have symbolic that make this ordering assumption more explicit such as JSScript::bodyLevelLetBegin/End()?  Also, even though this is all relatively cold code, it's probably still worth it to hoist the reads of these two fields since I expect the store in the body of the loop prevents their being LICM'd.

	Shu-yu Guo [:shu] Assignee
	Comment 10 • 10 years ago

(In reply to Luke Wagner [:luke] from comment #9)
> I keep hoping someone will embark on making the parser two-pass...

Not it!

	Shu-yu Guo [:shu] Assignee
	Comment 11 • 10 years ago

Passes jit-tests without --tbpl. Haven't implemented the JIT parts yet.

This is the most gnarly patch imaginable; good luck.

There's a particular bit of nastiness involving legacy genexprs. For an
expression |((E for X in I))|, we create a LexicalScope for the entire
expression, then proceed to parse the for head (X in I) inside that scope. This
is *inside out* of what we do for |for (let X in I)|, which attaches the
LexicalScope to the initializer position of the for head. The reason it does
this is that we just have a single scope no matter how many 'for's are chained
in the genexpr, e.g., even |((E for X in I for Y in J))| still has a single
lexical scope. I guess we could change this, but I... really don't want to
touch this part of the code.

So, there's special casing for legacy genexprs for when to emit the initlets for
for-of/for-in as resulting from legacy genexprs.

	Shu-yu Guo [:shu] Assignee
	Comment 12 • 10 years ago

Cleaned up misc debug things I left in.

	Shu-yu Guo [:shu] Assignee
	Comment 13 • 10 years ago

Things this bug do NOT fix:

 - Parsing consts as lets
 - Scoping for the various for statements
 - Global level lets

	Shu-yu Guo [:shu] Assignee
	Comment 17 • 10 years ago

Fix LazyScript::FreeVariable logic was wrong in BytecodeEmitter. The slot logic
was completely bogus and was marking upvars as hoisted let uses incorrectly.

	Shu-yu Guo [:shu] Assignee
	Comment 18 • 10 years ago

Had an inverted condition in codegen for LLetCheck.

	Shu-yu Guo [:shu] Assignee
	Comment 19 • 10 years ago

Jim, per our IRC conversation, changing UnwindScope uses where we want to
unwind to the outermost scope to not use a pc.

	Shu-yu Guo [:shu] Assignee
	Comment 36 • 10 years ago

Comment on attachment 8471205 [details] [diff] [review]
Part 3: Compile new let opcodes in Ion.

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

Not sure who to ask for these patches. Feel free to reassign, Jan.

	Shu-yu Guo [:shu] Assignee
	Comment 37 • 10 years ago

For all part 4 patch reviewers:

This bug is one of several bringing 'let' semantics in SpiderMonkey up to ES6 compliance. This bug changes the parsing of body-level 'let' declarations to be parsed as lets instead of as vars. As such, body-level 'let' declarations can no longer redeclare existing bindings.

These patches fix the redeclaration errors.

	Shu-yu Guo [:shu] Assignee
	Comment 38 • 10 years ago

Comment on attachment 8471906 [details] [diff] [review]
Part 4a: Fix parse errors in addon-sdk/

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

See instructions in comment 37.

	Shu-yu Guo [:shu] Assignee
	Comment 39 • 10 years ago

Comment on attachment 8471906 [details] [diff] [review]
Part 4a: Fix parse errors in addon-sdk/

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

See instructions in comment 37.

	Shu-yu Guo [:shu] Assignee
	Comment 40 • 10 years ago

Comment on attachment 8471908 [details] [diff] [review]
Part 4c: Fix parse errors in browser/devtools/ and toolkit/devtools/

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

See instructions in comment 37.

	Shu-yu Guo [:shu] Assignee
	Comment 41 • 10 years ago

Comment on attachment 8471910 [details] [diff] [review]
Part 4e: Fix parse errors in dom/contacts/

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

See instructions in comment 37.

	Shu-yu Guo [:shu] Assignee
	Comment 42 • 10 years ago

Comment on attachment 8471918 [details] [diff] [review]
Part 4l: Fix parse errors in toolkit/components/passwordmgr/

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

See instructions in comment 37.

	Tomislav Jovanovic :zombie
	Comment 43 • 10 years ago

Comment on attachment 8471906 [details] [diff] [review]
Part 4a: Fix parse errors in addon-sdk/

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

::: addon-sdk/source/lib/sdk/io/fs.js
@@ +646,5 @@
>  
>  /**
>   * Synchronous open(2).
>   */
> +function openSync(path, flags_, mode_) {

nit: this is an exported function (15 lines below), so i'd rather not change the argument definition, but change the local variable names instead.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 44 • 10 years ago

Comment on attachment 8471917 [details] [diff] [review]
Part 4k: Fix parse errors in toolkit/components/crashes/

>diff --git a/toolkit/components/crashes/CrashManager.jsm b/toolkit/components/crashes/CrashManager.jsm

>   _addSubmissionAsCrash: function (store, processType, crashType, succeeded,
>-                                   id, date) {
>-    let id = id + "-" + this.PROCESS_TYPE_SUBMISSION;
>+                                   id_, date) {
>+    let id = id_ + "-" + this.PROCESS_TYPE_SUBMISSION;
>     let process = processType + "-" + crashType + "-" +
>                   this.PROCESS_TYPE_SUBMISSION;
>     let submission_type = (
>       succeeded ? this.SUBMISSION_TYPE_SUCCEEDED : this.SUBMISSION_TYPE_FAILED);
> 
>     return store.addCrash(process, submission_type, id, date);
>   },

Why not just remove the "let"?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 45 • 10 years ago

Comment on attachment 8471916 [details] [diff] [review]
Part 4j: Fix parse errors in toolkit/components/contentprefs/

r=me with the let removed instead of renaming the parameter

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 46 • 10 years ago

Comment on attachment 8471917 [details] [diff] [review]
Part 4k: Fix parse errors in toolkit/components/crashes/

r=me with the let removed instead of renaming the parameter

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 47 • 10 years ago

Comment on attachment 8471913 [details] [diff] [review]
Part 4g: Fix parse errors in services/healthreport/

Re-using the parameter here is kind of gross, can you rename the parameter to "allAddons" and leave the "let addons" as-is? r=me with that.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 48 • 10 years ago

Comment on attachment 8471911 [details] [diff] [review]
Part 4f: Fix parse errors in services/crypto/

>diff --git a/services/crypto/modules/utils.js b/services/crypto/modules/utils.js

>-  stripHeaderAttributes: function(value) {
>-    let value = value || "";
>+  stripHeaderAttributes: function(value = "") {
>     let i = value.indexOf(";");
>     return value.substring(0, (i >= 0) ? i : undefined).trim().toLowerCase();
>   },

(Very poor use of the ternary operator IMO!)

	Jim Blandy :jimb
	Comment 49 • 10 years ago

Comment on attachment 8471204 [details] [diff] [review]
Part 3a: Fix unwinding all scopes to not use pc.

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

How could it be otherwise?

	Jan de Mooij [:jandem]
	Comment 50 • 10 years ago

Comment on attachment 8471206 [details] [diff] [review]
Part 2: Compile new let opcodes in Baseline.

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

Looks good, r=me with comments addressed.

::: js/src/jit/BaselineCompiler.cpp
@@ +256,5 @@
> +
> +    // Use R0 to minimize code size. If the number of locals to push is <
> +    // LOOP_UNROLL_FACTOR, then the initialization pushes are emitted directly
> +    // and inline.  Otherwise, they're emitted in a partially unrolled loop.
> +    size_t LOOP_UNROLL_FACTOR = 4;

Pre-existing nit: static const size_t

::: js/src/jit/VMFunctions.cpp
@@ +1186,5 @@
>      return &typedObj.typedProto();
>  }
>  
> +bool
> +ThrowUninitializedLet(JSContext *cx)

Maybe call it "ReportUnitializedLet" or "ThrowUnitializedLet" everywhere, since they do basically the same thing. I was initially confused by Throw vs Report.

@@ +1197,5 @@
> +        ReportUninitializedLet(cx, script, pc, GET_LOCALNO(pc));
> +    } else {
> +        MOZ_ASSERT(JSOp(*pc) == JSOP_CHECKALIASEDLET);
> +        ReportUninitializedLet(cx, script, pc, ScopeCoordinate(pc));
> +    }

If ReportUnitializedLet takes a cx/script/pc, you can just:

ReportUninitializedLet(cx, script, pc);
return false;

::: js/src/vm/Interpreter-inl.h
@@ +152,5 @@
>  }
>  
>  static inline bool
>  CheckUninitializedLet(JSContext *cx, HandleScript script, jsbytecode *pc,
>                        ScopeCoordinate sc, HandleValue val)

Can remove the |sc| argument here too (see other comment).

::: js/src/vm/Interpreter.cpp
@@ +4043,5 @@
>      ReportUninitializedLet(cx, name);
>  }
> +
> +void
> +js::ReportUninitializedLet(JSContext *cx, HandleScript script, jsbytecode *pc, ScopeCoordinate sc)

Remove the unused |sc| argument. I also think it'd be cleaner to have a single ReportUninitializedLet function to which you can pass just the script + pc, and have the function get the PropertyName depending on JSOp(*pc).

	Jan de Mooij [:jandem]
	Comment 51 • 10 years ago

(In reply to Jan de Mooij [:jandem] from comment #50)
> Maybe call it "ReportUnitializedLet" or "ThrowUnitializedLet" everywhere,
> since they do basically the same thing. I was initially confused by Throw vs
> Report.

Oh I just realized Throw* returns a bool and Report* does not, so using different names makes more sense then. (Alternative is to have both functions always |return false|, like js::Throw.)

	Jan de Mooij [:jandem]
	Comment 52 • 10 years ago

Comment on attachment 8471205 [details] [diff] [review]
Part 3: Compile new let opcodes in Ion.

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

r=me with comments addressed.

::: js/src/jit/CodeGenerator.cpp
@@ +8894,5 @@
> +    Label done;
> +    ValueOperand inputValue = ToValue(ins, LLetCheck::INPUT);
> +    masm.branchTestMagicValue(Assembler::Equal, inputValue, JS_UNINITIALIZED_LET, ool->entry());
> +
> +    masm.jump(&done);

Let's get rid of this jump in opt builds by moving it into the #ifdef DEBUG:

#ifdef DEBUG
    Label done;
    masm.jump(&done);
    masm.bind(ool->rejoin());
    masm.assumeUnreachable("ThrowUninitializedLet should always bail out.");
    masm.bind(&done);
#else
    masm.bind(ool->rejoin());
#endif

    return true;

Or just remove the assumeUnreachable call and "Label done" completely; it's not really necessary IMO.

@@ +8915,5 @@
> +    masm.bind(ool->rejoin());
> +#ifdef DEBUG
> +    masm.assumeUnreachable("ThrowUninitializedLet should always bail out.");
> +#endif
> +    return true;

If we make this a call instruction (see other comment), just do an inline call:

return callVM(ThrowUninitializedLetInfo, ins);

::: js/src/jit/IonBuilder.cpp
@@ +969,5 @@
> +{
> +    for (uint32_t i = 0; i < info().nlocals(); i++) {
> +        MConstant *val = MConstant::New(alloc(), (i < info().fixedLetBegin()
> +                                                  ? UndefinedValue()
> +                                                  : MagicValue(JS_UNINITIALIZED_LET)));

Pre-existing, but can you move the MConstant::New outside the loop? There's no need to add duplicate MIR instructions and then rely on GVN to coalesce them :)

@@ +9880,5 @@
> +    MDefinition *let = addLetCheck(getAliasedVar(sc));
> +    if (!let)
> +        return false;
> +
> +    jsbytecode *nextPc = pc + js_CodeSpec[JSOp(*pc)].length;

jsbytecode *nextPc = pc + JSOP_CHECKALIASEDLET_LENGTH;

@@ +9887,5 @@
> +
> +    // If we are checking for a load, push the checked let so that the load
> +    // can use it.
> +    if (JSOp(*nextPc) == JSOP_GETALIASEDVAR)
> +        pushLetCheck(let);

"push" is a bit confusing because it doesn't really push anything on the simulated expression stack. Maybe setLetCheck/getLetCheck?

@@ +10140,5 @@
>      JSObject *call = nullptr;
>      if (hasStaticScopeObject(sc, &call) && call) {
>          PropertyName *name = ScopeCoordinateName(scopeCoordinateNameCache, script(), pc);
>          bool succeeded;
>          if (!getStaticName(call, name, &succeeded))

The run-once closure case doesn't need changes? Please make sure we have tests for this, something like this in the global scope:

(function() {
    ...
})();

::: js/src/jit/LIR-Common.h
@@ +6172,5 @@
> +  public:
> +    LIR_HEADER(LetCheck)
> +
> +    explicit LLetCheck() {
> +    }

Nit: rm constructor

@@ +6178,5 @@
> +    MLetCheck *mir() {
> +        return mir_->toLetCheck();
> +    }
> +
> +    static const size_t INPUT = 0;

Nit: almost all other instructions in this file use "Input"

@@ +6181,5 @@
> +
> +    static const size_t INPUT = 0;
> +};
> +
> +class LThrowUninitializedLet : public LInstructionHelper<0, 0, 0>

s/LInstructionHelper/LCallInstructionHelper, as it always does a VM call.

@@ +6187,5 @@
> +  public:
> +    LIR_HEADER(ThrowUninitializedLet)
> +
> +    explicit LThrowUninitializedLet() {
> +    }

Nit: rm

	Shu-yu Guo [:shu] Assignee
	Comment 117 • 10 years ago

Consolidate all devtools test patches into one.

	Shu-yu Guo [:shu] Assignee
	Comment 118 • 10 years ago

Comment on attachment 8474037 [details] [diff] [review]
Part 5: Fix errors in browser/devtools/*/test/ and toolkit/devtools/*/tests/

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

Rob, would you do the honor?

See comment 37 for instructions.

	Rob Campbell [:rc] (:robcee)
	Comment 119 • 10 years ago

Comment on attachment 8474037 [details] [diff] [review]
Part 5: Fix errors in browser/devtools/*/test/ and toolkit/devtools/*/tests/

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

with pleasure. All hail the Temporal Dead Zone!

	Kevin Grandon :kgrandon
	Comment 120 • 10 years ago

Landed a gaia patch to update let usage in fake_update-checker.js: https://github.com/mozilla-b2g/gaia/commit/8732cae00450205ae23e8118668d1531055eb6f1

	Shu-yu Guo [:shu] Assignee
	Comment 127 • 10 years ago

Gavin, could I be given leeway for a mass r=me on the test fixes (all Part 5 patches)? New ones have been cropping up constantly since I started fixing these.

	Shu-yu Guo [:shu] Assignee
	Comment 128 • 10 years ago

Kevin, here's the new Gip failure. Another ID that looks like a use before def: https://tbpl.mozilla.org/php/getParsedLog.php?id=46339530&tree=Try

	Shu-yu Guo [:shu] Assignee
	Comment 129 • 10 years ago

Consolidate all part 5 test patches into one for easier review. Gavin, if you
don't want to do it please assign somebody.

	Shu-yu Guo [:shu] Assignee
	Comment 131 • 10 years ago

Please see comment 37 for instructions.