Bug 171265 – test262: Completion values for control flow do not match the spec

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Joseph Pecoraro]

test262: Completion values for control flow do not match the spec

Lots of test262 tests with "cptn" in the name fail.

Example:

    assert( eval("1; if(false);") === undefined )
    assert( eval("1; while(false);") === undefined )
    assert( eval("1; try{}catch(e){};") === undefined )
    ...
    assert( eval("0; try {throw 3} catch (e) {2} finally {1}") === 2 )

Expected:
* Most control flow cases return undefined when no statement executes, otherwise the statements inside them
* Finally is a bit whacky in that its value is not used, the try (or catch) value is used instead

Actual:
* JSC produces 1 for each of the productions above

Notes:
* Chrome seems to match the spec for each of these
* Firefox matches us for a few (finally, empty blocks)
* Not all statements produce values (variable declarations).

Spec:

Read through each of the different statement runtime semantics to see their Completion Values:
https://tc39.github.io/ecma262/#sec-statement-semantics

The Completion Record which is the foundation for each of these: (NormalCompletion, ReturnIfAbrupt, empty value, ...)
https://tc39.github.io/ecma262/#sec-completion-record-specification-type

Control flow tends to use UpdateEmpty. Most start out prepared to return the "undefined" value, and that only gets replaced if statements produce non-empty values:
https://tc39.github.io/ecma262/#sec-updateempty

[Joseph Pecoraro]

Created attachment 308077 [details]
[PATCH] Work in Progress

So this is the work in progress. It passes all of the "cptn" tests I could find in test262.

A few concerns I have to test and think through more:

  * performance
    - emits some extra stores of undefined in global code / eval code, how much of an impact is it
    - technically in a browser only eval code produces a result, program/module are never seen.
    - however in the jsc command line REPL it is most natural for each line to be a program and get this behavior
  * bytecode caching
    - the store byte codes are only emitted outside of functions, does this interact well with our caches?
  * compatibility
    - eval / JSC APIs (JSEvaluateScript, etc) can return different values now. They should be "more correct" but still different.

[Build Bot]

Comment on attachment 308077 [details]
[PATCH] Work in Progress

Attachment 308077 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3600608

New failing tests:
mozilla-tests.yaml/ecma/Statements/12.6.3-2.js.mozilla-ftl-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma/Statements/12.6.3-2.js.mozilla-llint
mozilla-tests.yaml/ecma/Statements/12.6.3-2.js.mozilla
mozilla-tests.yaml/ecma/Statements/12.6.3-2.js.mozilla-baseline
mozilla-tests.yaml/ecma/Statements/12.6.3-2.js.mozilla-no-ftl
mozilla-tests.yaml/ecma/Statements/12.6.3-2.js.mozilla-dfg-eager-no-cjit-validate-phases

[Build Bot]

Comment on attachment 308077 [details]
[PATCH] Work in Progress

Attachment 308077 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3600734

New failing tests:
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A9.html
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A9.1.html

[Build Bot]

Created attachment 308083 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308077 [details]
[PATCH] Work in Progress

Attachment 308077 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3600740

New failing tests:
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A9.html
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A9.1.html

[Build Bot]

Created attachment 308084 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308077 [details]
[PATCH] Work in Progress

Attachment 308077 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3600743

New failing tests:
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A9.html
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A9.1.html

[Build Bot]

Created attachment 308086 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308077 [details]
[PATCH] Work in Progress

Attachment 308077 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3600766

New failing tests:
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A9.html
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A9.1.html

[Build Bot]

Created attachment 308088 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

Created attachment 308218 [details]
[PATCH] Work in Progress

This should pass all tests as I rebaselined the last. Going to do performance next.

[Joseph Pecoraro]

>   * performance
>     - emits some extra stores of undefined in global code / eval code, how much of an impact is it
>     - technically in a browser only eval code produces a result, program/module are never seen.
>     - however in the jsc command line REPL it is most natural for each line to be a program and get this behavior

run-jsc-benchmarks shows negligible results, which would be expected since most of these run code in functions not global code. I don't have JSBench, that might be better to test instead of the OpenSource benchmarks like Sunspider.

>   * bytecode caching
>     - the store byte codes are only emitted outside of functions, does this interact well with our caches?

The CodeCache takes the CodeType into account already.

>   * compatibility
>     - eval / JSC APIs (JSEvaluateScript, etc) can return different values now. They should be "more correct" but still different.

