Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(186)

Issue 524683002: Retry Set RequestContextObject for PNaCl pexe fetches, to fix On-Demand update. (Closed)

Created:
6 years, 3 months ago by jvoung (off chromium)
Modified:
6 years, 3 months ago
CC:
chromium-reviews, native-client-reviews_googlegroups.com
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Retry Set RequestContextObject for PNaCl pexe fetches, to fix On-Demand update. There was a race condition in the added test check in the previous attempt. That made the test Super Flaky. This is more noticeable in the Release builds than the Debug build that I was working with. (see https://codereview.chromium.org/471233003/) With a Release build I was able to repro the flakiness (40% failure over 200 runs). After the fix, I tested 500 runs and the success rate is now 100%. I can't say it won't flake for other reasons, but this fixes the main flake that was introduced. Historically, the test does ocassionally need to be retried due to timeouts, according to the bot log history. We may want to split the CORS and the non-CORS test case to see if that will help w/ timeouts. I didn't want to do that for this patch, since it would get much more complicated and this is intended for merging to M38. Retry with the race condition fixed. Also re-enable the test since it got disabled by another CL for being flaky. BUG=401755 BUG=315328 Committed: https://crrev.com/efd4b3c8968ae61cdfcdbce0f8068e98b5cea6ff Cr-Commit-Position: refs/heads/master@{#292807}

Patch Set 1 #

Patch Set 2 : the actual fix, compare to patchset 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+58 lines, -5 lines) Patch
M chrome/test/nacl/pnacl_header_test.h View 3 chunks +27 lines, -0 lines 0 comments Download
M chrome/test/nacl/pnacl_header_test.cc View 1 5 chunks +30 lines, -5 lines 0 comments Download
M components/nacl/renderer/ppb_nacl_private_impl.cc View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 7 (2 generated)
jvoung (off chromium)
6 years, 3 months ago (2014-08-29 17:55:47 UTC) #2
Nick Bray (chromium)
owners LGTM
6 years, 3 months ago (2014-08-29 22:43:35 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jvoung@chromium.org/524683002/20001
6 years, 3 months ago (2014-08-30 21:25:00 UTC) #5
commit-bot: I haz the power
Committed patchset #2 (id:20001) as 7f61515a1fc5c236a9b7ae32305fd282810d8605
6 years, 3 months ago (2014-08-30 21:49:47 UTC) #6
commit-bot: I haz the power
6 years, 3 months ago (2014-09-10 03:14:49 UTC) #7
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/efd4b3c8968ae61cdfcdbce0f8068e98b5cea6ff
Cr-Commit-Position: refs/heads/master@{#292807}

Powered by Google App Engine
This is Rietveld 408576698