Summary: | [soup] Propagate TLS error information for resource requests | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ben Boeckel <mathstuf> | ||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | NEW --- | ||||||||||
Severity: | Normal | CC: | berto, bugs-noreply, cdumez, cgarcia, commit-queue, danw, darin, gustavo, mrobinson | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 129654 | ||||||||||
Attachments: |
|
Description
Ben Boeckel
2014-03-03 20:01:11 PST
Created attachment 225732 [details]
Patch
Attachment 225732 [details] did not pass style-queue:
Total errors found: 0 in 0 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 225732 [details]
Patch
The patch is empty.
Oops. Seems I botched the Tools/Scripts tool I used (on a different machine). I'm using the git repo; what's the proper command to use for submitting a single patch from a branch (I think I used webkit-patch -g HEAD~ 129653 patch-name)? I'll respin once I get home. Created attachment 225858 [details]
Set-the-TLS-error-information-if-a-TLS-error-occurred
FWIW, here's the command I used to attach to the bug: "Tools/Scripts/webkit-patch attach-to-bug -g HEAD -m Patch 129653 TLS-error-propagation.patch".
Attachment 225858 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/network/soup/ResourceErrorSoup.cpp:61: Declaration has space between type name and * in GTlsCertificate *certificate [whitespace/declaration] [3]
ERROR: Source/WebCore/platform/network/soup/ResourceErrorSoup.cpp:64: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/soup/ResourceErrorSoup.cpp:57: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
Total errors found: 3 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 225865 [details]
Set-the-TLS-error-information-if-a-TLS-error-occurred (2)
Style errors fixed.
Comment on attachment 225865 [details] Set-the-TLS-error-information-if-a-TLS-error-occurred (2) View in context: https://bugs.webkit.org/attachment.cgi?id=225865&action=review What does this fix or how does this improve things? Are you fixing a specific bug? > Source/WebCore/platform/network/soup/ResourceErrorSoup.cpp:57 > + if (message && SOUP_STATUS_IS_TRANSPORT_ERROR(message->status_code)) { This should be turned into an early return, so reverse the check and return generic error immediately. > Source/WebCore/platform/network/soup/ResourceErrorSoup.cpp:61 > + ResourceError error = transportError(request, message->status_code, > String::fromUTF8(message->reason_phrase)); > - else > - return genericGError(error, request); > + if (message->status_code == SOUP_STATUS_SSL_FAILED) { > + GTlsCertificate* certificate; Nit: an empty line before the if will make this more readable I think. Thanks for the review. (In reply to comment #8) > What does this fix or how does this improve things? Are you fixing a specific bug? That's filed in bugzilla? No. The problem is that when an SSL error occurs, there's no information available for a browser to say "here's the certificate that failed; do you want to ignore the error?". Currently, there's nothing there since the generic error doesn't contain such information. Of course, things may have changed since I last checked; I'll see if I can get my WebKit repository up to speed tonight and double check the state of things (i.e., whether the patch is necessary and still provides the correct information). > > Source/WebCore/platform/network/soup/ResourceErrorSoup.cpp:57 > > + if (message && SOUP_STATUS_IS_TRANSPORT_ERROR(message->status_code)) { > > This should be turned into an early return, so reverse the check and return generic error immediately. Sounds good. > > Source/WebCore/platform/network/soup/ResourceErrorSoup.cpp:61 > > + ResourceError error = transportError(request, message->status_code, > > String::fromUTF8(message->reason_phrase)); > > - else > > - return genericGError(error, request); > > + if (message->status_code == SOUP_STATUS_SSL_FAILED) { > > + GTlsCertificate* certificate; > > Nit: an empty line before the if will make this more readable I think. Will change. |