This is the price of moving forward. I think it should be rare for users to have some code after the last expression and expect that not to impact the result.

[Joseph Pecoraro]

Created attachment 308229 [details]
[PATCH] Proposed Fix

Reviewable!

[Saam Barati]

Comment on attachment 308229 [details]
[PATCH] Proposed Fix

I talked to Joe in person and suggested he tries a different approach of determining statically the last statement in a statement list that produces a value.

[Joseph Pecoraro]

> I talked to Joe in person and suggested he tries a different approach of
> determining statically the last statement in a statement list that produces
> a value.

I'll need to do some reading to be sure this can be determined completely statically. My guess is yes it can.

For instance a block statement may produce empty (if no statement list) or return the completion value of a statement in the list:
https://tc39.github.io/ecma262/#sec-block-runtime-semantics-evaluation

So a StatementNode::hasCompletionValue for BlockNode might then have to walk its statements recursively. But I think the point (especially with control flow statements producing undefined or a value) is that it should be possible to determine statically.

[Joseph Pecoraro]

After writing a bunch of my own tests I found some issues in other browsers. New test262 tests to cover the differences:
https://github.com/tc39/test262/pull/1012

[Joseph Pecoraro]

Created attachment 308662 [details]
[PATCH] Proposed Fix v2

This takes the approach of statically determining (as best we can) the last statement that can produce a completion value.

[Joseph Pecoraro]

Comment on attachment 308662 [details]
[PATCH] Proposed Fix v2

View in context: https://bugs.webkit.org/attachment.cgi?id=308662&action=review

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:879
> +        bool shouldBeConcernedWithCompletionValue() { return m_codeType != FunctionCode; }

This could be `const`.

[Build Bot]

Comment on attachment 308662 [details]
[PATCH] Proposed Fix v2

Attachment 308662 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3633639

New failing tests:
stress/super-get-by-id.js.dfg-maximal-flush-validate-no-cjit
stress/super-get-by-id.js.ftl-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-dfg-eager-no-cjit
stress/super-get-by-id.js.default
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-no-ftl
stress/super-get-by-id.js.ftl-no-cjit-no-put-stack-validate
stress/super-get-by-id.js.dfg-eager-no-cjit-validate
stress/super-get-by-id.js.ftl-eager-no-cjit
stress/super-get-by-id.js.no-cjit-collect-continuously
stress/super-get-by-id.js.ftl-eager-no-cjit-b3o1
stress/super-get-by-id.js.ftl-no-cjit-no-inline-validate
stress/super-get-by-id.js.no-llint
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-ftl-eager-no-cjit
stress/super-get-by-id.js.ftl-no-cjit-validate-sampling-profiler
stress/super-get-by-id.js.dfg-eager
stress/super-get-by-id.js.no-ftl
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-ftl-no-cjit
stress/super-get-by-id.js.no-cjit-validate-phases
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout
stress/super-get-by-id.js.ftl-no-cjit-small-pool
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-no-llint
stress/super-get-by-id.js.ftl-eager

[Joseph Pecoraro]

Created attachment 308678 [details]
[PATCH] Proposed Fix v2

[Saam Barati]

Comment on attachment 308678 [details]
[PATCH] Proposed Fix v2

View in context: https://bugs.webkit.org/attachment.cgi?id=308678&action=review

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2466
> +        if (statement == lastStatementWithCompletionValue)

If we do this here, why do we need to load undefined in a few of the various places you added it like with/if-else, etc?

[Joseph Pecoraro]

