38128 2010-04-26 10:06:37 -0700 Don't add empty credential to CredentialStorage. 2010-04-26 14:44:07 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Mac (Intel) OS X 10.5 RESOLVED FIXED P2 Normal --- 1 yongjun_zhang webkit-unassigned ap commit-queue mitz oldest_to_newest 217170 0 yongjun_zhang 2010-04-26 10:06:37 -0700 If the user respond an authentication challenge with an empty credential, it is added to the CredentialStorage. However, an empty credential is currently regarded as a invalid login because it means a missing value in protectionSpaceToCredentialMap. 217178 1 54315 yongjun_zhang 2010-04-26 10:18:56 -0700 Created attachment 54315 First attempt to fix this issue. Add empty credential check before adding it to CredentialStorage. 217184 2 54315 levin 2010-04-26 10:32:56 -0700 Comment on attachment 54315 First attempt to fix this issue. Needs a layout test or an explanation in the ChangeLog of why a layout test isn't possible. 217190 3 54318 yongjun_zhang 2010-04-26 10:54:04 -0700 Created attachment 54318 change the ChangeLog as per Davin's comment. 217218 4 54318 ap 2010-04-26 12:16:36 -0700 Comment on attachment 54318 change the ChangeLog as per Davin's comment. This patch only modifies one code path, avoiding badness in CredentialStorage::set() on some platforms. I think that it would be better for consistency to add an early return regardless of OS X version, something like if (credential.isEmpty()) { clearAuthentication(); return; } + No new tests added because it doesn't change the current behavior. I think that that a more accurate explanation would be "because this only affects credentials entered by the user, and we cannot test authentication dialog in DumpRenderTree". We need to make the same changes on Windows, but that can be a separate patch that someone (possibly myself) can make later. 217253 5 yongjun_zhang 2010-04-26 13:43:40 -0700 (In reply to comment #4) > (From update of attachment 54318 [details]) > This patch only modifies one code path, avoiding badness in > CredentialStorage::set() on some platforms. I think that it would be better for > consistency to add an early return regardless of OS X version, something like > > if (credential.isEmpty()) { > clearAuthentication(); > return; > } Hmm.. Wouldn't that make WebCore loading stay at a undefined state? For example, the progress bar is started because user clicks a link which leads to a page requires authentication. Then the challenge comes and user enters an empty credential, then we just bail out early without sending the _invalid_ credential out. I believe the progress bar will stay running because the FrameLoader is not notified in any way in this case. so I might need to change to something like: if (credential.isEmpty()) { m_client->cancel(ResourceError); // tell the FrameLoader to clear the Provisional Load and stop the progress bar. clearAuthentiation(); return; } > + No new tests added because it doesn't change the current behavior. > > I think that that a more accurate explanation would be "because this only > affects credentials entered by the user, and we cannot test authentication > dialog in DumpRenderTree". Good point! I will update the Changelog. > We need to make the same changes on Windows, but that can be a separate patch > that someone (possibly myself) can make later. 217255 6 ap 2010-04-26 13:53:12 -0700 > Wouldn't that make WebCore loading stay at a undefined state? Oops, you are right. I think that it would be most consistent to call receivedRequestToContinueWithoutCredential(). 217267 7 54329 yongjun_zhang 2010-04-26 14:08:17 -0700 Created attachment 54329 Use receivedRequestToContinueWithoutCredential for empty credentials as per Alexey comments. 217278 8 54329 ap 2010-04-26 14:24:29 -0700 Comment on attachment 54329 Use receivedRequestToContinueWithoutCredential for empty credentials as per Alexey comments. r=me, thanks! 217294 9 54329 commit-queue 2010-04-26 14:44:01 -0700 Comment on attachment 54329 Use receivedRequestToContinueWithoutCredential for empty credentials as per Alexey comments. Clearing flags on attachment: 54329 Committed r58264: <http://trac.webkit.org/changeset/58264> 217295 10 commit-queue 2010-04-26 14:44:07 -0700 All reviewed patches have been landed. Closing bug. 54315 2010-04-26 10:18:56 -0700 2010-04-26 10:54:04 -0700 First attempt to fix this issue. empty_credential.patch text/plain 1709 yongjun_zhang SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA1ODI2MCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTYgQEAKKzIwMTAtMDQtMjYgIFlvbmdqdW4gWmhhbmcgIDx5b25nanVuX3poYW5n QGFwcGxlLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICBEb24ndCBhZGQgZW1wdHkgY3JlZGVudGlhbCB0byBDcmVkZW50aWFsU3RvcmFnZS4KKyAg ICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTM4MTI4CisKKyAg ICAgICAgQW4gZW1wdHkgY3JlZGVudGlhbCBpcyBjdXJyZW50bHkgcmVnYXJkZWQgYXMgYSBpbnZh bGlkIGxvZ2luIGJlY2F1c2UgaXQgbWVhbnMgYSBtaXNzaW5nIHZhbHVlIGluIAorICAgICAgICBw cm90ZWN0aW9uU3BhY2VUb0NyZWRlbnRpYWxNYXAuICBUaGlzIGNoYW5nZSBtYWtlcyBpdCBjb25z aXN0ZW50IHdpdGggY3VycmVudCBXZWJDb3JlJ3MgYmVoYXZpb3IuCisKKyAgICAgICAgKiBwbGF0 Zm9ybS9uZXR3b3JrL21hYy9SZXNvdXJjZUhhbmRsZU1hYy5tbToKKyAgICAgICAgKFdlYkNvcmU6 OlJlc291cmNlSGFuZGxlOjpyZWNlaXZlZENyZWRlbnRpYWwpOgorCiAyMDEwLTA0LTI2ICBSb21h biBHZXJzaG1hbiAgPHJvbWFuZ2VAZ29vZ2xlLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBB ZGFtIEJhcnRoLgpJbmRleDogV2ViQ29yZS9wbGF0Zm9ybS9uZXR3b3JrL21hYy9SZXNvdXJjZUhh bmRsZU1hYy5tbQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsvbWFjL1Jl c291cmNlSGFuZGxlTWFjLm1tCShyZXZpc2lvbiA1ODI1NykKKysrIFdlYkNvcmUvcGxhdGZvcm0v bmV0d29yay9tYWMvUmVzb3VyY2VIYW5kbGVNYWMubW0JKHdvcmtpbmcgY29weSkKQEAgLTU4OCw3 ICs1ODgsMTAgQEAgdm9pZCBSZXNvdXJjZUhhbmRsZTo6cmVjZWl2ZWRDcmVkZW50aWFsKAogICAg ICAgICBLVVJMIHVybFRvU3RvcmU7CiAgICAgICAgIGlmIChjaGFsbGVuZ2UuZmFpbHVyZVJlc3Bv bnNlKCkuaHR0cFN0YXR1c0NvZGUoKSA9PSA0MDEpCiAgICAgICAgICAgICB1cmxUb1N0b3JlID0g ZC0+bV9yZXF1ZXN0LnVybCgpOwotICAgICAgICBDcmVkZW50aWFsU3RvcmFnZTo6c2V0KHdlYkNy ZWRlbnRpYWwsIGNvcmUoW2QtPm1fY3VycmVudE1hY0NoYWxsZW5nZSBwcm90ZWN0aW9uU3BhY2Vd KSwgdXJsVG9TdG9yZSk7CisgICAgICAgIAorICAgICAgICBpZiAoIWNyZWRlbnRpYWwuaXNFbXB0 eSgpKQorICAgICAgICAgICAgQ3JlZGVudGlhbFN0b3JhZ2U6OnNldCh3ZWJDcmVkZW50aWFsLCBj b3JlKFtkLT5tX2N1cnJlbnRNYWNDaGFsbGVuZ2UgcHJvdGVjdGlvblNwYWNlXSksIHVybFRvU3Rv cmUpOworICAgICAgICAgICAgCiAgICAgICAgIFtbZC0+bV9jdXJyZW50TWFjQ2hhbGxlbmdlIHNl bmRlcl0gdXNlQ3JlZGVudGlhbDptYWMod2ViQ3JlZGVudGlhbCkgZm9yQXV0aGVudGljYXRpb25D aGFsbGVuZ2U6ZC0+bV9jdXJyZW50TWFjQ2hhbGxlbmdlXTsKICAgICB9IGVsc2UKICNlbmRpZgo= 54318 2010-04-26 10:54:04 -0700 2010-04-26 14:08:17 -0700 change the ChangeLog as per Davin's comment. empty_credential.patch text/plain 1787 yongjun_zhang SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA1ODI2MCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTggQEAKKzIwMTAtMDQtMjYgIFlvbmdqdW4gWmhhbmcgIDx5b25nanVuX3poYW5n QGFwcGxlLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICBEb24ndCBhZGQgZW1wdHkgY3JlZGVudGlhbCB0byBDcmVkZW50aWFsU3RvcmFnZS4KKyAg ICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTM4MTI4CisKKyAg ICAgICAgQW4gZW1wdHkgY3JlZGVudGlhbCBpcyBjdXJyZW50bHkgcmVnYXJkZWQgYXMgYSBpbnZh bGlkIGxvZ2luIGJlY2F1c2UgaXQgbWVhbnMgYSBtaXNzaW5nIHZhbHVlIGluIAorICAgICAgICBw cm90ZWN0aW9uU3BhY2VUb0NyZWRlbnRpYWxNYXAuICBUaGlzIGNoYW5nZSBtYWtlcyBpdCBjb25z aXN0ZW50IHdpdGggY3VycmVudCBXZWJDb3JlJ3MgYmVoYXZpb3IuCisKKyAgICAgICAgTm8gbmV3 IHRlc3RzIGFkZGVkIGJlY2F1c2UgaXQgZG9lc24ndCBjaGFuZ2UgdGhlIGN1cnJlbnQgYmVoYXZp b3IuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9uZXR3b3JrL21hYy9SZXNvdXJjZUhhbmRsZU1hYy5t bToKKyAgICAgICAgKFdlYkNvcmU6OlJlc291cmNlSGFuZGxlOjpyZWNlaXZlZENyZWRlbnRpYWwp OgorCiAyMDEwLTA0LTI2ICBSb21hbiBHZXJzaG1hbiAgPHJvbWFuZ2VAZ29vZ2xlLmNvbT4KIAog ICAgICAgICBSZXZpZXdlZCBieSBBZGFtIEJhcnRoLgpJbmRleDogV2ViQ29yZS9wbGF0Zm9ybS9u ZXR3b3JrL21hYy9SZXNvdXJjZUhhbmRsZU1hYy5tbQo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJDb3JlL3Bs YXRmb3JtL25ldHdvcmsvbWFjL1Jlc291cmNlSGFuZGxlTWFjLm1tCShyZXZpc2lvbiA1ODI1NykK KysrIFdlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9tYWMvUmVzb3VyY2VIYW5kbGVNYWMubW0JKHdv cmtpbmcgY29weSkKQEAgLTU4OCw3ICs1ODgsMTAgQEAgdm9pZCBSZXNvdXJjZUhhbmRsZTo6cmVj ZWl2ZWRDcmVkZW50aWFsKAogICAgICAgICBLVVJMIHVybFRvU3RvcmU7CiAgICAgICAgIGlmIChj aGFsbGVuZ2UuZmFpbHVyZVJlc3BvbnNlKCkuaHR0cFN0YXR1c0NvZGUoKSA9PSA0MDEpCiAgICAg ICAgICAgICB1cmxUb1N0b3JlID0gZC0+bV9yZXF1ZXN0LnVybCgpOwotICAgICAgICBDcmVkZW50 aWFsU3RvcmFnZTo6c2V0KHdlYkNyZWRlbnRpYWwsIGNvcmUoW2QtPm1fY3VycmVudE1hY0NoYWxs ZW5nZSBwcm90ZWN0aW9uU3BhY2VdKSwgdXJsVG9TdG9yZSk7CisgICAgICAgIAorICAgICAgICBp ZiAoIWNyZWRlbnRpYWwuaXNFbXB0eSgpKQorICAgICAgICAgICAgQ3JlZGVudGlhbFN0b3JhZ2U6 OnNldCh3ZWJDcmVkZW50aWFsLCBjb3JlKFtkLT5tX2N1cnJlbnRNYWNDaGFsbGVuZ2UgcHJvdGVj dGlvblNwYWNlXSksIHVybFRvU3RvcmUpOworICAgICAgICAgICAgCiAgICAgICAgIFtbZC0+bV9j dXJyZW50TWFjQ2hhbGxlbmdlIHNlbmRlcl0gdXNlQ3JlZGVudGlhbDptYWMod2ViQ3JlZGVudGlh bCkgZm9yQXV0aGVudGljYXRpb25DaGFsbGVuZ2U6ZC0+bV9jdXJyZW50TWFjQ2hhbGxlbmdlXTsK ICAgICB9IGVsc2UKICNlbmRpZgo= 54329 2010-04-26 14:08:17 -0700 2010-04-26 14:44:01 -0700 Use receivedRequestToContinueWithoutCredential for empty credentials as per Alexey comments. empty_credential.patch text/plain 1673 yongjun_zhang SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA1ODI2MikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMjAgQEAKKzIwMTAtMDQtMjYgIFlvbmdqdW4gWmhhbmcgIDx5b25nanVuX3poYW5n QGFwcGxlLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKyAgICAg ICAgCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0zODEy OAorICAgICAgICBEb24ndCBhZGQgZW1wdHkgY3JlZGVudGlhbCB0byBDcmVkZW50aWFsU3RvcmFn ZS4KKyAgICAgICAgCisgICAgICAgIEFuIGVtcHR5IGNyZWRlbnRpYWwgaXMgY3VycmVudGx5IHJl Z2FyZGVkIGFzIGEgaW52YWxpZCBsb2dpbiBiZWNhdXNlIGl0IG1lYW5zIGEgbWlzc2luZyB2YWx1 ZSBpbiAKKyAgICAgICAgcHJvdGVjdGlvblNwYWNlVG9DcmVkZW50aWFsTWFwLiAgVGhpcyBjaGFu Z2UgbWFrZXMgaXQgY29uc2lzdGVudCB3aXRoIGN1cnJlbnQgV2ViQ29yZSdzIGJlaGF2aW9yCisg ICAgICAgIGJ5IGlnbm9yaW5nIGVtcHR5IGNyZWRlbnRpYWxzLgorICAgICAgICAKKyAgICAgICAg Tm8gbmV3IHRlc3RzIGFkZGVkIHNpbmNlIHRoaXMgb25seSBhZmZlY3RzIGNyZWRlbnRpYWwgZW50 ZXJlZCBieSB1c2VyIGZyb20gY3JlZGVudGlhbCBkaWFsb2csCisgICAgICAgIHdoaWNoIGlzIG5v dCB0ZXN0YWJsZSBpbiBEUlQuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9uZXR3b3JrL21hYy9SZXNv dXJjZUhhbmRsZU1hYy5tbToKKyAgICAgICAgKFdlYkNvcmU6OlJlc291cmNlSGFuZGxlOjpyZWNl aXZlZENyZWRlbnRpYWwpOgorCiAyMDEwLTA0LTI2ICBMYXN6bG8gR29tYm9zICA8bGFzemxvLjEu Z29tYm9zQG5va2lhLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBTaW1vbiBIYXVzbWFubi4K SW5kZXg6IFdlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9tYWMvUmVzb3VyY2VIYW5kbGVNYWMubW0K PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PQotLS0gV2ViQ29yZS9wbGF0Zm9ybS9uZXR3b3JrL21hYy9SZXNvdXJjZUhhbmRs ZU1hYy5tbQkocmV2aXNpb24gNTgyNTcpCisrKyBXZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsvbWFj L1Jlc291cmNlSGFuZGxlTWFjLm1tCSh3b3JraW5nIGNvcHkpCkBAIC01NzIsNiArNTcyLDExIEBA IHZvaWQgUmVzb3VyY2VIYW5kbGU6OnJlY2VpdmVkQ3JlZGVudGlhbCgKICAgICBBU1NFUlQoIWNo YWxsZW5nZS5pc051bGwoKSk7CiAgICAgaWYgKGNoYWxsZW5nZSAhPSBkLT5tX2N1cnJlbnRXZWJD aGFsbGVuZ2UpCiAgICAgICAgIHJldHVybjsKKyAgICAKKyAgICBpZiAoY3JlZGVudGlhbC5pc0Vt cHR5KCkpIHsKKyAgICAgICAgcmVjZWl2ZWRSZXF1ZXN0VG9Db250aW51ZVdpdGhvdXRDcmVkZW50 aWFsKGNoYWxsZW5nZSk7CisgICAgICAgIHJldHVybjsKKyAgICB9CiAKICNpZmRlZiBCVUlMRElO R19PTl9USUdFUgogICAgIGlmIChjcmVkZW50aWFsLnBlcnNpc3RlbmNlKCkgPT0gQ3JlZGVudGlh bFBlcnNpc3RlbmNlTm9uZSkgewo=