Bug 184890 – Ensure DNT is set for redirections handled in NetworkProcess

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Ensure DNT is set for redirections handled in NetworkProcess

[youenn fablet]

Created attachment 338592 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/39651760>

[youenn fablet]

Created attachment 338596 [details]
Patch

[EWS Watchlist]

Comment on attachment 338596 [details]
Patch

Attachment 338596 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7414264

New failing tests:
http/wpt/fetch/dnt-header-after-redirection.html

[EWS Watchlist]

Created attachment 338598 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 338596 [details]
Patch

Attachment 338596 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7414701

New failing tests:
http/wpt/fetch/dnt-header-after-redirection.html

[EWS Watchlist]

Created attachment 338601 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[youenn fablet]

Created attachment 338619 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 338619 [details]
Patch

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

> Source/WebKit/ChangeLog:13
> +        Covered by added test.

Please enumerate tests as done in any other change log entry.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:77
> +        m_dntHeaderValue = "1";

Why don't we use a boolean flag instead? I guess you're thinking that could potentially use a different value in the future?

> LayoutTests/http/wpt/fetch/dnt-header-after-redirection.html:21
> +    for (var cptr = 0; cptr < 20; cptr++) {

Why do we need to repeat this 20 times? That seems quite arbitrary.

> LayoutTests/http/wpt/fetch/dnt-header-after-redirection.html:22
> +        waitFor(test, 10);

What is this await for? Like code, test should make it self evident why every line of code exists.
This waitFor is very mysterious as is.

> LayoutTests/http/wpt/fetch/dnt-header-after-redirection.html:35
> +        return Promise.reject("Test requires internals API");

You can just throw an exception instead of explicitly creating a rejected promise.
But you're not using internals at all so what is this check for?
r- because either this code or comment is wrong.

[youenn fablet]

> > Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:77
> > +        m_dntHeaderValue = "1";
> 
> Why don't we use a boolean flag instead? I guess you're thinking that could
> potentially use a different value in the future?

"0" is a valid value meaning user is ok to be tracked. Probably not used a lot but should be supported.
As such, it should be conveyed redirection after redirection.

> > LayoutTests/http/wpt/fetch/dnt-header-after-redirection.html:21
> > +    for (var cptr = 0; cptr < 20; cptr++) {
> 
> Why do we need to repeat this 20 times? That seems quite arbitrary.

We are polling the server until we get the result we expect or we fail.
We need to do polling because we do not always know from JS when the load reached the server.
This is a usual pattern in some WPT tests, like in beacon tests.
Usually 20 times is largely sufficient even with very slow bots.

> What is this await for? Like code, test should make it self evident why
> every line of code exists.
> This waitFor is very mysterious as is.

The waitFor is used elsewhere in LayouTests.
Test should be await waitFor(test, 10) 

> > LayoutTests/http/wpt/fetch/dnt-header-after-redirection.html:35
> > +        return Promise.reject("Test requires internals API");
> 
> You can just throw an exception instead of explicitly creating a rejected
> promise.
> But you're not using internals at all so what is this check for?
> r- because either this code or comment is wrong.

The test is relying on testRunner.setPrivateBrowsingMode, so the code and comment seem correct.
I'll update the message to be clear about testRunner and private browsing mode.

[youenn fablet]

Created attachment 338634 [details]
Patch

[youenn fablet]

Created attachment 338637 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 338637 [details]
Patch

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

> LayoutTests/http/wpt/fetch/dnt-header-after-redirection.html:40
> +    video.src = "resources/redirect.py?location=" + encodeURIComponent(finalURL);
> +

Why can't we wait for the load or error event of video and check the token instead?

[youenn fablet]

> > LayoutTests/http/wpt/fetch/dnt-header-after-redirection.html:40
> > +    video.src = "resources/redirect.py?location=" + encodeURIComponent(finalURL);
> > +
> 
> Why can't we wait for the load or error event of video and check the token
> instead?

It could probably be done this way too.
Polling is probably more robust though in case there is a difference between the different ports and we need polling for beacon anyway.

[Ryosuke Niwa]

Comment on attachment 338637 [details]
Patch

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

r=me if the following comments are addressed.

> LayoutTests/http/wpt/fetch/dnt-header-after-redirection.html:21
> +    for (var cptr = 0; cptr < 20; cptr++) {

Use let and non-abbreviated variable name such as counter.
Also, 20 should be stored in a variable with a descriptive name such as the maxPollCount.

> LayoutTests/http/wpt/fetch/dnt-header-after-redirection.html:35
> +    if (!window.testRunner)
> +        throw "Test requires testRunner API to turn on private browsing";

It appears to me that this test can be run manually if you opened it in a private browsing mode.
We should add that description in the test instead of error'ing out like this.

[Ryosuke Niwa]

It's very important that these tests can be manually ran so that e.g. when we have a release of macOS or iOS, we can manually verify that the bug got fixed / feature works as intended.

[youenn fablet]

Created attachment 338758 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 338758 [details]
Patch for landing

Clearing flags on attachment: 338758

Committed r231005: <https://trac.webkit.org/changeset/231005>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.