New issue
Advanced search Search tips

Issue 898704 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 29
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-10-29
OS: Windows
Pri: 1
Type: Bug
Proj-Servicification



Sign in to add a comment

Requests with network service enabled may not have the correct cookies

Project Member Reported by rmcelrath@google.com, Oct 24

Issue description

If you set a cookie via JS and then send a request immediately after, the request might not have the cookie set.

external/wpt/cookie-store/httponly_cookies.https.window.html is flakily failing due to this, more reliably on debug builds.
 
Labels: Proj-Servicification-Stable M-71
Cc: dxie@chromium.org
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 25

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a71979244edd931ed1837d0670423e026011749c

commit a71979244edd931ed1837d0670423e026011749c
Author: Robbie McElrath <rmcelrath@chromium.org>
Date: Thu Oct 25 21:52:12 2018

Make setting cookies synchronous.

Right now setting cookies is asynchronous, which can lead to race
conditions where setting a cookie then sending a request doesn't result
in the request having the cookie set (see the bug).

Bug:  898704 
Change-Id: I1d362b0312942f57b933352b85c60cc1ff855fba
Reviewed-on: https://chromium-review.googlesource.com/c/1298489
Commit-Queue: Robbie McElrath <rmcelrath@chromium.org>
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602878}
[modify] https://crrev.com/a71979244edd931ed1837d0670423e026011749c/content/browser/frame_host/render_frame_message_filter.cc
[modify] https://crrev.com/a71979244edd931ed1837d0670423e026011749c/content/renderer/renderer_webcookiejar_impl.cc

Status: Fixed (was: Assigned)
Status: Assigned (was: Fixed)
Oops, reopening as this should be cherry picked into 71.
Labels: Merge-Request-71
Labels: OS-Windows
Adding OS=Windows, next time pls apply appropriate OSs label. 

How is the change looking in canary and how safe it is to merge to M71?
Labels: Hotlist-KnownIssue
It made it to canary 3592, and I haven't seen any crashes related to it yet, though it's only been ~13 hours. We can wait until Monday if you want to give it more time.

It should be safe to merge.
NextAction: 2018-10-29
Thank you,pls update bug with additional canary coverage data on Monday morning.
Project Member

Comment 11 by sheriffbot@chromium.org, Oct 26

Labels: -Merge-Request-71 Hotlist-Merge-Review Merge-Review-71
This bug requires manual review: M71 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
The NextAction date has arrived: 2018-10-29
I'm not seeing any crashes from over the weekend either.
Labels: -Merge-Review-71 Merge-Approved-71
Approving merge to M71 branch 3578 based on comment #13. Pls merge ASAP so we can pick it up for this week beta. Thank you.
Project Member

Comment 15 by bugdroid1@chromium.org, Oct 29

Labels: -merge-approved-71 merge-merged-3578
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/86202450f25e720d5b87304467adf1dbb1ec4599

commit 86202450f25e720d5b87304467adf1dbb1ec4599
Author: Robbie McElrath <rmcelrath@chromium.org>
Date: Mon Oct 29 17:55:01 2018

Make setting cookies synchronous.

Right now setting cookies is asynchronous, which can lead to race
conditions where setting a cookie then sending a request doesn't result
in the request having the cookie set (see the bug).

Bug:  898704 
Change-Id: I1d362b0312942f57b933352b85c60cc1ff855fba
Reviewed-on: https://chromium-review.googlesource.com/c/1298489
Commit-Queue: Robbie McElrath <rmcelrath@chromium.org>
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#602878}(cherry picked from commit a71979244edd931ed1837d0670423e026011749c)
Reviewed-on: https://chromium-review.googlesource.com/c/1302322
Reviewed-by: Robbie McElrath <rmcelrath@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#371}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/86202450f25e720d5b87304467adf1dbb1ec4599/content/browser/frame_host/render_frame_message_filter.cc
[modify] https://crrev.com/86202450f25e720d5b87304467adf1dbb1ec4599/content/renderer/renderer_webcookiejar_impl.cc

Status: Fixed (was: Assigned)
Labels: Merge-Merged-71-3578
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/86202450f25e720d5b87304467adf1dbb1ec4599

Commit: 86202450f25e720d5b87304467adf1dbb1ec4599
Author: rmcelrath@chromium.org
Commiter: rmcelrath@chromium.org
Date: 2018-10-29 17:55:01 +0000 UTC

Make setting cookies synchronous.

Right now setting cookies is asynchronous, which can lead to race
conditions where setting a cookie then sending a request doesn't result
in the request having the cookie set (see the bug).

Bug:  898704 
Change-Id: I1d362b0312942f57b933352b85c60cc1ff855fba
Reviewed-on: https://chromium-review.googlesource.com/c/1298489
Commit-Queue: Robbie McElrath <rmcelrath@chromium.org>
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#602878}(cherry picked from commit a71979244edd931ed1837d0670423e026011749c)
Reviewed-on: https://chromium-review.googlesource.com/c/1302322
Reviewed-by: Robbie McElrath <rmcelrath@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#371}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}

Sign in to add a comment