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

Issue 1946143002: Destroy (Password)AutofillAgent safely (Closed)

Created:
2 years, 10 months ago by vabr (Chromium)
Modified:
2 years, 10 months ago
Reviewers:
dvadym, robwu
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, vabr+watchlistpasswordmanager_chromium.org, rouslan+autofill_chromium.org, jam, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org, bondd+autofillwatch_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org, vabr+watchlistautofill_chromium.org, mkwst+watchlist-passwords_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Destroy (Password)AutofillAgent safely AutofillAgent and related code often edits field values. Those edits may trigger JavaScript capable of deleting the associated frame. Currently, AutofillAgent and related classes are RenderFrameObservers and delete themselves on the frame deletion. This results in use-after-free if the deletion happens up in the stack and there is still the method which changed the field value down on the stack. Therefore this CL postpones deletion by sending a DeleteSoon task on the frame destruction. The CL also changes a couple of places relying on render frame being alive if the observer is alive to handle a null frame gratefully. R=dvadym@chromium.org BUG=609010, 609007, 608100, 608101 Committed: https://crrev.com/d62bc3e6e2c3be6bbb01fa325e3389f089974017 Cr-Commit-Position: refs/heads/master@{#391524}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Total comments: 2

Patch Set 4 : Also setChecked #

Unified diffs Side-by-side diffs Delta from patch set Stats (+54 lines, -6 lines) Patch
M components/autofill/content/renderer/autofill_agent.h View 1 2 2 chunks +9 lines, -0 lines 0 comments Download
M components/autofill/content/renderer/autofill_agent.cc View 1 2 chunks +16 lines, -1 line 0 comments Download
M components/autofill/content/renderer/form_autofill_util.cc View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M components/autofill/content/renderer/password_autofill_agent.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M components/autofill/content/renderer/password_autofill_agent.cc View 1 1 chunk +4 lines, -0 lines 0 comments Download
M components/autofill/content/renderer/password_generation_agent.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M components/autofill/content/renderer/password_generation_agent.cc View 1 6 chunks +19 lines, -5 lines 0 comments Download

Messages

Total messages: 21 (7 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1946143002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1946143002/1
2 years, 10 months ago (2016-05-04 08:37:27 UTC) #2
vabr (Chromium)
Hi Vadym, Please have a look. I checked that this fixes the issue from http://crbug.com/609007. ...
2 years, 10 months ago (2016-05-04 08:37:53 UTC) #3
vabr (Chromium)
Turns out this was not really doing much, and the fact the issue was not ...
2 years, 10 months ago (2016-05-04 09:16:36 UTC) #4
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
2 years, 10 months ago (2016-05-04 09:43:46 UTC) #6
vabr (Chromium)
2 years, 10 months ago (2016-05-04 13:26:58 UTC) #7
vabr (Chromium)
Hi Vadym, PTAL. I changed the implementation substantially, so please ignore the previous patch sets. ...
2 years, 10 months ago (2016-05-04 14:04:41 UTC) #9
robwu
https://codereview.chromium.org/1946143002/diff/40001/components/autofill/content/renderer/form_autofill_util.cc File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/1946143002/diff/40001/components/autofill/content/renderer/form_autofill_util.cc#newcode898 components/autofill/content/renderer/form_autofill_util.cc:898: return; Are you sure that |field| is still valid? ...
2 years, 10 months ago (2016-05-04 14:35:38 UTC) #11
vabr (Chromium)
Also setChecked
2 years, 10 months ago (2016-05-04 14:49:29 UTC) #12
vabr (Chromium)
Thanks. Vaclav https://codereview.chromium.org/1946143002/diff/40001/components/autofill/content/renderer/form_autofill_util.cc File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/1946143002/diff/40001/components/autofill/content/renderer/form_autofill_util.cc#newcode898 components/autofill/content/renderer/form_autofill_util.cc:898: return; On 2016/05/04 14:35:38, robwu wrote: > ...
2 years, 10 months ago (2016-05-04 14:49:46 UTC) #13
dvadym
LGTM
2 years, 10 months ago (2016-05-04 14:54:34 UTC) #14
vabr (Chromium)
Thanks. Rob, I noticed your http://crbug.com/608101#c9, but I am running out of time before holiday, ...
2 years, 10 months ago (2016-05-04 15:48:04 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1946143002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1946143002/60001
2 years, 10 months ago (2016-05-04 15:54:13 UTC) #17
commit-bot: I haz the power
Committed patchset #4 (id:60001)
2 years, 10 months ago (2016-05-04 15:59:02 UTC) #19
commit-bot: I haz the power
2 years, 10 months ago (2016-05-04 16:00:04 UTC) #21
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/d62bc3e6e2c3be6bbb01fa325e3389f089974017
Cr-Commit-Position: refs/heads/master@{#391524}

Powered by Google App Engine
This is Rietveld 408576698