Summary: | Don't add empty credential to CredentialStorage. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yongjun Zhang <yongjun_zhang> | ||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, commit-queue, mitz | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Mac (Intel) | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
Yongjun Zhang
2010-04-26 10:06:37 PDT
Created attachment 54315 [details]
First attempt to fix this issue.
Add empty credential check before adding it to CredentialStorage.
Comment on attachment 54315 [details]
First attempt to fix this issue.
Needs a layout test or an explanation in the ChangeLog of why a layout test isn't possible.
Created attachment 54318 [details]
change the ChangeLog as per Davin's comment.
Comment on attachment 54318 [details]
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.
(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. > 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().
Created attachment 54329 [details]
Use receivedRequestToContinueWithoutCredential for empty credentials as per Alexey comments.
Comment on attachment 54329 [details]
Use receivedRequestToContinueWithoutCredential for empty credentials as per Alexey comments.
r=me, thanks!
Comment on attachment 54329 [details] Use receivedRequestToContinueWithoutCredential for empty credentials as per Alexey comments. Clearing flags on attachment: 54329 Committed r58264: <http://trac.webkit.org/changeset/58264> All reviewed patches have been landed. Closing bug. |