34492 2010-02-02 10:45:52 -0800 avoid using invalidated KURL object in Element::baseURI 2010-02-03 08:11:15 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) All All RESOLVED FIXED P3 Minor --- 0 bryeung webkit-unassigned brettw commit-queue darin eric fishd levin oldest_to_newest 187046 0 bryeung 2010-02-02 10:45:52 -0800 Currently, an invalidated KURL object is used to carry around a relative URL string in Element::baseURI. KURLGoogle does not support this behaviour, and I think it is better to not make use of invalidated KURL objects. Here is a simple patch that makes no semantic change and circumvents the use of the invalidated object. 187051 1 darin 2010-02-02 10:53:05 -0800 (In reply to comment #0) > Here is a simple patch that makes no semantic change and circumvents the use of > the invalidated object. Where? 187053 2 darin 2010-02-02 10:57:53 -0800 Please be clear, holding arbitrary strings that are not valid URLs is an intentional feature of KURL. To remove it we need to remove all clients that depend on that. It sure would be great to have the freedom to not need that feature, but we can’t get that by just fixing the cases we have already stumbled on, we need to look at all callers. And then change the implementation so the mistake is easy to detect. I know that Chromium has been living with all these bugs all along since KURLGoogle omitted that feature, but when fixing this please keep in mind that all other platforms have not been subject to the bugs. KURLGoogle still seems like a bad idea to me. I know it’s pleasant for Chromium to share the same URL parsing with the other components in Chromium -- it would be nice to have that on the Mac too -- and someone may have the idea that it helps with security, but I believe that for the WebKit project as a whole would be great to make changes as needed to both the original KURL and KURLGoogle so their behavior is identical and then get rid of this unnecessary difference between platforms by deleting KURLGoogle. 187055 3 47952 bryeung 2010-02-02 11:00:12 -0800 Created attachment 47952 patch Does it not work to add a patch from the bug creation page? Anyway, here is the patch. 187056 4 bryeung 2010-02-02 11:00:46 -0800 Sorry! Wrong bug to attach the patch to. Please ignore. 187058 5 bryeung 2010-02-02 11:01:52 -0800 (In reply to comment #4) > Sorry! Wrong bug to attach the patch to. Please ignore. Nope: I just didn't realize there were more comments, so this looked like a different bug. Clearly I'm not really used to bugzilla. Sorry for the confusion and extraneous comments. 187059 6 darin 2010-02-02 11:02:38 -0800 (In reply to comment #3) > Does it not work to add a patch from the bug creation page? I never tried it before. Maybe it’s broken. 187060 7 47952 darin 2010-02-02 11:05:24 -0800 Comment on attachment 47952 patch Change looks great, although I’d probably use assignment syntax rather than initialization to set up the baseAttribute local variable. Generally it's not correct to set the review flag on a patch without a change log entry. We require regression tests for bugs fixes. Since this patch fixes a bug, you must also include a regression test that fails on Chromium before this change and now succeeds with the fix. But neither the test itself nor the test results should be Chromium-specific. review- because of the lack of change log and regression test 187068 8 47955 bryeung 2010-02-02 11:22:33 -0800 Created attachment 47955 revised patch Sorry for the incomplete patch last time. This includes the ChangeLog entry. There was already a test failing on Chromium without this change (LayoutTests/svg/W3C-SVG-1.1/struct-image-07-t.svg), so I do not think an additional regression test is necessary. 187071 9 darin 2010-02-02 11:25:40 -0800 (In reply to comment #8) > Sorry for the incomplete patch last time. This includes the ChangeLog entry. > There was already a test failing on Chromium without this change > (LayoutTests/svg/W3C-SVG-1.1/struct-image-07-t.svg), so I do not think an > additional regression test is necessary. Typically a patch would include the corrected results or a change to the skipped list for a patch -- so I could see what test was changing behavior. But since Chromium has a different way of doing things, maybe that's why there's no such change here. Despite the fact that a test was failing, a simpler test that targets this behavior more directly would be far better. It could even be a dumpAsText test. Tests where success can only be judged from the pixel results are the lowest on our totem pole of test usefulness, so this existing test is only barely sufficient. 187072 10 47955 darin 2010-02-02 11:26:32 -0800 Comment on attachment 47955 revised patch > +2010-02-02 Bryan Yeung <bryeung@bryeung-chrome.(none)> Strange address here. r=me despite my concerns mentioned in comments above. 187085 11 47955 commit-queue 2010-02-02 12:04:36 -0800 Comment on attachment 47955 revised patch Clearing flags on attachment: 47955 Committed r54245: <http://trac.webkit.org/changeset/54245> 187086 12 commit-queue 2010-02-02 12:04:44 -0800 All reviewed patches have been landed. Closing bug. 187205 13 levin 2010-02-02 16:54:23 -0800 Committed r54264: <http://trac.webkit.org/changeset/54264> 187208 14 levin 2010-02-02 17:06:05 -0800 Rolled out due to major chromium bustage with this patch. Rolllout: http://trac.webkit.org/changeset/54264 187220 15 levin 2010-02-02 17:54:23 -0800 Actually, it looks like I may have been wrong. Once the canary bot gets well. I'd be happy to re-commit this (since it was rolled out incorrectly) -- sometimes it is hard to tell which patch caused the issue :( 187367 16 levin 2010-02-03 08:11:15 -0800 Committed again unchanged from before http://trac.webkit.org/changeset/54282 Sorry about the mix-up here. 47952 2010-02-02 11:00:12 -0800 2010-02-02 11:22:33 -0800 patch p1.txt text/plain 733 bryeung ZGlmZiAtLWdpdCBhL1dlYkNvcmUvZG9tL0VsZW1lbnQuY3BwIGIvV2ViQ29yZS9kb20vRWxlbWVu dC5jcHAKaW5kZXggYWM0ZmRiMy4uNTU2MWZlZiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9kb20vRWxl bWVudC5jcHAKKysrIGIvV2ViQ29yZS9kb20vRWxlbWVudC5jcHAKQEAgLTcwMiw3ICs3MDIsOCBA QCB2b2lkIEVsZW1lbnQ6OnNldFByZWZpeChjb25zdCBBdG9taWNTdHJpbmcmIHByZWZpeCwgRXhj ZXB0aW9uQ29kZSYgZWMpCiAKIEtVUkwgRWxlbWVudDo6YmFzZVVSSSgpIGNvbnN0CiB7Ci0gICAg S1VSTCBiYXNlKEtVUkwoKSwgZ2V0QXR0cmlidXRlKGJhc2VBdHRyKSk7CisgICAgY29uc3QgQXRv bWljU3RyaW5nJiBiYXNlQXR0cmlidXRlKGdldEF0dHJpYnV0ZShiYXNlQXR0cikpOworICAgIEtV UkwgYmFzZShLVVJMKCksIGJhc2VBdHRyaWJ1dGUpOwogICAgIGlmICghYmFzZS5wcm90b2NvbCgp LmlzRW1wdHkoKSkKICAgICAgICAgcmV0dXJuIGJhc2U7CiAKQEAgLTcxNCw3ICs3MTUsNyBAQCBL VVJMIEVsZW1lbnQ6OmJhc2VVUkkoKSBjb25zdAogICAgIGlmIChwYXJlbnRCYXNlLmlzTnVsbCgp KQogICAgICAgICByZXR1cm4gYmFzZTsKIAotICAgIHJldHVybiBLVVJMKHBhcmVudEJhc2UsIGJh c2Uuc3RyaW5nKCkpOworICAgIHJldHVybiBLVVJMKHBhcmVudEJhc2UsIGJhc2VBdHRyaWJ1dGUp OwogfQogCiB2b2lkIEVsZW1lbnQ6OmNyZWF0ZUF0dHJpYnV0ZU1hcCgpIGNvbnN0Cg== 47955 2010-02-02 11:22:33 -0800 2010-02-02 12:04:35 -0800 revised patch p1.txt text/plain 1507 bryeung ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg YWJkYmI1OC4uNjczMDc4MiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxOSBAQAorMjAxMC0wMi0wMiAgQnJ5YW4gWWV1bmcg IDxicnlldW5nQGJyeWV1bmctY2hyb21lLihub25lKT4KKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICBBdm9pZCB1c2luZyBhbiBpbnZhbGlkYXRlZCBLVVJM IG9iamVjdCBpbiBiYXNlVVJJLgorCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3No b3dfYnVnLmNnaT9pZD0zNDQ5MgorCisgICAgICAgIFRoaXMgY2hhbmdlIGZpeGVzIGJhc2VVUkkg Zm9yIENocm9taXVtICh3aGVyZSB0aGUgS1VSTCBpbXBsZW1lbnRhdGlvbgorICAgICAgICBkb2Vz IG5vdCBhbGxvdyBpbnZhbGlkIEtVUkxzIHRvIGNhcnJ5IHJlbGF0aXZlIHBhdGhzKS4gIFRoaXMg aXMKKyAgICAgICAgcmVncmVzc2lvbiB0ZXN0ZWQgYnkKKyAgICAgICAgTGF5b3V0VGVzdHMvc3Zn L1czQy1TVkctMS4xL3N0cnVjdC1pbWFnZS0wNy10LnN2ZworCisgICAgICAgICogZG9tL0VsZW1l bnQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6RWxlbWVudDo6YmFzZVVSSSk6CisKIDIwMTAtMDIt MDIgIEJlbmphbWluIFBvdWxhaW4gIDxiZW5qYW1pbi5wb3VsYWluQG5va2lhLmNvbT4KIAogICAg ICAgICBSZXZpZXdlZCBieSBBcml5YSBIaWRheWF0LgpkaWZmIC0tZ2l0IGEvV2ViQ29yZS9kb20v RWxlbWVudC5jcHAgYi9XZWJDb3JlL2RvbS9FbGVtZW50LmNwcAppbmRleCBhYzRmZGIzLi43Mjll YzhkIDEwMDY0NAotLS0gYS9XZWJDb3JlL2RvbS9FbGVtZW50LmNwcAorKysgYi9XZWJDb3JlL2Rv bS9FbGVtZW50LmNwcApAQCAtNzAyLDcgKzcwMiw4IEBAIHZvaWQgRWxlbWVudDo6c2V0UHJlZml4 KGNvbnN0IEF0b21pY1N0cmluZyYgcHJlZml4LCBFeGNlcHRpb25Db2RlJiBlYykKIAogS1VSTCBF bGVtZW50OjpiYXNlVVJJKCkgY29uc3QKIHsKLSAgICBLVVJMIGJhc2UoS1VSTCgpLCBnZXRBdHRy aWJ1dGUoYmFzZUF0dHIpKTsKKyAgICBjb25zdCBBdG9taWNTdHJpbmcmIGJhc2VBdHRyaWJ1dGUg PSBnZXRBdHRyaWJ1dGUoYmFzZUF0dHIpOworICAgIEtVUkwgYmFzZShLVVJMKCksIGJhc2VBdHRy aWJ1dGUpOwogICAgIGlmICghYmFzZS5wcm90b2NvbCgpLmlzRW1wdHkoKSkKICAgICAgICAgcmV0 dXJuIGJhc2U7CiAKQEAgLTcxNCw3ICs3MTUsNyBAQCBLVVJMIEVsZW1lbnQ6OmJhc2VVUkkoKSBj b25zdAogICAgIGlmIChwYXJlbnRCYXNlLmlzTnVsbCgpKQogICAgICAgICByZXR1cm4gYmFzZTsK IAotICAgIHJldHVybiBLVVJMKHBhcmVudEJhc2UsIGJhc2Uuc3RyaW5nKCkpOworICAgIHJldHVy biBLVVJMKHBhcmVudEJhc2UsIGJhc2VBdHRyaWJ1dGUpOwogfQogCiB2b2lkIEVsZW1lbnQ6OmNy ZWF0ZUF0dHJpYnV0ZU1hcCgpIGNvbnN0Cg==