Bug 87660 – [BlackBerry] http authentication challenge issue when loading subresource

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

[Jonathan Dong]

RIM PR: 160585

way to reproduce:
try to load an digest http authenticated page which has a subresource, e.g. an img tag pointing to a png file which needs the same authentication. the auth challenge dialog would pop up twice, even you've entered the right credentials for the main resource.

root cause:
we load store the credentials in handleNotifyDone, after finish loading the whole main resource, which is too late. We should do it as soon as we receives the headers which indicate that we've passed the authentication. Then we can find the credential information from CredentialStorage when loading subresource.

[Jonathan Dong]

Created attachment 144357 [details]
Patch

[Joe Mason]

LGTM

[Rob Buis]

Comment on attachment 144357 [details]
Patch

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

Looks good, but can maybe cleaned up some more.

> Source/WebCore/ChangeLog:14
> +        No new tests because of no behavior changed.

No new tests since there is no change in behavior.

> Source/WebCore/platform/network/blackberry/NetworkJob.cpp:214
> +    // the saved credentials.

The comment is quite obvious, and the code below it is readable, maybe not needed.

> Source/WebCore/platform/network/blackberry/NetworkJob.cpp:218
> +        purgeCredentials();

Is it useful to log the fact that there was an error? How about dealing with isError case first, you do not need the ! operation in that case.

[Jonathan Dong]

Created attachment 144438 [details]
Patch

[Jonathan Dong]

Created attachment 144443 [details]
Patch

[Jonathan Dong]

Created attachment 144446 [details]
Patch

[Jonathan Dong]

(In reply to comment #6)
> Created an attachment (id=144446) [details]
> Patch

after talk with Joe, we agreed to keep the exact same code as before, just move them to notifyStatusReceived. We need to think more before being sure we are right about that, such as 500 or 404 or something else, and we need have more tests. For now the way won't be perfect but at least we won't make things worse.

[Rob Buis]

Hi Jonathan,

(In reply to comment #7)
> (In reply to comment #6)
> > Created an attachment (id=144446) [details] [details]
> > Patch
> 
> after talk with Joe, we agreed to keep the exact same code as before, just move them to notifyStatusReceived. We need to think more before being sure we are right about that, such as 500 or 404 or something else, and we need have more tests. For now the way won't be perfect but at least we won't make things worse.

So I made my comment because if the isError is true but isUnauthorized is false, we would not report/log this error, isn't that losing information? Hence my log suggestion, why not always log if there is an error?
Cheers,

Rob.

[Joe Mason]

(In reply to comment #8)
> So I made my comment because if the isError is true but isUnauthorized is false, we would not report/log this error, isn't that losing information? Hence my log suggestion, why not always log if there is an error?

Of coures we report that error.  If isError is true but isUnauthorized is false, it just falls through to existing error handling and doesn't touch the credentials at all.

[Rob Buis]

Comment on attachment 144446 [details]
Patch

Looks good.

[WebKit Review Bot]

Comment on attachment 144446 [details]
Patch

Clearing flags on attachment: 144446

Committed r118799: <http://trac.webkit.org/changeset/118799>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.