Early return when the value of test-justify is distribute, and the length of line is 0

Early return when the value of test-justify is distribute, and the length of line is 0 

There is no expansion opportunity if the length of the line is 0.
So, early return for the cases is needed to prevent buffer overflow which detected by ClusterFuzz.

This patch is to fix the crash issue which is introduced by https://src.chromium.org/viewvc/blink?view=rev&revision=186099. 

The test case is created by ClusterFuzz originally, I've tried to simplify it as much as possible.

BUG=438618
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187326

[dw.im, 2014-12-04 05:50:57]

dw.im@samsung.com changed reviewers:
+ leviw@chromium.org

[dw.im, 2014-12-04 05:50:58]

PTAL!

[dw.im, 2014-12-04 06:06:58]

dw.im@samsung.com changed reviewers:
+ steveblock@chromium.org

[dw.im, 2014-12-04 06:08:31]

dw.im@samsung.com changed reviewers:
+ pdr@chromium.org

[pdr., 2014-12-04 06:25:22]

On 2014/12/04 at 06:08:31, dw.im wrote:
> dw.im@samsung.com changed reviewers:
> + pdr@chromium.org

I don't see how the testcase in the bug would hit this since all the values are
small. Can you help me understand what's causing us to have extreme values here?

Lets go ahead and add the test to the bug too. Please clean it up as best you
can and double-check that we crash without the patch and passes with it.

[dw.im, 2014-12-04 06:48:45]

On 2014/12/04 06:25:22, pdr wrote:
> On 2014/12/04 at 06:08:31, dw.im wrote:
> > mailto:dw.im@samsung.com changed reviewers:
> > + mailto:pdr@chromium.org
> 
> I don't see how the testcase in the bug would hit this since all the values
are
> small. Can you help me understand what's causing us to have extreme values
here?
> 
> Lets go ahead and add the test to the bug too. Please clean it up as best you
> can and double-check that we crash without the patch and passes with it.

I also cannot understand the reason of this crash, as I mentioned on the bug.
I will keep trying to find it out.

[pdr., 2014-12-04 06:52:36]

On 2014/12/04 at 06:48:45, dw.im wrote:
> On 2014/12/04 06:25:22, pdr wrote:
> > On 2014/12/04 at 06:08:31, dw.im wrote:
> > > mailto:dw.im@samsung.com changed reviewers:
> > > + mailto:pdr@chromium.org
> > 
> > I don't see how the testcase in the bug would hit this since all the values
are
> > small. Can you help me understand what's causing us to have extreme values
here?
> > 
> > Lets go ahead and add the test to the bug too. Please clean it up as best
you
> > can and double-check that we crash without the patch and passes with it.
> 
> I also cannot understand the reason of this crash, as I mentioned on the bug.
> I will keep trying to find it out.

My guess is that you have an underflow occurring. You could confirm this by
asserting that length > 0.

[dw.im, 2014-12-04 09:52:00]

On 2014/12/04 06:52:36, pdr wrote:
> My guess is that you have an underflow occurring. You could confirm this by
> asserting that length > 0.

Correct guessing!
Somehow, there is line which length is 0.
So, let return 0 when the length is 0.

Also, keep the previous change - size_t.
It is needed, anyway.

[pdr., 2014-12-04 19:33:34]

pdr@chromium.org changed reviewers:
+ eae@chromium.org
- leviw@chromium.org, steveblock@chromium.org

[pdr., 2014-12-04 19:35:45]

On 2014/12/04 at 09:52:00, dw.im wrote:
> On 2014/12/04 06:52:36, pdr wrote:
> > My guess is that you have an underflow occurring. You could confirm this by
> > asserting that length > 0.
> 
> Correct guessing!
> Somehow, there is line which length is 0.
> So, let return 0 when the length is 0.
> 
> Also, keep the previous change - size_t.
> It is needed, anyway.

Excellent! I've added eae to this review as he is more familiar with this code.
Please add a test first though. Make sure to double-check that the test
crashes/fails without the patch and passes with it, and make sure the test is
well formatted and as simple as possible.

[eae, 2014-12-04 19:43:24]

The change itself looks great, I would however like to see a test case added.

[dw.im, 2014-12-05 00:29:43]

On 2014/12/04 19:43:24, eae wrote:
> The change itself looks great, I would however like to see a test case added.

