42102 2010-07-12 12:58:36 -0700 Moving BOM stripping from JSC specific code to V8+JSC shared code caused V8 crashes 2010-07-13 18:19:21 -0700 1 1 1 Unclassified WebKit WebCore JavaScript 528+ (Nightly build) Other All RESOLVED FIXED http://www.1616.net/ P2 Normal --- 42224 1 tonyg tonyg abarth ager commit-queue loki oliver zherczeg oldest_to_newest 249979 0 tonyg 2010-07-12 12:58:36 -0700 It appears that https://bugs.webkit.org/show_bug.cgi?id=41539 moved BOM stripping from JSC specific code to CachedScript which is shared by V8 & JSC. This caused crashes in V8: http://code.google.com/p/chromium/issues/detail?id=48475 I'm looking into a fix now. Oliver, please let me know if you have any suggestions or insight. 250080 1 61275 tonyg 2010-07-12 14:59:17 -0700 Created attachment 61275 Patch 250096 2 61275 oliver 2010-07-12 15:26:27 -0700 Comment on attachment 61275 Patch How is v8 getting the text without boms removed? this patch basically makes it so that decoded script data has the BOMs removed. If V8 is getting scripts from webcore without decoding occuring then v8 is breaking itself. That said the guard should be USE(JSC) not "WTF_USE_JSC" 250124 3 tonyg 2010-07-12 16:17:45 -0700 (In reply to comment #2) > (From update of attachment 61275 [details]) > How is v8 getting the text without boms removed? Ah yes, that is the key question. It gets it via bindings/v8/ScriptSourceCode.h. That seems to be the equivalent of JSC's StringSourceProvider to which you added the copyStringWithoutBOMs() call to the ctor. So an alternative and cleaner fix is to add the same code to that ctor that you added to StringSourceProvider's. > this patch basically makes it so that decoded script data has the BOMs removed. If V8 is getting scripts from webcore without decoding occuring then v8 is breaking itself. Here's where I'm not understanding something. Hopefully you can fill me in. V8 skips over BOMs, treating them as whitespace. This method copies the string, stripping the BOMs. But it doesn't appear to actually be doing anything with the BOM. So why pay the expense of the string copy to strip instead of just skipping them as whitespace during parsing. > > That said the guard should be USE(JSC) not "WTF_USE_JSC" Noted. 250130 4 oliver 2010-07-12 16:23:42 -0700 (In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 61275 [details] [details]) > > How is v8 getting the text without boms removed? > > Ah yes, that is the key question. It gets it via bindings/v8/ScriptSourceCode.h. That seems to be the equivalent of JSC's StringSourceProvider to which you added the copyStringWithoutBOMs() call to the ctor. So an alternative and cleaner fix is to add the same code to that ctor that you added to StringSourceProvider's. > > > this patch basically makes it so that decoded script data has the BOMs removed. If V8 is getting scripts from webcore without decoding occuring then v8 is breaking itself. > > Here's where I'm not understanding something. Hopefully you can fill me in. V8 skips over BOMs, treating them as whitespace. This method copies the string, stripping the BOMs. But it doesn't appear to actually be doing anything with the BOM. So why pay the expense of the string copy to strip instead of just skipping them as whitespace during parsing. We have found historically that there is a substantial cost to allowing BOMs to be scattered throughout the source -- the lexer needs to do a lot more work for any symbol that is more than a single character long. 250140 5 61289 tonyg 2010-07-12 16:41:08 -0700 Created attachment 61289 Patch 250144 6 61289 oliver 2010-07-12 16:43:29 -0700 Comment on attachment 61289 Patch As long as the V8 lexer handles boms itself this is unlikely to be a win, you should test impact after removing bom handling from the v8 lexer. 250166 7 tonyg 2010-07-12 17:10:55 -0700 (In reply to comment #6) > (From update of attachment 61289 [details]) > As long as the V8 lexer handles boms itself this is unlikely to be a win, you should test impact after removing bom handling from the v8 lexer. @ager, I'm landing this as a correctness fix. WebKit now guarantees not to pass BOMs to V8, so do you know if it is possible to remove the BOM check from scanner.cc? If so, this might actually be a win for V8. If not, we probably want to look into a clean way to avoid this extra copy for V8 without a #if guard in CachedScript. 250257 8 61289 commit-queue 2010-07-12 19:57:59 -0700 Comment on attachment 61289 Patch Clearing flags on attachment: 61289 Committed r63162: <http://trac.webkit.org/changeset/63162> 250258 9 commit-queue 2010-07-12 19:58:04 -0700 All reviewed patches have been landed. Closing bug. 250395 10 ager 2010-07-13 02:13:39 -0700 The BOM handling in the V8 scanner doesn't cost. I have removed it and I see no performance impact. Also, I don't think the BOM handling should be removed from the scanner. I see a couple of issues: 1. Stripping BOM chars outside of the scanner means that all BOM chars are stripped no matter where they occur. This is not done in any of the other browsers. In particular BOM chars are not stripped from string literals. BOM chars are only treated as whitespace between tokens. This has been treated as a security issue in Firefox: http://www.mozilla.org/security/announce/2008/mfsa2008-43.html https://bugzilla.mozilla.org/show_bug.cgi?id=430740 V8 is following the other engines here. 2. There is no handling of BOM chars in strings generated in JavaScript code and passed to eval if the handling is removed from the scanner. Therefore the handling of BOM in script blocks and in strings generated in JS is different. 3. The stripping currently done in WebKit only handles 0xFEFF. The other engines handle 0xFFFE as well. For the above reasons i don't think BOM chars should be stripped early like this. It should be handled by the scanner and only between tokens. Oliver, what do you think? 250425 11 zherczeg 2010-07-13 03:40:28 -0700 (In reply to comment #10) Wow, this is interesting. I have tried moz regression test: https://bug430740.bugzilla.mozilla.org/attachment.cgi?id=318439 and it was passed. After a little thinking, I changed the \ufffe to \ufeff, and got the following output: 861480 STATUS: Do not strip format-control characters from string literals evildone FAILED!: [reported from test()] Do not strip format-control characters from string literals FAILED!: [reported from test()] Expected value 'a%EF%BF%BE,+doevil()%5D)//', Actual value 'a',%20evildone' FAILED!: [reported from test()] Evil things happen here. The Unicode spec says (in chapter 2.13): To have an efficient way to indicate which byte order is used in a text, the Unicode Standard contains two code points, U+FEFF zero width no-break space (byte order mark) and U+FFFE (a noncharacter), which are the byte-ordered mirror images of each other. When a BOM is received with the opposite byte order, it will be recognized as a noncharacter and can therefore be used to detect the intended byte order of the text. The BOM is not a control character that selects the byte order of the text; rather, its function is to allow recipients to determine which byte ordering is used in a file. "zero width no-break space" : I cannot find the definition of "no-break space", but it implies it is NOT a regular white space. AB\ufeffCD is simply an ABCD identifier, or 12\ufeff34 is number 1234. Looks like neither V8 nor SS do the right thing. 250683 12 oliver 2010-07-13 12:53:42 -0700 It looks like there's a change in behaviour from ES3 to ES5 -- ES3 requires BOMs to be stripped, ES5 says we should treat BOMs as whitespace. I'm just harassing brendan to find out what the correct behaviour is meant to be. 61275 2010-07-12 14:59:17 -0700 2010-07-12 16:41:05 -0700 Patch bug-42102-20100712145915.patch text/plain 2153 tonyg ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg YjQ2OGQwMjI0NTdhZTZhM2U2M2ZmNzE1OTI0ZGYxYzU0MDQxNmNkNS4uZTg2MTQ4MDQwZGI4Yjdi NzAzZGRjMzhkYzlmNWQ2M2U4ODU2ZTQ1MiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cK KysrIGIvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwyMCBAQAorMjAxMC0wNy0xMiAgVG9u eSBHZW50aWxjb3JlICA8dG9ueWdAY2hyb21pdW0ub3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIERvIG5vdCBjb3B5IHN0cmluZyB3aXRob3V0IEJP TXMgZm9yIFY4CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD00MjEwMgorCisgICAgICAgIFRoaXMgaXMgYSBjaGVlc3kgdGVtcG9yYXJ5IHNvbHV0aW9uIHVu dGlsIHdlIHN0cmFpZ2h0ZW4gdGhpcyBvdXQuIEkKKyAgICAgICAgcGxhbiB0byBjcmVhdGUgYSB0 ZXN0IGNhc2UgdG8gcmVwcm9kdWNlIHRoaXMgYW5kIGZpeCBpdCBpbiB0aGUgcmlnaHQKKyAgICAg ICAgd2F5LiBCdXQgSSdkIGxpa2UgdG8gZ2V0IHNvbWV0aGluZyBpbiBwbGFjZSBmb3Igbm93IHNp bmNlIHRoaXMgaXMgYQorICAgICAgICBzaWduaWZpY2FudCBidWcuCisKKyAgICAgICAgTm8gbmV3 IHRlc3RzIGJlY2F1c2UgdGhvc2Ugd2lsbCBiZSBhZGRlZCB3aXRoIHJlYWwgZml4LgorCisgICAg ICAgICogbG9hZGVyL0NhY2hlZFNjcmlwdC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpDYWNoZWRT Y3JpcHQ6OnNjcmlwdCk6CisKIDIwMTAtMDctMTIgIEFuZHJlYXMgS2xpbmcgIDxhbmRyZWFzLmts aW5nQG5va2lhLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBPbGl2ZXIgSHVudC4KZGlmZiAt LWdpdCBhL1dlYkNvcmUvbG9hZGVyL0NhY2hlZFNjcmlwdC5jcHAgYi9XZWJDb3JlL2xvYWRlci9D YWNoZWRTY3JpcHQuY3BwCmluZGV4IGUzZDYxOGFhN2NiMWY1YTgzOTNjMjYwNmM3OTE4M2NkOGU5 YjNlY2EuLjZjNTJhYWZkZWIyMzVlYTM3YTMwNDNjZWRkYmVlZWRjM2M1YTFkZDcgMTAwNjQ0Ci0t LSBhL1dlYkNvcmUvbG9hZGVyL0NhY2hlZFNjcmlwdC5jcHAKKysrIGIvV2ViQ29yZS9sb2FkZXIv Q2FjaGVkU2NyaXB0LmNwcApAQCAtNzMsMTEgKzczLDE4IEBAIGNvbnN0IFN0cmluZyYgQ2FjaGVk U2NyaXB0OjpzY3JpcHQoKQogICAgIGlmICghbV9zY3JpcHQgJiYgbV9kYXRhKSB7CiAgICAgICAg IG1fc2NyaXB0ID0gbV9kZWNvZGVyLT5kZWNvZGUobV9kYXRhLT5kYXRhKCksIGVuY29kZWRTaXpl KCkpOwogICAgICAgICBtX3NjcmlwdCArPSBtX2RlY29kZXItPmZsdXNoKCk7CisvLyBGSVhNRTog QXMgb2YgcjYyNDQ5LCBKU0MgZXhwZWN0cyBCT01zIHRvIGJlIHN0cmlwcGVkIGZyb20gdGhlIHNj cmlwdCBzb3VyY2UuCisvLyBIb3dldmVyLCBWOCBzdGlsbCB0cmVhdHMgQk9NcyBhcyB3aGl0ZXNw YWNlLCBzbyBjb3B5U3RyaW5nV2l0aG91dEJPTXMgaXMgbm90CisvLyBvbmx5IHVubmVjZXNzYXJ5 LCBidXQgY2hhbmdpbmcgbV9zY3JpcHQgYXQgdGhpcyBwb2ludCBicmVha3MgY2FjaGluZyBvZiBW OCdzCisvLyBwcmUtcGFyc2VkIHNjcmlwdCBkYXRhLiBKU0MvVjggc3BlY2lmaWMgZ3VhcmRzIGRv IG5vdCBiZWxvbmcgaW4gV2ViQ29yZSwKKy8vIHNvIHRoaXMgaXMgYSB0ZW1wb3JhcnkgZml4IHVu dGlsIHRoZSBhcmNoaXRlY3R1cmUgaXMgc3RyYWlnaHRlbmVkIG91dC4KKyNpZiBXVEZfVVNFX0pT QwogICAgICAgICBpZiAobV9zY3JpcHRIYXNCT01zICE9IFNvdXJjZUhhc05vQk9NcyAmJiBtX3Nj cmlwdC5sZW5ndGgoKSkgewogICAgICAgICAgICAgYm9vbCBoYXNCT01zID0gZmFsc2U7CiAgICAg ICAgICAgICBtX3NjcmlwdCA9IFN0cmluZyhtX3NjcmlwdC5pbXBsKCktPmNvcHlTdHJpbmdXaXRo b3V0Qk9NcyhtX3NjcmlwdEhhc0JPTXMgPT0gU291cmNlSGFzQk9NcywgaGFzQk9NcykpOwogICAg ICAgICAgICAgbV9zY3JpcHRIYXNCT01zID0gaGFzQk9NcyA/IFNvdXJjZUhhc0JPTXMgOiBTb3Vy Y2VIYXNOb0JPTXM7CiAgICAgICAgIH0KKyNlbmRpZgogICAgICAgICBzZXREZWNvZGVkU2l6ZSht X3NjcmlwdC5sZW5ndGgoKSAqIHNpemVvZihVQ2hhcikpOwogICAgIH0KIAo= 61289 2010-07-12 16:41:08 -0700 2010-07-12 19:57:59 -0700 Patch bug-42102-20100712164107.patch text/plain 1743 tonyg ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg YjQ2OGQwMjI0NTdhZTZhM2U2M2ZmNzE1OTI0ZGYxYzU0MDQxNmNkNS4uZmIxY2MzZjM0MjYwMDg3 ODE1ZDRhMWI2NzM5MjMwZTE2ZWJjMGY2NSAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cK KysrIGIvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxOCBAQAorMjAxMC0wNy0xMiAgVG9u eSBHZW50aWxjb3JlICA8dG9ueWdAY2hyb21pdW0ub3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFN0cmlwIEJPTXMgZnJvbSBzb3VyY2UgYmVmb3Jl IHBhc3NpbmcgdG8gVjgKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTQyMTAyCisKKyAgICAgICAgVGhpcyBleHRyYSBjb3B5IG1heSBjYXJyeSBhIHBlcmZv cm1hbmNlIHBlbmFsdHkuIFdlIHNob3VsZCBpbnZlc3RpZ2F0ZQorICAgICAgICB3aGV0aGVyIHRo aXMgYWxsb3dzIGFueSBzaW1wbGlmaWNhdGlvbiBpbiB2OC9zY2FubmVyLmNjOlNraXBKYXZhU2Ny aXB0V2hpdGVTcGFjZSgpLgorCisgICAgICAgIE5vIG5ldyB0ZXN0cyBiZWNhdXNlIG5vIG5ldyBm dW5jdGlvbmFsaXR5LgorCisgICAgICAgICogYmluZGluZ3MvdjgvU2NyaXB0U291cmNlQ29kZS5o OgorICAgICAgICAoV2ViQ29yZTo6U2NyaXB0U291cmNlQ29kZTo6U2NyaXB0U291cmNlQ29kZSk6 CisKIDIwMTAtMDctMTIgIEFuZHJlYXMgS2xpbmcgIDxhbmRyZWFzLmtsaW5nQG5va2lhLmNvbT4K IAogICAgICAgICBSZXZpZXdlZCBieSBPbGl2ZXIgSHVudC4KZGlmZiAtLWdpdCBhL1dlYkNvcmUv YmluZGluZ3MvdjgvU2NyaXB0U291cmNlQ29kZS5oIGIvV2ViQ29yZS9iaW5kaW5ncy92OC9TY3Jp cHRTb3VyY2VDb2RlLmgKaW5kZXggZGJjOWQ1ZTZmYzYwMDdmY2ZkZDY2MDI0ZGIwNzgyNmM2NjI2 M2I1Yi4uNDg1ZGFlYTVmMTk3NGMyNzRlNjY3YjFmNWM5MTk5NWQyMDE1ZWI3YSAxMDA2NDQKLS0t IGEvV2ViQ29yZS9iaW5kaW5ncy92OC9TY3JpcHRTb3VyY2VDb2RlLmgKKysrIGIvV2ViQ29yZS9i aW5kaW5ncy92OC9TY3JpcHRTb3VyY2VDb2RlLmgKQEAgLTQ2LDYgKzQ2LDE0IEBAIHB1YmxpYzoK ICAgICAgICAgLCBtX3VybCh1cmwpCiAgICAgICAgICwgbV9zdGFydExpbmUoc3RhcnRMaW5lKQog ICAgIHsKKyAgICAgICAgLy8gRklYTUU6IENvcHlpbmcgdGhlIHNvdXJjZSB0byBzdHJpcCBCT01z IGlzbid0IHN0cmljdGx5IG5lY2Vzc2FyeQorICAgICAgICAvLyBiZWNhdXNlIFY4IHRyZWF0cyBC T01zIGFzIHdoaXRlc3BhY2UuIEhvd2V2ZXIsIGl0IGlzIGhlcmUgYmVjYXVzZSBpdAorICAgICAg ICAvLyBtdXN0IGJlIGluIHN5bmMgd2l0aCBDYWNoZWRTY3JpcHQ6OnNjcmlwdCgpIHdoaWNoIHN0 cmlwcyB0aGUgQk9Ncy4gV2UKKyAgICAgICAgLy8gc2hvdWxkIGludmVzdGlnYXRlIHRoZSBwZXJm b3JtYW5jZSBpbXBsaWNhdGlvbnMuCisgICAgICAgIGlmIChtX3NvdXJjZS5sZW5ndGgoKSkgeyAK KyAgICAgICAgICAgIGJvb2wgc2NyYXRjaCA9IGZhbHNlOyAKKyAgICAgICAgICAgIG1fc291cmNl ID0gU3RyaW5nKHNvdXJjZS5pbXBsKCktPmNvcHlTdHJpbmdXaXRob3V0Qk9NcyhmYWxzZSwgc2Ny YXRjaCkpOyAKKyAgICAgICAgfSAKICAgICB9CiAKICAgICAvLyBXZSBsb3NlIHRoZSBlbmNvZGlu ZyBpbmZvcm1hdGlvbiBmcm9tIENhY2hlZFNjcmlwdC4K