Bug 97397 – [BlackBerry] Fix assertion in NetworkJob::notifyChallengeResult.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit BlackBerry

[Lyon Chen]

Changeset 129251 for https://bugs.webkit.org/show_bug.cgi?id=97348 caused an assertion. This is due to calling to NetworkJob::updateDeferLoadingCount() is not balanced. It's called for increment only when a authentication challenge is request, but will be called for decrement no matter authentication challenge is request or not.

[Lyon Chen]

+ Yong, Joe, and Rob.

[Lyon Chen]

Created attachment 165268 [details]
patch for 97397.

[Joe Mason]

Comment on attachment 165268 [details]
patch for 97397.

View in context: https://bugs.webkit.org/attachment.cgi?id=165268&action=review

> Source/WebCore/platform/network/blackberry/NetworkJob.cpp:-849
> -    if (result != AuthenticationChallengeSuccess || protectionSpace.host().isEmpty() || !url.isValid()) {

What happened to the protectionSpace and url checks in the refactored code?

[Yong Li]

Comment on attachment 165268 [details]
patch for 97397.

LGTM

[Yong Li]

Oops. didn't see Joe's question. will wait for Lyon's answer

[Lyon Chen]

(In reply to comment #3)
> (From update of attachment 165268 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165268&action=review
> 
> > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:-849
> > -    if (result != AuthenticationChallengeSuccess || protectionSpace.host().isEmpty() || !url.isValid()) {
> 
> What happened to the protectionSpace and url checks in the refactored code?

Sorry for the late response. I think the URL and protectionSpace checking is not necessary because if they are invalid we should fail somewhere already. For example url is checked before, and a url without host (or empty host) could not possibly asking for an authentication challenge.

[Joe Mason]

Comment on attachment 165268 [details]
patch for 97397.

View in context: https://bugs.webkit.org/attachment.cgi?id=165268&action=review

>>> Source/WebCore/platform/network/blackberry/NetworkJob.cpp:-849
>>> -    if (result != AuthenticationChallengeSuccess || protectionSpace.host().isEmpty() || !url.isValid()) {
>> 
>> What happened to the protectionSpace and url checks in the refactored code?
> 
> Sorry for the late response. I think the URL and protectionSpace checking is not necessary because if they are invalid we should fail somewhere already. For example url is checked before, and a url without host (or empty host) could not possibly asking for an authentication challenge.

I disagree - url and protectionSpace are passed in as parameters, so they should be checked in case the call site changes. Just from looking at the function on its own its not clear that they're always correct, and we shouldn't require people to find every call site to reason about correctness. Or at least there should be an assert.

[Lyon Chen]

(In reply to comment #7)
> I disagree - url and protectionSpace are passed in as parameters, so they should be checked in case the call site changes. Just from looking at the function on its own its not clear that they're always correct, and we shouldn't require people to find every call site to reason about correctness. Or at least there should be an assert.

OK, I prefer the assertions, as these 2 values (the url here and host) are not user input, when there is a problem with url or protectionSpace host at this stage, it must be a software error.

[Lyon Chen]

Created attachment 168129 [details]
Updated patch for 97397.

This patch also includes the original patch for bug 97348 (https://bugs.webkit.org/show_bug.cgi?id=97348), because that patch is reverted in changeset 129419 (http://trac.webkit.org/changeset/129419) due to other reason.

[WebKit Review Bot]

Attachment 168129 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/blackberry/AuthenticationChallengeManager.h:53:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/blackberry/AuthenticationChallengeManager.h:54:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/blackberry/AuthenticationChallengeManager.h:55:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/blackberry/AuthenticationChallengeManager.h:56:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/blackberry/AuthenticationChallengeManager.h:61:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/blackberry/AuthenticationChallengeManager.h:62:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/blackberry/AuthenticationChallengeManager.h:63:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:726:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/platform/network/blackberry/NetworkJob.cpp:800:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/platform/blackberry/AuthenticationChallengeManager.cpp:50:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/blackberry/AuthenticationChallengeManager.cpp:51:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/blackberry/AuthenticationChallengeManager.cpp:52:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/blackberry/AuthenticationChallengeManager.cpp:53:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/blackberry/AuthenticationChallengeManager.cpp:100:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/blackberry/AuthenticationChallengeManager.cpp:101:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/blackberry/AuthenticationChallengeManager.cpp:159:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/blackberry/AuthenticationChallengeManager.cpp:160:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/blackberry/AuthenticationChallengeManager.cpp:161:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/blackberry/AuthenticationChallengeManager.cpp:162:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/blackberry/AuthenticationChallengeManager.cpp:205:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/blackberry/AuthenticationChallengeManager.cpp:206:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/blackberry/AuthenticationChallengeManager.cpp:207:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 22 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Lyon Chen]

Created attachment 168134 [details]
Updated patch that conforms to new indention style requirement.

[WebKit Review Bot]

Comment on attachment 168134 [details]
Updated patch that conforms to new indention style requirement.

Clearing flags on attachment: 168134

Committed r131014: <http://trac.webkit.org/changeset/131014>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.