(In reply to Saam Barati from comment #21)
> Comment on attachment 308678 [details]
> [PATCH] Proposed Fix v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308678&action=review
> 
> > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2466
> > +        if (statement == lastStatementWithCompletionValue)
> 
> If we do this here, why do we need to load undefined in a few of the various
> places you added it like with/if-else, etc?

This is because a with/if/else can include a break statement. In those cases it may break out of its statement list without having set some completion value. I'll investigate this a bit more and provide a clearer answer.

[Joseph Pecoraro]

> I'll investigate this a bit more and provide a clearer answer.

IfStatement:
https://tc39.github.io/ecma262/#sec-if-statement-runtime-semantics-evaluation

> 13.6.7 Runtime Semantics: Evaluation
> 
> 1. Let exprRef be the result of evaluating Expression.
> 2. Let exprValue be ToBoolean(? GetValue(exprRef)).
> 3. If exprValue is false, then
>     a. Return NormalCompletion(undefined).
> 4. Else,
>     a. Let stmtCompletion be the result of evaluating Statement.
> 5. Return Completion(UpdateEmpty(stmtCompletion, undefined)).

BlockStatement:
https://tc39.github.io/ecma262/#sec-block-runtime-semantics-evaluation

> 13.2.13 Runtime Semantics: Evaluation
> 
> Block : { StatementList }
> 
> 1. Let oldEnv be the running execution context's LexicalEnvironment.
> 2. Let blockEnv be NewDeclarativeEnvironment(oldEnv).
> 3. Perform BlockDeclarationInstantiation(StatementList, blockEnv).
> 4. Set the running execution context's LexicalEnvironment to blockEnv.
> 5. Let blockValue be the result of evaluating StatementList.
> 6. Set the running execution context's LexicalEnvironment to oldEnv.
> 7. Return blockValue.
> 
> 
> StatementList : StatementList StatementListItem
> 
> 1. Let sl be the result of evaluating StatementList.
> 2. ReturnIfAbrupt(sl).
> 3. Let s be the result of evaluating StatementListItem.
> 4. Return Completion(UpdateEmpty(s, sl)).
> 
> NOTE 2
> The value of a StatementList is the value of the last value producing item in the StatementList.
> For example, the following calls to the eval function all return the value 1:
> 
>     eval("1;;;;;")
>     eval("1;{}")
>     eval("1;var a;")


BreakStatement:
https://tc39.github.io/ecma262/#sec-break-statement-runtime-semantics-evaluation

> 13.9.3 Runtime Semantics: Evaluation
> 
> BreakStatement : break;
> 
> 1. Return Completion{[[Type]]: break, [[Value]]: empty, [[Target]]: empty}.

---

So take this case from:
test262/test/language/statements/if/cptn-else-true-abrupt-empty.js

    eval('1; do { 2; if (true) { break; } else { }; 3 } while (false)')

The AST roughly breaks down as:

    - Program
      - SourceElements
        - ExpressionStatement - '1;'
        - DoWhileStatement
          - BlockStatement
            - ExpressionStatement - '2'
            - IfStatement
              - Expression - 'true'
              - BlockStatement
                - BreakStatement - 'break'
            - ExpressionStatement - '3'
          - Condition - 'false'

The last statement of the global code to have a completion value is the DoWhileStatement. It should produce undefined or the completion value of its block.

When we generate bytecode for the block we detect the last statement is the Expression Statement '3' so we'll load an undefined before evaluating that expression statement.

However, while executing the block the `3` is not the last statement to execute. The block evaluate the `2`. Then when evaluating the if expression, which should produce a value or `undefined`, we `break`. This break actually takes us out of the do/while. So the do/while's BlockStatement got its completion value from the last statement to execute (the IfStatement).

For this to be correct something needs to produce the `undefined` here. So that is why the load is needed.

---

This boils down to:

A BreakableStatement (loops and switch) may get its completion value from something other than the last statement in its BlockStatement due to the break statement. In those cases, statements which need to produce value or undefined (namely IfElse, With, or TryCatch) need to produce the undefineds that are expected of it.

---

This might still be statically detectable, but it is starting to no longer be clean / straightforward. I think we could do this by only generating the undefined load inside a IfElse/With/Try if it (1) contains a break statement and (2) no statement before the break statement has a completion value. Something like a "hasEarlyBreak".

Which of course means walking into sub-blocks and over statements:

    do {
        1;
        if (true) {
            var emptyCompletionValue;
            {
                { }
                function foo() {};
                debugger;
                break;
            }
        }
        2;
    } while (false);

I'm not sure how deep we want to go. The complexity of this approach might not be warranted.

[Joseph Pecoraro]

Comment on attachment 308678 [details]
[PATCH] Proposed Fix v2

View in context: https://bugs.webkit.org/attachment.cgi?id=308678&action=review

> JSTests/stress/completion-value.js:190
> +shouldBe(eval(`99; do { try { break; } catch (e) { -1 } } while (false);`), undefined);

A slight modification to this causes it to fail:

    shouldBe(eval(`99; do { try { break; } catch (e) { -1 } } while (false);`), undefined); // pass
    shouldBe(eval(`99; do { -2; try { break; } catch (e) { -1 }; -3 } while (false);`), undefined); // fails with -2

We would need the loads of undefined like we did with IfElse/With.

[Saam Barati]

(In reply to Joseph Pecoraro from comment #24)
> Comment on attachment 308678 [details]
> [PATCH] Proposed Fix v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308678&action=review
> 
> > JSTests/stress/completion-value.js:190
> > +shouldBe(eval(`99; do { try { break; } catch (e) { -1 } } while (false);`), undefined);
> 
> A slight modification to this causes it to fail:
> 
>     shouldBe(eval(`99; do { try { break; } catch (e) { -1 } } while
> (false);`), undefined); // pass
>     shouldBe(eval(`99; do { -2; try { break; } catch (e) { -1 }; -3 } while
> (false);`), undefined); // fails with -2
> 
> We would need the loads of undefined like we did with IfElse/With.
Does your first proposed fix also break?

[Joseph Pecoraro]

Created attachment 311422 [details]
[PATCH] Proposed Fix v3

Extended tests to cover more cases. More checks are done statically to avoid outputting the `undefined` in If/With/Try cases.

[Build Bot]

Comment on attachment 311422 [details]
[PATCH] Proposed Fix v3

Attachment 311422 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3829072

New failing tests:
http/tests/cache/cancel-during-revalidation-succeeded.html
webrtc/peer-connection-audio-mute.html
fast/css/target-fragment-match.html

[Build Bot]

Created attachment 311426 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Joseph Pecoraro]

Comment on attachment 311422 [details]
[PATCH] Proposed Fix v3

View in context: https://bugs.webkit.org/attachment.cgi?id=311422&action=review

> Source/JavaScriptCore/parser/Nodes.cpp:83
> +bool SourceElements::hasEarlyBreak() const

This isn't enough, it needs to allow for continue as well. For example:

    assert.sameValue(
      eval('12; do { 13; if (true) { continue; } 14; } while (false)'), undefined
    );

[Joseph Pecoraro]

Created attachment 311430 [details]
[PATCH] Proposed Fix v3

[Saam Barati]

Comment on attachment 311430 [details]
[PATCH] Proposed Fix v3

View in context: https://bugs.webkit.org/attachment.cgi?id=311430&action=review

r=me

> JSTests/mozilla/mozilla-tests.yaml:1035
> +  cmd: defaultRunMozillaTest :failDueToOutdatedOrBadTest, "../shell.js"

my favorite kind of failure.

> JSTests/stress/completion-value.js:20
> +// Statements that produce an (empty) completion value do not affect results:

This test is great. Maybe it's so good we should give it to test262 if they want it?

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2593
> +    if (generator.shouldBeConcernedWithCompletionValue()) {

So this is the completion value of the "if" itself? Would it work just to put this logic in SourceElements::emitBytecode?

Perhaps another way to phrase what I'm asking: If we have an earlyBreakOrContinue(), are there places we actually get to skip caring about the return value? Maybe it's worth consolidating this logic in SourceElements::emitBytecode just for cleanliness?

I'm leaving it up to you since you understand the problem here much better than I do.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3345
> +    if (generator.shouldBeConcernedWithCompletionValue() && m_tryBlock->hasEarlyBreakOrContinue())

ditto

> Source/JavaScriptCore/parser/Nodes.cpp:83
> +bool SourceElements::hasEarlyBreakOrContinue() const

nice!

[Joseph Pecoraro]

Comment on attachment 311430 [details]
[PATCH] Proposed Fix v3

View in context: https://bugs.webkit.org/attachment.cgi?id=311430&action=review

>> JSTests/stress/completion-value.js:20
>> +// Statements that produce an (empty) completion value do not affect results:
> 
> This test is great. Maybe it's so good we should give it to test262 if they want it?

They have a bunch of the trickier ones, but there are some that they don't have. I added a bunch they were missing and other browsers were failing:
https://github.com/tc39/test262/pull/1012

I can search and individually add others. They have a nice naming convention, but it just means a lot of small tests in a lot of different places. Its work, but kind of nice in the end.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2593
>> +    if (generator.shouldBeConcernedWithCompletionValue()) {
> 
> So this is the completion value of the "if" itself? Would it work just to put this logic in SourceElements::emitBytecode?
> 
> Perhaps another way to phrase what I'm asking: If we have an earlyBreakOrContinue(), are there places we actually get to skip caring about the return value? Maybe it's worth consolidating this logic in SourceElements::emitBytecode just for cleanliness?
> 
> I'm leaving it up to you since you understand the problem here much better than I do.

Yes, this is the completion value of the if statement itself. It'll either produce undefined or the last completion value of its block.

Earlier I had determined we could put this just inside of If/With/Try which should allow us to skip putting it in While/For/Do. The idea being that all of these have a completion value of undefined || value and we aim to only produce the undefined if this is the last statement that can include a completion value. If an If/With/Try is nested inside of a While/For/Do then its completion value can change based on whether or not the if is reached.

I'll try some other cases I just thought of to confirm this a bit more. I've since forgotten relevant parts of the spec. Note to self, test these cases:

    99; do { 1; do { break; 2; } while (false); } while (false);
    99; do { 1; do { break; 2; } while (false); 3; } while (false);
    99; label: do { 1; do { break label; 2; } while (false); } while (false);
    99; label: do { 1; do { break label; 2; } while (false); 3; } while (false);

[Saam Barati]

Comment on attachment 311430 [details]
[PATCH] Proposed Fix v3

View in context: https://bugs.webkit.org/attachment.cgi?id=311430&action=review

>>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2593
>>> +    if (generator.shouldBeConcernedWithCompletionValue()) {
>> 
>> So this is the completion value of the "if" itself? Would it work just to put this logic in SourceElements::emitBytecode?
>> 
>> Perhaps another way to phrase what I'm asking: If we have an earlyBreakOrContinue(), are there places we actually get to skip caring about the return value? Maybe it's worth consolidating this logic in SourceElements::emitBytecode just for cleanliness?
>> 
>> I'm leaving it up to you since you understand the problem here much better than I do.
> 
> Yes, this is the completion value of the if statement itself. It'll either produce undefined or the last completion value of its block.
> 
> Earlier I had determined we could put this just inside of If/With/Try which should allow us to skip putting it in While/For/Do. The idea being that all of these have a completion value of undefined || value and we aim to only produce the undefined if this is the last statement that can include a completion value. If an If/With/Try is nested inside of a While/For/Do then its completion value can change based on whether or not the if is reached.
> 
> I'll try some other cases I just thought of to confirm this a bit more. I've since forgotten relevant parts of the spec. Note to self, test these cases:
> 
>     99; do { 1; do { break; 2; } while (false); } while (false);
>     99; do { 1; do { break; 2; } while (false); 3; } while (false);
>     99; label: do { 1; do { break label; 2; } while (false); } while (false);
>     99; label: do { 1; do { break label; 2; } while (false); 3; } while (false);

It might not be necessary. Perhaps we should just land what you have, I think the patch is elegant as is.

[Joseph Pecoraro]

> I'll try some other cases I just thought of to confirm this a bit more. I've
> since forgotten relevant parts of the spec. Note to self, test these cases:
> 
>     99; do { 1; do { break; 2; } while (false); } while (false);
>     99; do { 1; do { break; 2; } while (false); 3; } while (false);
>     99; label: do { 1; do { break label; 2; } while (false); } while (false);
>     99; label: do { 1; do { break label; 2; } while (false); 3; } while
> (false);

Yeah I was incorrect. We'll need to do this for do/for/while as well.

The last case produces `3` with this patch but should have been `undefined`.

All these smarts and I think we'll end up close to the original patch with some logic to avoid outputting a few bytecodes in some cases.

[Joseph Pecoraro]

> The last case produces `3` with this patch but should have been `undefined`.

Err it produced `1` and should have been `undefined`. :)

[Joseph Pecoraro]

Created attachment 313268 [details]
[PATCH] Proposed Fix v4

[WebKit Commit Bot]

Comment on attachment 313268 [details]
[PATCH] Proposed Fix v4

Clearing flags on attachment: 313268

Committed r218512: <http://trac.webkit.org/changeset/218512>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 173981

[Joseph Pecoraro]

Created attachment 319885 [details]
[PATCH] Rebaselined

The patch re-applied without any issue. I re-ran tests and things look good. I'll see what the EWS bots think and then cq this.

[WebKit Commit Bot]

Comment on attachment 319885 [details]
[PATCH] Rebaselined

Clearing flags on attachment: 319885

Committed r221622: <http://trac.webkit.org/changeset/221622>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34694423>