Bug 138687 – testRunner.setAlwaysAcceptCookies does not work with NetworkProcess

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Carlos Garcia Campos]

Some tests don't pass when running with the network process enabled

[Alexey Proskuryakov]

Related to this, NetworkProcess picks up cookie accept policy from Safari on Mac, so certain tests fail when the Safari preference is not the default one.

I have a fix for both issues.

[Alexey Proskuryakov]

Created attachment 248468 [details]
proposed fix

[Alexey Proskuryakov]

I should probably remove this Gtk expectation, too.

LayoutTests/platform/gtk/TestExpectations:webkit.org/b/138687 http/tests/xmlhttprequest/cross-origin-cookie-storage.html [ Skip ]

[Carlos Garcia Campos]

Comment on attachment 248468 [details]
proposed fix

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

Thanks ap!, this looks good to me, and I confirm this fixes http/tests/xmlhttprequest/cross-origin-cookie-storage.html in GTK+ port. This changes WebCookieManagerProxy.cpp, so I guess I can't r+ this.

> Tools/WebKitTestRunner/TestInvocation.cpp:652
> +        // FIXME: This doesn't synchronously update the policy in WebProcess. This shouldn't affect tests much, because most loading is in NetworkProcess.

I don't get this comment, the message is also async for the network process.

[Carlos Garcia Campos]

Comment on attachment 248468 [details]
proposed fix

Oops, sorry, I removed the flag :-P

[Alexey Proskuryakov]

Actually, you can r+ it - when the patch author is a WebKit2 owner, anyone can r+.

> I don't get this comment, the message is also async for the network process.

Good point, they are both affected. I was thinking of WebProcess primarily because it is blocked in a sync message send, and then continues parsing without an opportunity to handle the message.

I can re-word the comment as:

// FIXME: This updates the policy in WebProcess and in NetworkProcess asynchronously, which might break some tests' expectations.

[Carlos Garcia Campos]

(In reply to comment #6)
> Actually, you can r+ it - when the patch author is a WebKit2 owner, anyone
> can r+.

Ah, I didn't know that, r=me then.

> > I don't get this comment, the message is also async for the network process.
> 
> Good point, they are both affected. I was thinking of WebProcess primarily
> because it is blocked in a sync message send, and then continues parsing
> without an opportunity to handle the message.
> 
> I can re-word the comment as:
> 
> // FIXME: This updates the policy in WebProcess and in NetworkProcess
> asynchronously, which might break some tests' expectations.

That makes more sense :-)

[Carlos Garcia Campos]

Comment on attachment 248468 [details]
proposed fix

Please, remember to unskip the GTK+ test too.

[Alexey Proskuryakov]

Created attachment 248523 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 248523 [details]
patch for landing

Rejecting attachment 248523 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 248523, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.appspot.com/results/5987861060911104

[Alexey Proskuryakov]

Created attachment 248525 [details]
patch for landing

Annoying. Not sure if we actually have a policy like that.

[WebKit Commit Bot]

Comment on attachment 248525 [details]
patch for landing

Clearing flags on attachment: 248525

Committed r181446: <http://trac.webkit.org/changeset/181446>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.