New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 599427 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug-Regression

Blocking:
issue 309849
issue 440500



Sign in to add a comment

xsl translate method do not work properly

Reported by serj.ale...@gmail.com, Mar 31 2016

Issue description

UserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2694.1 Safari/537.36

Steps to reproduce the problem:
You just need to execute xsl transformation with translate method in it, like in repro-page bellow.

What is the expected behavior?
Expected result is "Simple_sting_line" (all spaces should be replaced by underscores).
It works fine in Stable Chrome (49.0.2623.110 m)

What went wrong?
The actual result is "Siimpmplple le e s_" which makes no sense at all

Did this work before? Yes 49.0.2623.110 m

Chrome version: 51.0.2694.1  Channel: canary
OS Version: 10.0
Flash Version: Shockwave Flash 21.0 r0

Simple reproducible page: https://jsfiddle.net/9r1nLumk/1/
 
issue.html
1.1 KB View Download
Components: -Blink Blink>XML
Labels: -Type-Bug M-51 Type-Bug-Regression
Owner: erikc...@chromium.org
Status: Assigned (was: Unconfirmed)
Able to reproduce the issue on Windows 7 using 51.0.2694.1, latest canary 51.0.2696.0 as per steps in comment #0.

This is regression issue broken in M-51.

Please find below bisect info:

Last good build:51.0.2674.0
First bad build:51.0.2675.0

MANUAL CHANGE LOG:
https://chromium.googlesource.com/chromium/src/+log/51.0.2674.0..51.0.2675.0?pretty=fuller&n=10000

From above CL, suspecting below:
https://chromium.googlesource.com/chromium/src/+/b39bbac875e4ee4f041058b95b03d4c2cc95c501

erikchen@Could you please look into this issue if it is related to your change, else feel free to assign it to an appropriate dev person.

Owner: ssamanoori@chromium.org
my CL only affects Mac.
Owner: ----
Status: Untriaged (was: Assigned)

Comment 5 by kojii@chromium.org, Apr 11 2016

Cc: kojii@chromium.org
Labels: Needs-Bisect
Could we get a narrower bisect? 324 CLs are a bit hard to find the suspect.
Cc: hirosh...@chromium.org
Labels: -Needs-Bisect
Re Bisected the issue,again tool is invoking all good builds.
Not able to find the exact suspect though suspecting the below Change might be related to this issue.Hence ccing the dev from the below Review URL.
https://codereview.chromium.org/1274063003

hiroshige@Could you please look into this issue,else please help us in finding the appropriate dev person.


Unable to reproduce the issue on Mac 10.10.5, Ubuntu 14.04.
My change seems unrelated (it touches core/xmlhttprequest but not core/xml).

> Re Bisected the issue,again tool is invoking all good builds.
Is this done on Windows?

Comment 9 by kojii@chromium.org, Apr 12 2016

Cc: tkent@chromium.org
tkent@, could you advice?

I extended the last good build to my Canary 51.0.2704.2 and ran this on my Win7:
>python tools\bisect-builds.py -a win -b 386523 -g 380800 --use-local-cache -- --no-first-run --user-data-dir=/tmp https://jsfiddle.net/9r1nLumk/1/

All archives in the bisect pass the test, resulting:

You are probably looking for a change made after 386506 (known good), but no later than 386515 (first known bad).
CHANGELOG URL:
  https://chromium.googlesource.com/chromium/src/+log/378332aefbad9649983a2e886945f8db5cccde68..f9f08737b2b5f24a29f3b689d5e81e271f9ce8bf

Is it possible that released Canary fails but all archives pass?

Comment 10 by tkent@chromium.org, Apr 12 2016

It looks like a memory corruption, and depends on memory layout?
We might set different compiler flags in Google Chrome Canary and continuous build.

Someone should try to reproduce this with a local official build.

Comment 11 by kojii@chromium.org, Apr 12 2016

Cc: -hirosh...@chromium.org
Owner: brucedaw...@chromium.org
brucedawson@, I have difficulty to find the suspect on this issue. The current situation is that:

* First bad build is 51.0.2675.0 as per comment #2.
* Reproduces on Canary.
* bisect-builds.py does not reproduce.
* Local content_shell does not reproduce.

So the possibility is the switch to VS 2015 and we hit a compiler issue.

Could you advice?
Status: Started (was: Untriaged)
kojii@ - in comment #9 you gave a changelog URL that you said might be the problem, but those are changes from this week, whereas this bug was reported 12 days ago.