I guess we can use the test case which clusterfuzz provided.
It fails without this patch, and pass with this.

BTW, how can I upload the test case? as a layouttest?

[eae, 2014-12-05 00:34:00]

On 2014/12/05 00:29:43, dw.im wrote:
> On 2014/12/04 19:43:24, eae wrote:
> > The change itself looks great, I would however like to see a test case
added.
> 
> I guess we can use the test case which clusterfuzz provided.
> It fails without this patch, and pass with this.
> 
> BTW, how can I upload the test case? as a layouttest?

If you could clean up and reduce the clusterfuzz test it would make a good
layout test.

[dw.im, 2014-12-05 00:41:47]

On 2014/12/05 00:34:00, eae wrote:
> On 2014/12/05 00:29:43, dw.im wrote:
> > On 2014/12/04 19:43:24, eae wrote:
> > > The change itself looks great, I would however like to see a test case
> added.
> > 
> > I guess we can use the test case which clusterfuzz provided.
> > It fails without this patch, and pass with this.
> > 
> > BTW, how can I upload the test case? as a layouttest?
> 
> If you could clean up and reduce the clusterfuzz test it would make a good
> layout test.

Actually, itself is very small, enough to use as layouttest. :)

[pdr., 2014-12-05 01:37:09]

On 2014/12/05 at 00:41:47, dw.im wrote:
> On 2014/12/05 00:34:00, eae wrote:
> > On 2014/12/05 00:29:43, dw.im wrote:
> > > On 2014/12/04 19:43:24, eae wrote:
> > > > The change itself looks great, I would however like to see a test case
> > added.
> > > 
> > > I guess we can use the test case which clusterfuzz provided.
> > > It fails without this patch, and pass with this.
> > > 
> > > BTW, how can I upload the test case? as a layouttest?
> > 
> > If you could clean up and reduce the clusterfuzz test it would make a good
> > layout test.
> 
> Actually, itself is very small, enough to use as layouttest. :)

Please make sure to reduce it so that it is as small and clean as possible
(i.e., remove the crazy characters if possible). Also double check that the
crash occurs without the patch and no longer crashes with it.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
is a good example to follow.

[dw.im, 2014-12-05 03:03:33]

On 2014/12/05 01:37:09, pdr wrote:
>
touched the sample little bit.
PTAL!

[kojii, 2014-12-05 06:23:22]

kojii@chromium.org changed reviewers:
+ kojii@chromium.org

[kojii, 2014-12-05 06:23:22]

https://codereview.chromium.org/777143002/diff/80001/Source/platform/fonts/Ch...
File Source/platform/fonts/Character.cpp (right):

https://codereview.chromium.org/777143002/diff/80001/Source/platform/fonts/Ch...
Source/platform/fonts/Character.cpp:345: isAfterExpansion = false;
Why do we need to set |isAfterExpansion = false| here? The code before your last
change, and the code for UChar, both do not change |isAfterExpansion| and
returns 0 if length == 0, so this line is changing the behavior of this
function.

[dw.im, 2014-12-05 07:16:42]

https://codereview.chromium.org/777143002/diff/80001/Source/platform/fonts/Ch...
File Source/platform/fonts/Character.cpp (right):

https://codereview.chromium.org/777143002/diff/80001/Source/platform/fonts/Ch...
Source/platform/fonts/Character.cpp:345: isAfterExpansion = false;
On 2014/12/05 06:23:22, koji wrote:
> Why do we need to set |isAfterExpansion = false| here? The code before your
last
> change, and the code for UChar, both do not change |isAfterExpansion| and
> returns 0 if length == 0, so this line is changing the behavior of this
> function.

Because the caller use this value after getting result.
If isAfterExpansion is true, then it is considered as there is one space at the
end of the line so that it will reduce the expansion opportunity because no need
to expand the space at the end of the line.

[kojii, 2014-12-05 13:11:21]

https://codereview.chromium.org/777143002/diff/80001/Source/platform/fonts/Ch...
File Source/platform/fonts/Character.cpp (right):

