Bug 102593 – Add an integer identifier field to AuthenticationChallengeBase

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Brady Eidson]

Add an integer identifier field to AuthenticationChallengeBase

This is to support linking two different challenges that might not compare as equal but that represent the same logical authentication challenge.
One example is in an IPC environment where the platform challenge can only exist in one process.

[Brady Eidson]

Created attachment 174824 [details]
Patch v1

[WebKit Review Bot]

Attachment 174824 [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/network/cf/AuthenticationChallenge.h:52:  The parameter name "protectionSpace" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/AuthenticationChallenge.h:52:  The parameter name "response" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/cf/AuthenticationChallenge.h:52:  The parameter name "error" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 8 files


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

[Early Warning System Bot]

Comment on attachment 174824 [details]
Patch v1

Attachment 174824 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14877336

[EFL EWS Bot]

Comment on attachment 174824 [details]
Patch v1

Attachment 174824 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14859835

[kov's GTK+ EWS bot]

Comment on attachment 174824 [details]
Patch v1

Attachment 174824 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14875419

[Brady Eidson]

Created attachment 174827 [details]
Patch v2 - Probably need one more round at EWS

[Brady Eidson]

One more shot at EWS to make sure qt/soup/win don't need changes within WebCore.

[WebKit Review Bot]

Attachment 174827 [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/network/qt/AuthenticationChallenge.h:43:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/platform/network/soup/AuthenticationChallenge.h:45:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/platform/network/win/AuthenticationChallenge.h:41:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 3 in 11 files


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

[Brady Eidson]

Replaced tabs locally.  As long as the rest of EWS passes, I won't reupload for that.

[Build Bot]

Comment on attachment 174827 [details]
Patch v2 - Probably need one more round at EWS

Attachment 174827 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14876374

[Brady Eidson]

Created attachment 174829 [details]
Patch v3 - Hopefully last shot at EWS

[WebKit Review Bot]

Attachment 174829 [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/network/cf/AuthenticationCF.cpp:56:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/cf/AuthenticationCF.cpp:57:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/cf/AuthenticationCF.cpp:64:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/platform/network/cf/AuthenticationCF.cpp:77:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 4 in 12 files


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

[Build Bot]

Comment on attachment 174829 [details]
Patch v3 - Hopefully last shot at EWS

Attachment 174829 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14879295

[Brady Eidson]

Created attachment 174830 [details]
Patch v4 - Come on Windows, you can do it.

[WebKit Review Bot]

Attachment 174830 [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/network/cf/AuthenticationCF.cpp:56:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/cf/AuthenticationCF.cpp:57:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/win/WebURLAuthenticationChallenge.cpp:151:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 3 in 14 files


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

[Brady Eidson]

With EWS finally satisfied, marking r?

[Darin Adler]

Comment on attachment 174830 [details]
Patch v4 - Come on Windows, you can do it.

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

> Source/WebCore/platform/network/AuthenticationChallengeBase.cpp:50
> +    , m_identifier(0)

Seems this constructor should take the identifier as an argument.

> Source/WebCore/platform/network/AuthenticationChallengeBase.cpp:87
> +    m_identifier = 0;

Why need this one field be zeroed out explicitly, but not others, such as m_error?

[Brady Eidson]

(In reply to comment #17)
> (From update of attachment 174830 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174830&action=review
> 
> > Source/WebCore/platform/network/AuthenticationChallengeBase.cpp:50
> > +    , m_identifier(0)
> 
> Seems this constructor should take the identifier as an argument.

Eventually it might.

But for now the usefulness of the identifier is tied up directly with creation from a platform authentication challenge.  This will be clear in https://bugs.webkit.org/show_bug.cgi?id=102592

> 
> > Source/WebCore/platform/network/AuthenticationChallengeBase.cpp:87
> > +    m_identifier = 0;
> 
> Why need this one field be zeroed out explicitly, but not others, such as m_error?

I'll leave this m_identifier = 0; out of this patch and re-add it with the patch upcoming in https://bugs.webkit.org/show_bug.cgi?id=102592 along with a more thorough nulling-out.

[Brady Eidson]

http://trac.webkit.org/changeset/135055

[Adam Barth]

Comment on attachment 174830 [details]
Patch v4 - Come on Windows, you can do it.

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

> Source/WebCore/platform/network/AuthenticationChallengeBase.cpp:35
> +    , m_identifier(0)

Historically, the Chromium port has made an effort to keep identifiers like this out of WebCore because they are usually a concern of multiprocess code an not a concern of WebCore.  Would it be possible to keep these WebKit2-only concerns in the WebKit2 layer so as not to add complexity to ports that don't care to use WebKit2?

[Brady Eidson]

(In reply to comment #20)
> (From update of attachment 174830 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174830&action=review
> 
> > Source/WebCore/platform/network/AuthenticationChallengeBase.cpp:35
> > +    , m_identifier(0)
> 
> Historically, the Chromium port has made an effort to keep identifiers like this out of WebCore because they are usually a concern of multiprocess code an not a concern of WebCore.

We've done a lot of the same in WebKit2.

> Would it be possible to keep these WebKit2-only concerns in the WebKit2 layer so as not to add complexity to ports that don't care to use WebKit2?

Each situation should probably be viewed on its own merits.

In WebKit2's CoreIPC we commonly pass WebCore types over the wire, and there's been plenty of periodic work to make changes in WebCore to support this.

The WebCore/platform directly is both an interface layer to adapt platforms to WebCore and a support layer to provide those ports the support they need for their goals.  Especially since the frequency of AuthenticationChallenges is so low and there's basically no risk for performance or memory usage issues, it seemed reasonable to advance in this manner.

Especially compared to some of the much larger and more turbulent refactorings in the past that were solely to support one port's IPC architecture, I'm not sure what the big deal is about this one.

If you feel strongly about it I can revisit and put the identifier member in each of the relevant ports AuthenticationChallenge.h/cpps.  But I'm not sure how that is better.

[Adam Barth]

> I'm not sure what the big deal is about this one.

Oh, I don't think it's such a big deal.  It's just a design pattern that we consciously try to avoid, and I wanted to make sure we were all on the same page.  It sounds like you agree with the principle in general.

[Brady Eidson]

(In reply to comment #22)
> > I'm not sure what the big deal is about this one.
> 
> Oh, I don't think it's such a big deal.  It's just a design pattern that we consciously try to avoid, and I wanted to make sure we were all on the same page.  It sounds like you agree with the principle in general.

I think we agree in general.

[Alexey Proskuryakov]

> Historically, the Chromium port has made an effort to keep identifiers like this out of WebCore because they are usually a concern of multiprocess code an not a concern of WebCore.  Would it be possible to keep these WebKit2-only concerns in the WebKit2 layer so as not to add complexity to ports that don't care to use WebKit2?

Yes, I agree. In fact, WebKit2 already uses a better technique when exchanging AuthenticationChallenges between WebProcess and UI Process. A transient ID is generated just for IPC, and is not part of AuthenticationChallenge.

I'll work on a patch that removes the identifier from AuthenticationChallenge. This patch will also make NetworkProcess IPC more efficient, as we won't be sending an unchanged AuthenticationChallenge in responses.