It looks like (according to comment #2) the bug appeared between March 11th/12th but was not noticed until the 31st, so VS 2015 is definitely a prime suspect.

It's still odd that a bisect didn't work because crossing the compiler switch line while running the bisect should have switched compilers.

I will test local official builds with VS 2013 and VS 2015.

Comment 13 by kojii@chromium.org, Apr 12 2016

Yeah, sorry, that was confusing.

From comment #2:

> Last good build:51.0.2674.0
> First bad build:51.0.2675.0
> 
> MANUAL CHANGE LOG:
> https://chromium.googlesource.com/chromium/src/+log/51.0.2674.0..51.0.2675.0?pretty=fuller&n=10000

I believe this is accurate and this range contains the switch to VS 2015.

But all bisect do not reproduce, in comment #6 and #9, so the bisect is unlikely to be able to find the culprit.

After talking to tkent@, since bisect pulls builds from try-bots archive, our best guess is that the bug reproduces only on official builds. Neither of us were very familiar with what flags to use for official builds, and our best guess it to consult with you for how to investigate further.
Getting official builds is just a matter of putting this in GYP_DEFINES:

    buildtype=Official

Then you run gclient runhooks or python build\gyp_chromium to regenerate the ninja files.

With gn the equivalent is "is_official_build = true"

It is also vaguely possible that chrome branding affects the result. That is set in gyp builds with:

    branding=Chrome

and in gn builds with "is_chrome_branded = true".

It is far more likely that buildtype=Official is the trigger, so that is what I am building with. I'm building ToT with VS 2013 and VS 2015 so that I can compare. Wish me luck.

It makes sense for me to investigate this because I have the builds pretty much finished, and because I have some experience with tracking down and reporting compiler bugs:

https://randomascii.wordpress.com/2016/03/24/compiler-bugs-found-when-porting-chromium-to-vc-2015/
Code-gen bug confirmed. I have a minimal repro and various options for workarounds. CL and bug to follow.

The bad code-gen occurs in xmlUTF8Strsize - the initial check for the high-bit being set is omitted so the function returns how many subsequent bits are set. That is, this test:

    if ((ch = *ptr++) & 0x80)

is optimized away.

Once this is understood then the behavior makes more sense. This translate call:

    translate("123456ABCDEFabcdef"," ","_")

returns:

    123456ABBCCDDEEFFaabcbcdcdedefeff

The length returned for digits (0x30 to 0x39) is correct because the 0x40 bit is not set.

The length returned for upper-case letters (0x41 to 0x5A) is two because 0x40 is set but 0x20 is not.

The length returned for initial lower-case characters (0x61 to 0x6f) is 3 because two consecutive set bits are found. Later lower-case letters return increasingly large lengths.

A ~ character (0x7E) returns a length of 7 and causes the next six characters to be copied.

Project Member

Comment 17 by bugdroid1@chromium.org, Apr 13 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/81e7bce94d50b04b1c1119e0fe460396dd1302fe

commit 81e7bce94d50b04b1c1119e0fe460396dd1302fe
Author: brucedawson <brucedawson@chromium.org>
Date: Wed Apr 13 01:40:42 2016

Workaround for VC++ 2015 Update 2 code-gen bug

The xsl transformation function 'translate' was broken by VS 2015 due to
a code-gen bug, reported to Microsoft as:
https://connect.microsoft.com/VisualStudio/feedback/details/2582138

The simplest workaround is to optimize for time instead of size in the
affected function.

TEST=Go to https://jsfiddle.net/9r1nLumk/1/ and confirm that the output
is "Simple_sting_line"
BUG= 599427 

Review URL: https://codereview.chromium.org/1878963005

Cr-Commit-Position: refs/heads/master@{#386890}

[modify] https://crrev.com/81e7bce94d50b04b1c1119e0fe460396dd1302fe/third_party/libxml/README.chromium
[modify] https://crrev.com/81e7bce94d50b04b1c1119e0fe460396dd1302fe/third_party/libxml/src/xmlstring.c

Hi all,
Thanks for investigation, this issue is critical for our product.

If I understood right the issue appears because of new build tools you start using. Does that mean that next stable build, which will build using the same tools, will contain this defect as well?

Sergii
My workaround for this bug will have to be integrated to the M51 branch. I will let the fix sit for a few days before using the Merge-Request-51 to request the integration.

I just tested with the latest canary and it still has the bug because my change just missed the cutoff. Look for a fix in the next canary build.

Note that I have seen the bug in both 32-bit and 64-bit builds.
Labels: Merge-Request-51
Today's canary (52.0.2708.0 canary (64-bit)) has the fix. The result is now "Simple_sting_line", as it should be.

Tagging as Merge-Request-51 because we need to merge this fix in order to avoid shipping a broken xsl translate method to stable.

Comment 21 by tin...@google.com, Apr 14 2016

Labels: -Merge-Request-51 Merge-Approved-51 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M51 (branch: 2704)
Project Member

Comment 22 by bugdroid1@chromium.org, Apr 14 2016

Labels: -merge-approved-51 merge-merged-2704
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8a902bf6f6601e665e9ef656176f5497bdc06d97

commit 8a902bf6f6601e665e9ef656176f5497bdc06d97
Author: Bruce Dawson <brucedawson@chromium.org>
Date: Thu Apr 14 21:05:57 2016

Workaround for VC++ 2015 Update 2 code-gen bug

The xsl transformation function 'translate' was broken by VS 2015 due to
a code-gen bug, reported to Microsoft as:
https://connect.microsoft.com/VisualStudio/feedback/details/2582138

The simplest workaround is to optimize for time instead of size in the
affected function.

TEST=Go to https://jsfiddle.net/9r1nLumk/1/ and confirm that the output
is "Simple_sting_line"
BUG= 599427 

Review URL: https://codereview.chromium.org/1878963005

Cr-Commit-Position: refs/heads/master@{#386890}
(cherry picked from commit 81e7bce94d50b04b1c1119e0fe460396dd1302fe)

Review URL: https://codereview.chromium.org/1886903003 .

Cr-Commit-Position: refs/branch-heads/2704@{#62}
Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251}

[modify] https://crrev.com/8a902bf6f6601e665e9ef656176f5497bdc06d97/third_party/libxml/README.chromium
[modify] https://crrev.com/8a902bf6f6601e665e9ef656176f5497bdc06d97/third_party/libxml/src/xmlstring.c

Blocking: 440500
Cc: rnimmagadda@chromium.org
Labels: TE-Verified-M51 TE-Verified-51.0.2704.19
Verified the fix on Windows 7, MAC (10.11.4) & Ubuntu Trusty (14.04) for Google Chrome Dev Version - 51.0.2704.19

Screen-recording is attached.

TE-Verified labels are added. 
Screen Shot 2016-04-19 at 10.47.02 AM.png
24.8 KB View Download
Blocking: 309849
Project Member

Comment 26 by bugdroid1@chromium.org, Apr 20 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e2c0c25fa28142b5064e0fc9d9ae8dd749352bbe

commit e2c0c25fa28142b5064e0fc9d9ae8dd749352bbe
Author: stanisc <stanisc@chromium.org>
Date: Wed Apr 20 23:27:35 2016

Added XSLT translate test

This test is for detecting an error caused by a compiler
bug on Windows (which is now worked around by
crrev.com/1878963005).

I've verified that the test is failing without the
workaround by temporarily undoing it (by switching the optimization from speed to size).

BUG= 599427 

Review URL: https://codereview.chromium.org/1897383004

Cr-Commit-Position: refs/heads/master@{#388594}

[add] https://crrev.com/e2c0c25fa28142b5064e0fc9d9ae8dd749352bbe/third_party/WebKit/LayoutTests/fast/xsl/xslt-translate-expected.txt
[add] https://crrev.com/e2c0c25fa28142b5064e0fc9d9ae8dd749352bbe/third_party/WebKit/LayoutTests/fast/xsl/xslt-translate.html

Project Member

Comment 27 by bugdroid1@chromium.org, Apr 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/776e8a67c0e7fe1cf9399605f857aa93daf4c1ed

commit 776e8a67c0e7fe1cf9399605f857aa93daf4c1ed
Author: brucedawson <brucedawson@chromium.org>
Date: Fri Apr 22 15:49:26 2016

Alternate workaround for VS 2015 Update 2 code-gen bug

The xsl transformation function 'translate' was broken by VS 2015 due to
a code-gen bug, reported to Microsoft as:
https://connect.microsoft.com/VisualStudio/feedback/details/2582138

After getting feedback from Microsoft on the bug it became clear that
the original workaround was not stable, and could fail in the face of
PGO builds which change the optimization settings.

This is the recommended fix - introducing a copy of 'ch'.

This change also fixes some indentation errors caused by mixed tabs and
spaces in order to make it easier to see the correctness of the new
scoping.

There is now a layout test to check for this bug so no manual testing is
required for this CL.

BUG= 599427 , 440500 

Review URL: https://codereview.chromium.org/1911853002

Cr-Commit-Position: refs/heads/master@{#389119}

[modify] https://crrev.com/776e8a67c0e7fe1cf9399605f857aa93daf4c1ed/third_party/libxml/README.chromium
[modify] https://crrev.com/776e8a67c0e7fe1cf9399605f857aa93daf4c1ed/third_party/libxml/src/xmlstring.c

Status: Fixed (was: Started)
We now have a stable bug fix and a regression test so I am closing this.

Sign in to add a comment