https://codereview.chromium.org/777143002/diff/80001/Source/platform/fonts/Ch...
Source/platform/fonts/Character.cpp:345: isAfterExpansion = false;
On 2014/12/05 07:16:42, dw.im wrote:
> On 2014/12/05 06:23:22, koji wrote:
> > Why do we need to set |isAfterExpansion = false| here? The code before your
> last
> > change, and the code for UChar, both do not change |isAfterExpansion| and
> > returns 0 if length == 0, so this line is changing the behavior of this
> > function.
> 
> Because the caller use this value after getting result.
> If isAfterExpansion is true, then it is considered as there is one space at
the
> end of the line so that it will reduce the expansion opportunity because no
need
> to expand the space at the end of the line.

The |isAfterExpansion| is in-out parameter, not out-only. The caller of this
function is responsible to initialize before the call, this function updates the
value, and then the caller can pass the returned value on subsequent calls.
Please refer to an example in RenderBlockLineLayout.cpp:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

It initializes |isAfterExpansion| to |true|, then call
|expansionOpportunityCount()| repeatedly on BidiRun's until it reaches to the
end. With this change, it will calculate wrong expansions if there were an empty
run.

You could also check this function before your last patch, or the UChar version
below; both do not change |isAfterExpansion| if |length| is 0.

[dw.im, 2014-12-08 07:38:46]

On 2014/12/05 13:11:21, koji wrote:
> You could also check this function before your last patch, or the UChar
version
> below; both do not change |isAfterExpansion| if |length| is 0.

Right. 
The previous logic wouldn't change the value, because it will not enter into the
loop at all.

And, it is used for the iteration for the next line as well, (e.g.
RenderBlockFlow::computeInlineDirectionPositionsForSegment)
so, not changing the value would be correct behavior.

Thanks for the valuable comment.

[dw.im, 2014-12-09 00:33:06]

PTAL!

[pdr., 2014-12-09 06:19:41]

On 2014/12/09 at 00:33:06, dw.im wrote:
> PTAL!

I'm not a text layout expert but this still doesn't seem right to me.

Your test has align=justify, display: table-cell, direction: rtl, and <wbr>.
That's quite the edge case. Are you sure this is a bug all the way down in
expansionOpportunityCount and not something higher up? It doesn't seem right
that we would need a branch in a performance-sensitive function for such an edge
case.

[dw.im, 2014-12-09 06:24:55]

On 2014/12/09 06:19:41, pdr wrote:
> On 2014/12/09 at 00:33:06, dw.im wrote:
> > PTAL!
> 
> I'm not a text layout expert but this still doesn't seem right to me.
> 
> Your test has align=justify, display: table-cell, direction: rtl, and <wbr>.
> That's quite the edge case. Are you sure this is a bug all the way down in
> expansionOpportunityCount and not something higher up? It doesn't seem right
> that we would need a branch in a performance-sensitive function for such an
edge
> case.

This edge case is the test case which ClusterFuzz use to report this issue.
And, because of this case, I've recognized that I changed some of previous
behavior of the function, expansionOpportunityCount.

[pdr., 2014-12-09 06:36:42]

On 2014/12/09 at 06:24:55, dw.im wrote:
> On 2014/12/09 06:19:41, pdr wrote:
> > On 2014/12/09 at 00:33:06, dw.im wrote:
> > > PTAL!
> > 
> > I'm not a text layout expert but this still doesn't seem right to me.
> > 
> > Your test has align=justify, display: table-cell, direction: rtl, and <wbr>.
> > That's quite the edge case. Are you sure this is a bug all the way down in
> > expansionOpportunityCount and not something higher up? It doesn't seem right
> > that we would need a branch in a performance-sensitive function for such an
edge
> > case.
> 
> This edge case is the test case which ClusterFuzz use to report this issue.
> And, because of this case, I've recognized that I changed some of previous
behavior of the function, expansionOpportunityCount.

So this is a regression from
https://src.chromium.org/viewvc/blink?view=rev&revision=186646? If so, please
update your change description to say that.

I have the same question as before though. Your previous patch fixed an edge
case with align=justify. Why do we need a branch all the way down in
expansionOpportunityCount for an edge case align=justify can only hit?

[dw.im, 2014-12-11 05:37:04]

On 2014/12/09 06:36:42, pdr wrote:
> So this is a regression from
> https://src.chromium.org/viewvc/blink?view=rev&revision=186646? If so, please
> update your change description to say that.
> 
Done.

