Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

Issue 1855383002: Add DumpWithoutCrashing to RenderFrameMessageFilter cookie kills (Closed)

Created:
3 years, 1 month ago by ncarter (slow)
Modified:
3 years ago
CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@doghouse_now
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add DumpWithoutCrashing and crash keys to get more context for RFMF_SET_COOKIE_BAD_ORIGIN and RFMF_GET_COOKIES_BAD_ORIGIN renderer kills. This is all temporary code, which will be reverted after we understand the issue. BUG=600441 TEST=RenderFrameMessageFilterBrowserTest.CrossSiteCookieSecurityEnforcement CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/0e6e9faae9e11de8684440b128e75569b98c7553 Cr-Commit-Position: refs/heads/master@{#391396}

Patch Set 1 #

Patch Set 2 : Rebase. #

Patch Set 3 : Rebase / fix merge conficts #

Total comments: 2

Patch Set 4 : Rebase, change scoped_ptr to unique_ptr, fix crash key names #

Patch Set 5 : Use possibly_invalid_spec #

Total comments: 2

Patch Set 6 : Add second dump site. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+49 lines, -1 line) Patch
M chrome/common/crash_keys.cc View 1 2 3 4 5 1 chunk +5 lines, -0 lines 0 comments Download
M content/browser/child_process_security_policy_impl.h View 1 2 3 2 chunks +6 lines, -1 line 0 comments Download
M content/browser/child_process_security_policy_impl.cc View 1 2 3 4 5 3 chunks +16 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_message_filter.cc View 1 2 3 4 5 3 chunks +22 lines, -0 lines 0 comments Download

Messages

Total messages: 22 (10 generated)
ncarter (slow)
Hi avi, please take a look
3 years, 1 month ago (2016-04-04 21:38:59 UTC) #4
Avi (use Gerrit)
:( https://codereview.chromium.org/1855383002/diff/40001/content/browser/child_process_security_policy_impl.cc File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/1855383002/diff/40001/content/browser/child_process_security_policy_impl.cc#newcode837 content/browser/child_process_security_policy_impl.cc:837: "policy_origin_lock", state == security_state_.end() The key constant in ...
3 years, 1 month ago (2016-04-05 01:41:24 UTC) #5
ncarter (slow)
On 2016/04/05 01:41:24, Avi wrote: > :( > > https://codereview.chromium.org/1855383002/diff/40001/content/browser/child_process_security_policy_impl.cc > File content/browser/child_process_security_policy_impl.cc (right): > ...
3 years, 1 month ago (2016-04-05 18:35:52 UTC) #6
ncarter (slow)
I'm reviving this patch. We fixed 90% of the kills, but we still don't understand ...
3 years ago (2016-05-03 00:16:01 UTC) #7
Avi (use Gerrit)
lgtm
3 years ago (2016-05-03 01:45:28 UTC) #8
Charlie Reis
[Drive-by] https://codereview.chromium.org/1855383002/diff/80001/content/browser/frame_host/render_frame_message_filter.cc File content/browser/frame_host/render_frame_message_filter.cc (right): https://codereview.chromium.org/1855383002/diff/80001/content/browser/frame_host/render_frame_message_filter.cc#newcode299 content/browser/frame_host/render_frame_message_filter.cc:299: bad_message::RFMF_GET_COOKIES_BAD_ORIGIN); Isn't this the more frequent kill? Maybe ...
3 years ago (2016-05-03 17:33:34 UTC) #10
ncarter (slow)
Done. +rsesek for crash_keys.cc https://codereview.chromium.org/1855383002/diff/80001/content/browser/frame_host/render_frame_message_filter.cc File content/browser/frame_host/render_frame_message_filter.cc (right): https://codereview.chromium.org/1855383002/diff/80001/content/browser/frame_host/render_frame_message_filter.cc#newcode299 content/browser/frame_host/render_frame_message_filter.cc:299: bad_message::RFMF_GET_COOKIES_BAD_ORIGIN); On 2016/05/03 17:33:34, Charlie ...
3 years ago (2016-05-03 18:50:34 UTC) #13
Robert Sesek
lgtm
3 years ago (2016-05-03 21:54:32 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1855383002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1855383002/100001
3 years ago (2016-05-03 22:06:53 UTC) #17
Robert Sesek
FYI you're racing https://codereview.chromium.org/1925993004/ which will require adding the key in a second place.
3 years ago (2016-05-03 22:07:08 UTC) #18
commit-bot: I haz the power
Committed patchset #6 (id:100001)
3 years ago (2016-05-03 23:34:25 UTC) #20
commit-bot: I haz the power
3 years ago (2016-05-03 23:35:28 UTC) #22
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/0e6e9faae9e11de8684440b128e75569b98c7553
Cr-Commit-Position: refs/heads/master@{#391396}

Powered by Google App Engine
This is Rietveld 408576698