40347 2010-06-08 19:51:44 -0700 Fix fast/parser/entity-surrogate-pairs.html in HTML5 parser 2012-11-27 12:52:53 -0800 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) Other OS X 10.5 RESOLVED WONTFIX P2 Normal --- 39259 1 tonyg tonyg abarth ap eric ian mitz webkit.review.bot oldest_to_newest 235622 0 tonyg 2010-06-08 19:51:44 -0700 Fix fast/parser/entity-surrogate-pairs.html in HTML5 parser 235623 1 58206 tonyg 2010-06-08 19:55:02 -0700 Created attachment 58206 Patch 235624 2 58206 abarth 2010-06-08 19:57:55 -0700 Comment on attachment 58206 Patch WebCore/html/HTML5Lexer.cpp:101 + if (value == 0 || value > 0x10FFFF) Did I just dream up the condition on the right? I thought I got it out of the spec.... 235625 3 webkit.review.bot 2010-06-08 20:00:22 -0700 Attachment 58206 did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/html/HTML5Lexer.cpp:101: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style. 235626 4 tonyg 2010-06-08 20:29:23 -0700 (In reply to comment #2) > (From update of attachment 58206 [details]) > WebCore/html/HTML5Lexer.cpp:101 > + if (value == 0 || value > 0x10FFFF) > Did I just dream up the condition on the right? I thought I got it out of the spec.... That condition is here for sure: http://www.whatwg.org/specs/web-apps/current-work/multipage/parsing.html#preprocessing-the-input-stream And it is enforced in InputStreamPreprocessor::peek(). However, I can't find the part of the spec that talks about entity processing and enforcing it here certainly prevents surrogate pair entities from working. 235691 5 eric 2010-06-09 00:45:14 -0700 Fixing the test is good. I think our suragate pair handling is still confused. The spec: All U+0000 NULL characters and code points in the range U+D800 to U+DFFF in the input must be replaced by U+FFFD REPLACEMENT CHARACTERs. Any occurrences of such characters and code points are parse errors. That seems wrong. I guess I need to read the HTML5 spec more closely. U+D800–U+DBFF is the range used by the leading surrogate. 235693 6 eric 2010-06-09 00:49:07 -0700 I think the spec is written in terms of characters, so I'm confused by the use of "code points" in "All U+0000 NULL characters and code points in the range U+D800 to U+DFFF". But I think the "code points" bit is just a mistake. I think that this pre-processing should already have been done at a layer below the InputStreamPreprocessor, so we can just hack out that code. 235836 7 tonyg 2010-06-09 09:55:11 -0700 (In reply to comment #6) > I think the spec is written in terms of characters, so I'm confused by the use of "code points" in "All U+0000 NULL characters and code points in the range U+D800 to U+DFFF". But I think the "code points" bit is just a mistake. > > I think that this pre-processing should already have been done at a layer below the InputStreamPreprocessor, so we can just hack out that code. Yes. Code below InputStreamPreprocessor is handling surrogate pairs. That's why they work when the filter in legalEntityFor() is removed. Adam, let me know if this is off, but my assessment was that your checks in legalEntityFor were based on the spec in #preprocessing-the-input-stream. The bug was that those checks should only be enforced in peek() rather than prior to adding the entities to the stream. 235837 8 58206 abarth 2010-06-09 09:59:05 -0700 Comment on attachment 58206 Patch Ok. I'm still not sure I understand the issue 100%, but I'm starting to be convinced. 235842 9 tonyg 2010-06-09 10:02:44 -0700 (In reply to comment #8) > (From update of attachment 58206 [details]) > Ok. I'm still not sure I understand the issue 100%, but I'm starting to be convinced. I've flipped the cq? flag. I'm assuming: 1. This is an appropriate style violation. 2. There isn't another part of the spec that I'm missing which legalEntityFor() was based upon (ie. it was based on #preprocessing-the-input-stream). 235850 10 abarth 2010-06-09 10:11:20 -0700 http://www.whatwg.org/specs/web-apps/current-work/multipage/tokenization.html#tokenizing-character-references [[ Otherwise, if the number is in the range 0xD800 to 0xDFFF or is greater than 0x10FFFF, then this is a parse error. Return a U+FFFD REPLACEMENT CHARACTER. ]] 235854 11 tonyg 2010-06-09 10:16:08 -0700 (In reply to comment #10) > http://www.whatwg.org/specs/web-apps/current-work/multipage/tokenization.html#tokenizing-character-references > > [[ > Otherwise, if the number is in the range 0xD800 to 0xDFFF or is greater than 0x10FFFF, then this is a parse error. Return a U+FFFD REPLACEMENT CHARACTER. > ]] OK. So that means surrogate pair entities are expressly forbidden in HTML5. Since they were supported before, that leaves a compatibility question. How do we handle these LayoutTests which test for legacy behavior that is expressly forbidden in HTML5? 235857 12 abarth 2010-06-09 10:20:39 -0700 We note them in the spreadsheet. Can you test for how other browser (especially IE, Firefox, and Firefox nightly) behave? 235864 13 tonyg 2010-06-09 10:42:54 -0700 (In reply to comment #12) > We note them in the spreadsheet. Can you test for how other browser (especially IE, Firefox, and Firefox nightly) behave? We are following the spec, so I'm marking this issue as WONTFIX, and I've noted it in the spreadsheet. In answer to your question, here's how various browsers perform on entity-surrogate-pairs.html: IE8: PASS Firefox 3.6: FAIL Minefield: FAIL WK legacy: PASS WK HTML5: FAIL It is interesting that comment in the test says "Firefox allows these". I suppose that means FF did at some point but that has now been removed. It is a better scary for compat that IE8 allows surrogate pair entities. 235873 14 abarth 2010-06-09 10:52:39 -0700 We should record all this information in a doc and send it to the HTML5 WG once we've got everything sorted out. 235877 15 tonyg 2010-06-09 10:59:20 -0700 (In reply to comment #14) > We should record all this information in a doc and send it to the HTML5 WG once we've got everything sorted out. Presumably, we are going to end up with a spreadsheet of only lines that have an "N" in needs code change. Those will be the list of issues we'll have to resolve with the spec. I've added this bug number there so we can reference it when we prepare the list of issues. 235966 16 ap 2010-06-09 13:25:29 -0700 See bug 6446 for why this was done (no known Web compatibility issues, just compatibility with other browsers). But thinking about this now, I like our forgiving behavior. There are processes on the Web that blindly encode all non-ASCII "characters" in input stream as numeric entities. I don't have a specific example. but depending on their definition of "character", they can end up with exactly this. And I'm not aware of any downside in supporting such split entities. 235992 17 abarth 2010-06-09 13:57:05 -0700 I've added your comment to our new document that's tracking these issues: https://docs.google.com/document/edit?id=1as5xYjyMSCph4960iz0-Kb7hZKf_L6f2vts57NMcVBI&hl=en Our current plan is to have the new parser 100% match the spec. If we want to do something differently, we probably want to change the spec and then move the implementation to match the updated spec. 777140 18 ap 2012-11-27 12:52:53 -0800 For future reference: <rdar://problem/12747226>and <rdar://problem/10447418> track two regressions from switching to the new behavior. 58206 2010-06-08 19:55:02 -0700 2010-06-09 10:51:33 -0700 Patch bug-40347-20100608195500.patch text/plain 2056 tonyg ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg ZGJhNDJkYWM4NjhmMDYxNjkwZGRlZWJkZmE3ZDc4Y2RiMThkNjFlNS4uNjJjYzRkNGVkNTkxOTc2 ZjM2ODQ0N2M5N2RjNGU5YzA5MWFmMmJiNCAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cK KysrIGIvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNyBAQAorMjAxMC0wNi0wOCAgVG9u eSBHZW50aWxjb3JlICA8dG9ueWdAY2hyb21pdW0ub3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEZpeCBmYXN0L3BhcnNlci9lbnRpdHktc3Vycm9n YXRlLXBhaXJzLmh0bWwgaW4gSFRNTDUgcGFyc2VyCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD00MDM0NworCisgICAgICAgIE5vIG5ldyB0ZXN0cyBiZWNh dXNlIGNvdmVyZWQgYnkgZmFzdC9wYXJzZXIvZW50aXR5LXN1cnJvZ2F0ZS1wYWlycy5odG1sCisK KyAgICAgICAgKiBodG1sL0hUTUw1TGV4ZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6SFRNTE5h bWVzOjpsZWdhbEVudGl0eUZvcik6CisgICAgICAgICogaHRtbC9IVE1MNUxleGVyLmg6CisgICAg ICAgIChXZWJDb3JlOjpIVE1MNUxleGVyOjpJbnB1dFN0cmVhbVByZXByb2Nlc3Nvcjo6cGVlayk6 CisKIDIwMTAtMDYtMDggIEtlbm5ldGggUnVzc2VsbCAgPGtickBnb29nbGUuY29tPgogCiAgICAg ICAgIFVucmV2aWV3ZWQsIGJ1aWxkIGZpeC4KZGlmZiAtLWdpdCBhL1dlYkNvcmUvaHRtbC9IVE1M NUxleGVyLmNwcCBiL1dlYkNvcmUvaHRtbC9IVE1MNUxleGVyLmNwcAppbmRleCBmYWJlYWU0OWRh YjEzMDhiMGQyMzZiM2VkM2NjYzc2NTE2ZTg3NzUyLi4xNmFjMzAzMTU4MDE1ODg5NjlkMzdjMmNh ZWVlZDBhMTYwMjk2NTgyIDEwMDY0NAotLS0gYS9XZWJDb3JlL2h0bWwvSFRNTDVMZXhlci5jcHAK KysrIGIvV2ViQ29yZS9odG1sL0hUTUw1TGV4ZXIuY3BwCkBAIC05OCw4ICs5OCw3IEBAIGlubGlu ZSBVQ2hhciBhZGp1c3RFbnRpdHkodW5zaWduZWQgdmFsdWUpCiAKIGlubGluZSB1bnNpZ25lZCBs ZWdhbEVudGl0eUZvcih1bnNpZ25lZCB2YWx1ZSkKIHsKLSAgICAvLyBGSVhNRTogQSBudW1iZXIg b2Ygc3BlY2lmaWMgZW50aXR5IHZhbHVlcyBnZW5lcmF0ZSBwYXJzZSBlcnJvcnMuCi0gICAgaWYg KHZhbHVlID09IDAgfHwgdmFsdWUgPiAweDEwRkZGRiB8fCAodmFsdWUgPj0gMHhEODAwICYmIHZh bHVlIDw9IDB4REZGRikpCisgICAgaWYgKHZhbHVlID09IDAgfHwgdmFsdWUgPiAweDEwRkZGRikK ICAgICAgICAgcmV0dXJuIDB4RkZGRDsKICAgICBpZiAodmFsdWUgPCAweEZGRkYpCiAgICAgICAg IHJldHVybiBhZGp1c3RFbnRpdHkodmFsdWUpOwpkaWZmIC0tZ2l0IGEvV2ViQ29yZS9odG1sL0hU TUw1TGV4ZXIuaCBiL1dlYkNvcmUvaHRtbC9IVE1MNUxleGVyLmgKaW5kZXggZDlkNzU5YmYwMmNk NGE1NmNiM2IyYTAwZmUyMTE3YTFkYTQxMGVjNS4uOWJjYTgzOWJmZGY1NTk1NGRkNDI0N2UwMmJj NzhkMDgwNjZmMjEzZCAxMDA2NDQKLS0tIGEvV2ViQ29yZS9odG1sL0hUTUw1TGV4ZXIuaAorKysg Yi9XZWJDb3JlL2h0bWwvSFRNTDVMZXhlci5oCkBAIC0xNjEsNiArMTYxLDcgQEAgbmFtZXNwYWNl IFdlYkNvcmUgewogICAgICAgICAgICAgICAgICAgICBtX3NraXBOZXh0TmV3TGluZSA9IGZhbHNl OwogICAgICAgICAgICAgICAgICAgICBpZiAobV9uZXh0SW5wdXRDaGFyYWN0ZXIgPT0gJ1wwJyB8 fCAobV9uZXh0SW5wdXRDaGFyYWN0ZXIgPj0gMHhEODAwICYmIG1fbmV4dElucHV0Q2hhcmFjdGVy IDw9IDB4REZGRikpCiAgICAgICAgICAgICAgICAgICAgICAgICBtX25leHRJbnB1dENoYXJhY3Rl ciA9IDB4RkZGRDsKKyAgICAgICAgICAgICAgICAgICAgLy8gRklYTUU6IEEgbnVtYmVyIG9mIHNw ZWNpZmljIGNoYXJhY3RlciB2YWx1ZXMgc2hvdWxkIGdlbmVyYXRlIHBhcnNlIGVycm9ycy4KICAg ICAgICAgICAgICAgICB9CiAgICAgICAgICAgICAgICAgcmV0dXJuIHRydWU7CiAgICAgICAgICAg ICB9Cg==