> I have the same question as before though. Your previous patch fixed an edge
> case with align=justify. Why do we need a branch all the way down in
> expansionOpportunityCount for an edge case align=justify can only hit?
>
I've tried to restore the original algorithm not to reduce the performance.

But, still, IMO, we'd better have one branch for the specific case -
text-justify=distribute.
Having one branch is a lot better than count all of the characters of the line,
I think.

But, it depends.. if you think text-justify=distribute is too rare case, then we
can remove it, as well.

WDYT?

[kojii, 2014-12-11 07:43:27]

On 2014/12/11 05:37:04, dw.im wrote:
> On 2014/12/09 06:36:42, pdr wrote:
> > So this is a regression from
> > https://src.chromium.org/viewvc/blink?view=rev&revision=186646? If so,
please
> > update your change description to say that.
> > 
> Done.
> 
> > I have the same question as before though.
> [snip]

The commit is
https://src.chromium.org/viewvc/blink?view=revision&revision=186099, not 186646,
is that what confused you, pdr?

[pdr., 2014-12-11 07:58:28]

On 2014/12/11 at 07:43:27, kojii wrote:
> On 2014/12/11 05:37:04, dw.im wrote:
> > On 2014/12/09 06:36:42, pdr wrote:
> > > So this is a regression from
> > > https://src.chromium.org/viewvc/blink?view=rev&revision=186646? If so,
please
> > > update your change description to say that.
> > > 
> > Done.
> > 
> > > I have the same question as before though.
> > [snip]
> 
> The commit is
https://src.chromium.org/viewvc/blink?view=revision&revision=186099, not 186646,
is that what confused you, pdr?

Ahh, things make a lot more sense now! I was very confused trying to figure out
how 186646 was causing this. I now think this patch looks reasonable. I should
leave the final review to someone who understands this area of code better
though.

@kojii or @eae, does this LGTY?

[kojii, 2014-12-11 08:47:36]

https://codereview.chromium.org/777143002/diff/120001/Source/platform/fonts/C...
File Source/platform/fonts/Character.cpp (right):

https://codereview.chromium.org/777143002/diff/120001/Source/platform/fonts/C...
Source/platform/fonts/Character.cpp:350: return length - 1;
I jumped in from the mid of review discussions without reading past comments, so
ignore me if other reviewers said differently.

It looks like you reverted non-distribute case back to before the patch, I
personally like that.

This "if" is an optimization of distribute case, which is the assumption of
every call to |treatAsSpace()| returns true in the loop below. By thinking that
way, I think this is not a correct optimization. It should rather be:

isAfterExpansion = true;
return length;

Right?

There's also another edge case where |isAfterExpansion| is |false| on the entry.
In this case, you may want to return |length + 1| so that there's an expansion
opportunity before the first character, but the spec doesn't clearly state
whether there should be an expansion opportunity between non-distribute and
distribute. Since the 99% of use cases for distribute is applying to a whole
block, not to part of a block, and this suggestion applies only to such edge
cases, I suggest you ask this to www-style and then make another bug or CL if it
resolved to differently from what we have with this patch.

[dw.im, 2014-12-12 01:10:07]

On 2014/12/11 08:47:36, koji wrote:
> It looks like you reverted non-distribute case back to before the patch
> 
Yes, I did.
I changed it for performance, 
so, no reason to keep it if it is harm for performance.

> This "if" is an optimization of distribute case, which is the assumption of
> every call to |treatAsSpace()| returns true in the loop below. By thinking
that
> way, I think this is not a correct optimization. It should rather be:
> 
> isAfterExpansion = true;
> return length;
> 
> Right?
> 
I think, I can say I 'prefer' to take care of the last, or the first character
of the line,
even if text-justify is distribute.

WDYT, koji?
I believe you can give valuable comment about this, because, you are the one who
most familiar with the spec.

> There's also another edge case where |isAfterExpansion| is |false| on the
entry.
> In this case, you may want to return |length + 1| so that there's an expansion
> opportunity before the first character, but the spec doesn't clearly state
> whether there should be an expansion opportunity between non-distribute and
> distribute. Since the 99% of use cases for distribute is applying to a whole
> block, not to part of a block, and this suggestion applies only to such edge
> cases, I suggest you ask this to www-style and then make another bug or CL if
it
> resolved to differently from what we have with this patch.
>
Hopefully, as another patch.

[kojii, 2014-12-12 01:47:27]

