54304 2011-02-11 12:24:04 -0800 Check for empty scheme before accessing URLSchemeMaps in SchemeRegistry 2011-02-11 14:03:29 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Other OS X 10.5 RESOLVED FIXED P2 Normal --- 0 adamk webkit-unassigned abarth commit-queue oldest_to_newest 349912 0 adamk 2011-02-11 12:24:04 -0800 Check for empty scheme before accessing URLSchemeMaps in SchemeRegistry 349913 1 82159 adamk 2011-02-11 12:25:23 -0800 Created attachment 82159 Patch 349915 2 82159 darin 2011-02-11 12:27:59 -0800 Comment on attachment 82159 Patch This change is correct, but can you please construct a test case that shows the problem? Normally we require regression tests for all bug fixes. 349939 3 82159 commit-queue 2011-02-11 12:52:38 -0800 Comment on attachment 82159 Patch Rejecting attachment 82159 from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'land-a..." exit_code: 2 Last 500 characters of output: ebKit 2d3c60d..e45ab5e master -> origin/master M LayoutTests/platform/chromium/test_expectations.txt M LayoutTests/ChangeLog M Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp M Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp M Tools/ChangeLog M Tools/Scripts/webkitpy/layout_tests/port/chromium_win.py r78359 = e45ab5efb940172a265712fe69fb923eef21d2df (refs/remotes/trunk) First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/trunk. Full output: http://queues.webkit.org/results/7869732 349955 4 adamk 2011-02-11 13:08:55 -0800 Thanks for the quick review. It's not currently possible to exercise this code with a layout test. For shouldTreatURLSchemeAsSecure, the only caller is (in FrameLoader.cpp): if (!url.isValid || SchemeRegistry::shouldTreatURLSchemeAsSecure(url.protocol()) so the case is never hit. The situation is similar for shouldLoadURLSchemeAsSecure in MainResourceLoader.cpp: return url.isEmpty() || SchemeRegistry::shouldLoadURLSchemeAsEmptyDocument(url.protocol()); Finally, shouldTreatURLSchemeAsNoAccess has no callers at all. Nevertheless, I think this patch is a good idea, to avoid new, less careful callers causing crashes. I just ran into this problem when trying to get https://bugs.webkit.org/show_bug.cgi?id=53529 landed (SecurityOrigin.cpp calls these methods without pre-validating the scheme). And it appears to me that Adam Barth recently resolved the same issue for another method in http://trac.webkit.org/changeset/76637. 349984 5 82159 eric 2011-02-11 13:42:04 -0800 Comment on attachment 82159 Patch I think the cq might be sick. It did that to me yesterday as well. 350003 6 82159 commit-queue 2011-02-11 14:03:24 -0800 Comment on attachment 82159 Patch Clearing flags on attachment: 82159 Committed r78367: <http://trac.webkit.org/changeset/78367> 350004 7 commit-queue 2011-02-11 14:03:29 -0800 All reviewed patches have been landed. Closing bug. 82159 2011-02-11 12:25:23 -0800 2011-02-11 14:03:24 -0800 Patch bug-54304-20110211122522.patch text/plain 2085 adamk U3VidmVyc2lvbiBSZXZpc2lvbjogNzgzNDkKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBmOWYzOWY5OTA3MzQyOWNh Zjk0ZmExMTFkZGVkMjNhYTIzZjNkOWMwLi45YTg3MjljNjQyODRhN2ZiM2NjYTRmZDBlZDUyMTBm NDRiN2MzMTIwIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTggQEAKKzIwMTEtMDItMTEgIEFkYW0g S2xlaW4gIDxhZGFta0BjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZ IChPT1BTISkuCisKKyAgICAgICAgQ2hlY2sgZm9yIGVtcHR5IHNjaGVtZSBiZWZvcmUgYWNjZXNz aW5nIFVSTFNjaGVtZU1hcHMgaW4gU2NoZW1lUmVnaXN0cnkKKyAgICAgICAgaHR0cHM6Ly9idWdz LndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTU0MzA0CisKKyAgICAgICAgVGhpcyBhdm9pZHMg cG90ZW50aWFsIGNyYXNoZXMgaW4gSGFzaE1hcCwgYXMgV1RGJ3MgU3RyaW5nSGFzaCBkb2Vzbid0 CisgICAgICAgIGFjY2VwdCBlbXB0eSBTdHJpbmdzLgorCisgICAgICAgICogcGxhdGZvcm0vU2No ZW1lUmVnaXN0cnkuY3BwOgorICAgICAgICAoV2ViQ29yZTo6U2NoZW1lUmVnaXN0cnk6OnNob3Vs ZFRyZWF0VVJMU2NoZW1lQXNOb0FjY2Vzcyk6CisgICAgICAgIChXZWJDb3JlOjpTY2hlbWVSZWdp c3RyeTo6c2hvdWxkVHJlYXRVUkxTY2hlbWVBc1NlY3VyZSk6CisgICAgICAgIChXZWJDb3JlOjpT Y2hlbWVSZWdpc3RyeTo6c2hvdWxkTG9hZFVSTFNjaGVtZUFzRW1wdHlEb2N1bWVudCk6CisKIDIw MTEtMDItMTEgIFNoZXJpZmYgQm90ICA8d2Via2l0LnJldmlldy5ib3RAZ21haWwuY29tPgogCiAg ICAgICAgIFVucmV2aWV3ZWQsIHJvbGxpbmcgb3V0IHI3ODMzMS4KZGlmZiAtLWdpdCBhL1NvdXJj ZS9XZWJDb3JlL3BsYXRmb3JtL1NjaGVtZVJlZ2lzdHJ5LmNwcCBiL1NvdXJjZS9XZWJDb3JlL3Bs YXRmb3JtL1NjaGVtZVJlZ2lzdHJ5LmNwcAppbmRleCAwYTdjYjhjZjRiOWZkY2EwZjVlZmQxZGYz M2YyZmZhZjc1ZGJiMTJlLi5iOWY0MmU2M2Y5MWQ2YjEzZjg1Yjk2M2ZiZDQ2M2NlODM1YmRiYmQw IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9TY2hlbWVSZWdpc3RyeS5jcHAK KysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vU2NoZW1lUmVnaXN0cnkuY3BwCkBAIC0xMjEs NiArMTIxLDggQEAgdm9pZCBTY2hlbWVSZWdpc3RyeTo6cmVnaXN0ZXJVUkxTY2hlbWVBc05vQWNj ZXNzKGNvbnN0IFN0cmluZyYgc2NoZW1lKQogCiBib29sIFNjaGVtZVJlZ2lzdHJ5OjpzaG91bGRU cmVhdFVSTFNjaGVtZUFzTm9BY2Nlc3MoY29uc3QgU3RyaW5nJiBzY2hlbWUpCiB7CisgICAgaWYg KHNjaGVtZS5pc0VtcHR5KCkpCisgICAgICAgIHJldHVybiBmYWxzZTsKICAgICByZXR1cm4gc2No ZW1lc1dpdGhVbmlxdWVPcmlnaW5zKCkuY29udGFpbnMoc2NoZW1lKTsKIH0KIApAQCAtMTQzLDYg KzE0NSw4IEBAIHZvaWQgU2NoZW1lUmVnaXN0cnk6OnJlZ2lzdGVyVVJMU2NoZW1lQXNTZWN1cmUo Y29uc3QgU3RyaW5nJiBzY2hlbWUpCiAKIGJvb2wgU2NoZW1lUmVnaXN0cnk6OnNob3VsZFRyZWF0 VVJMU2NoZW1lQXNTZWN1cmUoY29uc3QgU3RyaW5nJiBzY2hlbWUpCiB7CisgICAgaWYgKHNjaGVt ZS5pc0VtcHR5KCkpCisgICAgICAgIHJldHVybiBmYWxzZTsKICAgICByZXR1cm4gc2VjdXJlU2No ZW1lcygpLmNvbnRhaW5zKHNjaGVtZSk7CiB9CiAKQEAgLTE1Myw2ICsxNTcsOCBAQCB2b2lkIFNj aGVtZVJlZ2lzdHJ5OjpyZWdpc3RlclVSTFNjaGVtZUFzRW1wdHlEb2N1bWVudChjb25zdCBTdHJp bmcmIHNjaGVtZSkKIAogYm9vbCBTY2hlbWVSZWdpc3RyeTo6c2hvdWxkTG9hZFVSTFNjaGVtZUFz RW1wdHlEb2N1bWVudChjb25zdCBTdHJpbmcmIHNjaGVtZSkKIHsKKyAgICBpZiAoc2NoZW1lLmlz RW1wdHkoKSkKKyAgICAgICAgcmV0dXJuIGZhbHNlOwogICAgIHJldHVybiBlbXB0eURvY3VtZW50 U2NoZW1lcygpLmNvbnRhaW5zKHNjaGVtZSk7CiB9CiAK