Issue metadata
Sign in to add a comment
|
xsl translate method do not work properly
Reported by
serj.ale...@gmail.com,
Mar 31 2016
|
||||||||||||||||||||||
Issue descriptionUserAgent: 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/
,
Apr 1 2016
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.
,
Apr 1 2016
my CL only affects Mac.
,
Apr 6 2016
,
Apr 11 2016
Could we get a narrower bisect? 324 CLs are a bit hard to find the suspect.
,
Apr 12 2016
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.
,
Apr 12 2016
Unable to reproduce the issue on Mac 10.10.5, Ubuntu 14.04.
,
Apr 12 2016
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?
,
Apr 12 2016
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?
,
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.
,
Apr 12 2016
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?
,
Apr 12 2016
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.
,
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.
,
Apr 12 2016
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/
,
Apr 12 2016
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.
,
Apr 12 2016
VC++ bug filed at https://connect.microsoft.com/VisualStudio/feedback/details/2582138
,
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
,
Apr 13 2016
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
,
Apr 13 2016
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.
,
Apr 14 2016
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.
,
Apr 14 2016
Your change meets the bar and is auto-approved for M51 (branch: 2704)
,
Apr 14 2016
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
,
Apr 15 2016
,
Apr 19 2016
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.
,
Apr 20 2016
,
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
,
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
,
Apr 22 2016
We now have a stable bug fix and a regression test so I am closing this. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by cbiesin...@chromium.org
, Mar 31 2016