Destroy (Password)AutofillAgent safely

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}

[vabr (Chromium), 2016-05-04 08:37:22]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-04 08:37:27]

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

[vabr (Chromium), 2016-05-04 08:37:53]

Hi Vadym,

Please have a look.

I checked that this fixes the issue from http://crbug.com/609007.

Cheers,
Vaclav

[vabr (Chromium), 2016-05-04 09:16:36]

Turns out this was not really doing much, and the fact the issue was not
observed after the patch was just a flakiness.

I will redo this fix, focussing on the real culprit (the LegacyAutofillAgent not
being aware of the shutdown), and ping the CL for review when ready.

Thanks for your patience!

Vaclav

[commit-bot: I haz the power, 2016-05-04 09:43:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-04 09:43:46]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-05-04 13:54:55]

Description was changed from

==========
Invalidate pointers to (Password)AutofillAgent on frame destruction

In https://codereview.chromium.org/1943873002, the destruction of the agent was
postponed to a separate task, the (Password)AutofillAgent was no longer
destroyed on the render frame destruction, but in a separately posted task.

However, the agent must not be used after the frame destruction (and before the
posted deletion happens). To achieve that, this CL also invalidates the weak
pointers to the agent on the destruction of the render frame.

R=dvadym@chromium.org
BUG=609010,609007,608100,608101
==========

to

==========
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
==========

[vabr (Chromium), 2016-05-04 14:04:41]

Hi Vadym,

PTAL. I changed the implementation substantially, so please ignore the previous
patch sets.

I has manually checked that the reproduction cases provided on the associated
bugs were fixed. I am trying to figure out how to make some regression tests,
but to speed up patching and merging this, considering a separate CL for those.

Thanks!
Vaclav

[robwu, 2016-05-04 14:35:37]

rob@robwu.nl changed reviewers:
+ rob@robwu.nl

[robwu, 2016-05-04 14:35:38]

https://codereview.chromium.org/1946143002/diff/40001/components/autofill/con...
File components/autofill/content/renderer/form_autofill_util.cc (right):

https://codereview.chromium.org/1946143002/diff/40001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:898: return;
Are you sure that |field| is still valid?
One of the new test cases from https://crbug.com/608101#c6 shows this stack:
https://bugs.chromium.org/p/chromium/issues/attachmentText?aid=233641.

Although I only reported about setValue, setChecked (a few lines back) should
also be fixed. Similarly for FormCache::ClearFormWithElement.

[vabr (Chromium), 2016-05-04 14:49:29]

Also setChecked

[vabr (Chromium), 2016-05-04 14:49:46]

Thanks.
Vaclav

https://codereview.chromium.org/1946143002/diff/40001/components/autofill/con...
File components/autofill/content/renderer/form_autofill_util.cc (right):

https://codereview.chromium.org/1946143002/diff/40001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:898: return;
On 2016/05/04 14:35:38, robwu wrote:
> Are you sure that |field| is still valid?
It's refcounted and a reference is held by the caller, so it should be.

> One of the new test cases from https://crbug.com/608101#c6 shows this stack:
> https://bugs.chromium.org/p/chromium/issues/attachmentText?aid=233641.
> 
> Although I only reported about setValue, setChecked (a few lines back) should
> also be fixed.
Good point, added.

> Similarly for FormCache::ClearFormWithElement.
While it does hold a reference to a frame without adding to its refcount, I
don't see it using it after setting any values. At that point it is the
responsibility of the caller to avoid calling it again once the frame is
deleted.

[dvadym, 2016-05-04 14:54:34]

LGTM

[vabr (Chromium), 2016-05-04 15:48:04]

Thanks.

Rob, I noticed your http://crbug.com/608101#c9, but I am running out of time
before holiday, so I'll need to look into that later.

In the meantime I don't see an issue in landing this, even if it is only a
partial fix.

Cheers,
Vaclav

[vabr (Chromium), 2016-05-04 15:53:54]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2016-05-04 15:54:13]

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

[commit-bot: I haz the power, 2016-05-04 15:59:01]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-05-04 15:59:02]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-04 16:00:02]

Description was changed from

==========
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
==========

to

==========
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}
==========

[commit-bot: I haz the power, 2016-05-04 16:00:04]

Patchset 4 (id:??) landed as
https://crrev.com/d62bc3e6e2c3be6bbb01fa325e3389f089974017
Cr-Commit-Position: refs/heads/master@{#391524}