On 2014/12/12 01:10:07, dw.im wrote:
> On 2014/12/11 08:47:36, koji wrote:
> 
> > It should rather be:
> > 
> > isAfterExpansion = true;
> > return length;
> > 
> > Right?
> > 
> I think, I can say I 'prefer' to take care of the last, or the first character
> of the line,
> even if text-justify is distribute.
> 
> WDYT, koji?
> I believe you can give valuable comment about this, because, you are the one
who
> most familiar with the spec.

Not sure what you meant here, can you elaborate a bit more?

The callers of this function assumes that you return every expansion
opportunities, including the one before the first character and after the last
character.
* They can call this function for segments of text in a line, and expansion
opportunities between segments need to be counted correctly.
* It is callers' responsibilities to handle expansion opportunities at the end
of line; i.e., they should decrement the count if |isAfterExpansion=true| at the
end of line.

In other words, I think your logic will fail when you return
|isAfterExpansion=true| and |length-1|. Try a test case where a line or a span
ends with a space, and I suppose you will get one less expansion opportunities
than needed.

> > There's also another edge case where |isAfterExpansion| is |false| on the
> entry.
> > In this case, you may want to return |length + 1| so that there's an
expansion
> > opportunity before the first character, but the spec doesn't clearly state
> > whether there should be an expansion opportunity between non-distribute and
> > distribute. Since the 99% of use cases for distribute is applying to a whole
> > block, not to part of a block, and this suggestion applies only to such edge
> > cases, I suggest you ask this to www-style and then make another bug or CL
if
> it
> > resolved to differently from what we have with this patch.
> >
> Hopefully, as another patch.

Sounds good to me.

[dw.im, 2014-12-12 02:52:56]

On 2014/12/12 01:47:27, koji wrote:
> On 2014/12/12 01:10:07, dw.im wrote:
> > On 2014/12/11 08:47:36, koji wrote:
> > 
> > > It should rather be:
> > > 
> > > isAfterExpansion = true;
> > > return length;
> > > 
> > > Right?
> > > 
> > I think, I can say I 'prefer' to take care of the last, or the first
character
> > of the line,
> > even if text-justify is distribute.
> > 
> > WDYT, koji?
> > I believe you can give valuable comment about this, because, you are the one
> who
> > most familiar with the spec.
> 
> Not sure what you meant here, can you elaborate a bit more?
> 
> The callers of this function assumes that you return every expansion
> opportunities, including the one before the first character and after the last
> character.
> * They can call this function for segments of text in a line, and expansion
> opportunities between segments need to be counted correctly.
> * It is callers' responsibilities to handle expansion opportunities at the end
> of line; i.e., they should decrement the count if |isAfterExpansion=true| at
the
> end of line.
> 
> In other words, I think your logic will fail when you return
> |isAfterExpansion=true| and |length-1|. Try a test case where a line or a span
> ends with a space, and I suppose you will get one less expansion opportunities
> than needed.
> 
Yes, the caller need to take care of it, 
so, I want to give what they expect, not just 'true'.

The value of isAfterExpansion is true when the last character can be treated as
space,
and vice versa.

And, I return 'length-1' is, there is 4 expansion opportunity if we have 5
characters.
no matter it is space or not.
If the 5th character is space, that is the thing which the "caller" need to take
care of using the value of isAfterExpansion.

[kojii, 2014-12-12 03:42:38]

On 2014/12/12 02:52:56, dw.im wrote:
> Yes, the caller need to take care of it, 
> so, I want to give what they expect, not just 'true'.
> 
> The value of isAfterExpansion is true when the last character can be treated
as
> space,
> and vice versa.

Getting closer, but a bit more. |isAfterExpansion| is not to test if the last
character is a space, but indicates whether there was an expansion opportunity
at the end of the string. The two are equal for normal justification, but when
|distribute|, there is always an expansion opportunity at the end of a string
even if the last character is not a space, so it must be always |true|.

Consider if the 5th character is not a space. There's no expansion opportunity
for normal case, but there should be an expansion opportunity if |distribute|.

You could refer to what WebKit does; it doesn't support |distribute|, but
there's a similar case of having an opportunity after the last character if the
last character is East Asian character. In that case, it is counted, and
|isAfterExpansion| becomes true.

Let me explain in another way. This is how the code should be before the
optimization:

if (direction == LTR) {
    for (size_t i = length; i > 0; --i) {
        if (treatAsSpace(characters[i - 1]) || textJustify ==
TextJustifyDistribute) {
            count++;
            isAfterExpansion = true;
        }
} else {
    for (size_t i = length; i > 0; --i) {
        if (treatAsSpace(characters[i - 1]) || textJustify ==
TextJustifyDistribute)
            count++;	
    }
}

Compare this code with your code, when the last character is not a space
character.

> And, I return 'length-1' is, there is 4 expansion opportunity if we have 5
> characters.
> no matter it is space or not.
> If the 5th character is space, that is the thing which the "caller" need to
take
> care of using the value of isAfterExpansion.

That's wrong, if there were 5 characters, there should be 5 expansion
opportunities, including the one after the last character, and let
|isAfterExpansion| be |true| to indicate that one of them is at the end of
string.

Consider a case where there's 10 characters in a line, and callers split it into
2 segments, 5 each, and call |Character::expansionOpportunityCount| for each
segment. If you return 4 for 5 characters, callers will get 8 expansion
opportunities in total, which is wrong.

You must agree that the end result should be 9. To do that,
|Character::expansionOpportunityCount| should return 5 for 5 characters, and the
caller subtract by 1 at the end of a line if |isAfterExpansion| is |true|. Does
this make sense?

[dw.im, 2014-12-12 11:12:44]

Regarding chat on IRC with koji,
I've added one more layouttest case to check distributed text jutify -
text-justify-distribute.html.

[kojii, 2014-12-15 06:59:26]

Not an owner but it looks good to me now.

eae@, PTAL?

[inferno, 2014-12-15 13:09:09]

inferno@chromium.org changed reviewers:
+ leviw@chromium.org

[eae, 2014-12-16 22:11:02]

LGTM

[eae, 2014-12-16 22:11:15]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2014-12-16 22:11:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/777143002/160001

[commit-bot: I haz the power, 2014-12-16 22:12:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-16 22:12:21]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 1463.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
1b85d3cf59577c8c6d29c55384796cd0966cbb78..e4db535b7252afe1369bd030c01af8d198033723
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1463,3 +1463,5 @@ crbug.com/441736 svg/as-image/svg-invalid-image-1.html [
Pass Failure ]
 crbug.com/441738 inspector/extensions/extensions-network.html [ Pass Failure ]
 
 crbug.com/441743 [ Mac Win ] plugins/js-from-destroy.html [ Timeout Crash ]
+
+crbug.com/438618 fast/css3-text/css3-text-justify/text-justify-distribute.html
[ NeedsRebaseline ]

[dw.im, 2014-12-17 00:21:48]

On 2014/12/16 22:12:21, I haz the power (commit-bot) wrote:
> Failed to apply patch for LayoutTests/TestExpectations:
> While running patch -p1 --forward --force --no-backup-if-mismatch;
>   patching file LayoutTests/TestExpectations
>   Hunk #1 FAILED at 1463.
>   1 out of 1 hunk FAILED -- saving rejects to file
> LayoutTests/TestExpectations.rej
> 
> Patch:       LayoutTests/TestExpectations
> Index: LayoutTests/TestExpectations
> diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
> index
>
1b85d3cf59577c8c6d29c55384796cd0966cbb78..e4db535b7252afe1369bd030c01af8d198033723
> 100644
> --- a/LayoutTests/TestExpectations
> +++ b/LayoutTests/TestExpectations
> @@ -1463,3 +1463,5 @@ crbug.com/441736 svg/as-image/svg-invalid-image-1.html [
> Pass Failure ]
>  crbug.com/441738 inspector/extensions/extensions-network.html [ Pass Failure
]
>  
>  crbug.com/441743 [ Mac Win ] plugins/js-from-destroy.html [ Timeout Crash ]
> +
> +crbug.com/438618
fast/css3-text/css3-text-justify/text-justify-distribute.html
> [ NeedsRebaseline ]

will rebase now.

[dw.im, 2014-12-17 00:52:54]

The CQ bit was checked by dw.im@samsung.com

[commit-bot: I haz the power, 2014-12-17 00:53:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/777143002/180001

[commit-bot: I haz the power, 2014-12-17 03:29:20]

Committed patchset #10 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187326