New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 734427 link

Starred by 28 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 2
Type: Bug

Blocked on:
issue 755489
issue 812182

Blocking:
issue 733730



Sign in to add a comment

Focused form controls are not GC'ed

Reported by gaby....@gmail.com, Jun 18 2017

Issue description

UserAgent: Mozilla/5.0 (Windows NT 6.3; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3134.0 Safari/537.36

Steps to reproduce the problem:
1. create element
2. add element to page
3. focus element using element.focus();
4. delete element

What is the expected behavior?
GC should clear the nodes after a while or when i press Collect Garbage in the Dev Tools and memory should be released

What went wrong?
Nodes never get GC'ed and memory increase

Did this work before? No 

Chrome version: 61.0.3134.0  Channel: canary
OS Version: 6.3
Flash Version: 

Attached a demo page with a function that reproduces the problem.
Thanks and good luck.
 
nodes.jpg
277 KB View Download

Comment 1 by gaby....@gmail.com, Jun 18 2017

Wrong html, here is the correct one
focusproblem.html
402 bytes View Download

Comment 2 by gaby....@gmail.com, Jun 18 2017

Same problem with Google Chrome Version 58.0.3029.110 (64-bit)

and

Version 59.0.3071.104 (Official Build) (64-bit)

just open focusproblem.html  and push the button couple of times ...
Labels: Needs-Triage-M61
Labels: Needs-Bisect

Comment 5 by hdodda@chromium.org, Jun 20 2017

Cc: hdodda@chromium.org
Labels: Needs-Feedback
Tested the issue on windows 7 & 10 using chrome latest stable M59 #59.0.3071.104 and using the steps below:

1. Launched chrome and opened given focusproblem.html 
2. Clicked on the button couple of times and observed fps meter.

Attached screencast for reference.

@gaby.lav-- Could you please help us in reproducing the issue with the screencast of the steps , that would be helpful for better understanding.

Thanks!
734427.mp4
875 KB View Download

Comment 6 by gaby....@gmail.com, Jun 20 2017

Hi, here it is, it's a bit long but i wanted to show you both the memory and node count.
I was using Version 61.0.3136.0 (Official Build) canary (64-bit) and as you can see at the end it got to 900k+ nodes and 600k+ memory, it can go to 3GB in about 30-60 minutes
In video 2 you can see there are no Detached DOM tree elements, so i really don't know how i can free that memory...
I'm creating a web app that focuses many elements and this is a problem for me, the app can't be used more than 1-2 hours because of this bug or what ever it is :)
Thanks
Recording #1.mp4
5.8 MB View Download
Recording #2.mp4
1.4 MB View Download
Project Member

Comment 7 by sheriffbot@chromium.org, Jun 20 2017

Labels: -Needs-Feedback
Thank you for providing more feedback. Adding requester "hdodda@chromium.org" to the cc list and removing "Needs-Feedback" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Needs-Bisect -Needs-Triage-M61 M-61 OS-Linux OS-Mac
Status: Untriaged (was: Unconfirmed)
Able to reproduce the issue on Mac 10.12.5, Windows 7 & Ubuntu 14.04 Using chrome stable#59.0.3071.109 & Canary#61.0.3136.0 as per screencast provided in comment#6 & comment#0.

Observed memory is not getting cleared when we press Collect Garbage in the Dev Tools.Memory is getting increased in chrome Task manager when user click on the element in the html file.
This is non regression issue observed from M30 builds, hence marking it as 'Untriaged' to get more inputs from dev.

Please find the attached screen cast fro reference.
Thank you..!!

734427.mp4
1.9 MB View Download

Comment 9 by bokan@chromium.org, Jun 22 2017

Components: -Blink Blink>DOM Blink>Input

Comment 10 by gaby....@gmail.com, Jun 22 2017

Just one more observation, the bug seems to happen only on form elements, input, select, textarea, button... there is no bug on div, span ... elements, somehow the new HTML 5 form elements <datalist>, <keygen>, <output> are working fine... (<legend> too)
Thanks for looking into this

Comment 11 by tkent@chromium.org, Jun 23 2017

Blocking: 733730

Comment 12 by tkent@chromium.org, Jun 23 2017

Cc: keishi@chromium.org
Components: -Blink>Input -Blink>DOM Blink>Focus Blink>Forms
Labels: -M-61
Status: Available (was: Untriaged)
Confirmed.

keishi@, what's a modern way to determine who retains nodes?

Comment 13 by tkent@chromium.org, Jun 23 2017

Summary: Focused form controls are not GC'ed (was: Focus element not GC'ed)
We don't have one.

I took a quick look. The wrapper doesn't seem to be leaking so this is a purely Blink issue.
I think I narrowed the leak down to ShowVirtualKeyboardOnElementFocus in Element::focus().
Components: -Blink>Focus -Blink>Forms UI>Browser>Passwords
Owner: dvadym@chromium.org
Looks like this leak is because PasswordAutofillAgent::field_value_and_properties_map_ is trying to holding on to elements that had focus. I only see it being cleared in FrameClosing().

Comment 17 by gaby....@gmail.com, Jun 23 2017

One more update, it seems that if the form is more complex more nodes are beeing leaked ... see attached file and screenshot, i have no loop now, just a form with 50 inputs, focus one and delete form, every time 150+ nodes are beeing added and none is GC'ed
Thanks for looking into this, hope you can find the bug :)
formtest.jpg
228 KB View Download
focusproblem.html
3.3 KB View Download

Comment 18 by vabr@chromium.org, Jun 27 2017

Labels: Hotlist-Polish
Status: Assigned (was: Available)
I just noticed that leaked nodes are not always removed on page reload, i used to press 
1. clear
2. Start profiling and reload page
3. Collect Garbage

from Developer Tools - Performance tab and observe the initial page JS heap, nodes and listeners (Network - Disable cache is checked), sometimes the leaked nodes are retained and the only way to get rid of them is to close the browser and reopen, for example I am testing a form and 50k+ nodes are leaked, on page reload (even CTRL + F5) i get 49k+ nodes when i should only have about 1k nodes, if i close browser and reopen everything is fine, but as long as don't, the tab have 49k+ nodes even on a blank page. 
I can't reproduce in a simple, HTML page, will try to create a script to reproduce this behavior, but it may need to be run with a nodejs or apache server...
Thanks
Here is a screencast with the node bug on page reload ... as you can see the page must have about 103 nodes and 10 listeners in the new tab or after reload, but the bugged tab is retaining 13576 nodes from before even after Hard Reload ...
Recording #6.mp4
19.1 MB Download
Is this related or do i need to post a new issue?
Thanks
One more observation, the leak is there even if focus() is not used, if you click any form input it causes a leak ... so there is no way to write a leak free app if a form exists in the page :(
I was able to reach 100k+ leaked nodes and 900+ MB memory in about 4 hours of testing a page with only a form on it...
I have attached a screencast and the file to reproduce this bug.
Been able to reproduce this bug with 
Version 59.0.3071.115 (Official Build) (64-bit)
and
Version 61.0.3150.0 (Official Build) canary (64-bit)
on Windows 8.1
Thanks and good luck

Recording #8.mp4
3.8 MB View Download
focusproblem1.html
2.7 KB View Download
Thanks for submitting this bug and investigating reasons.

I've checked code and results are the following:
1.Chrome Password Manager keeps form input elements which were in focus or the user was typing, in order to know user typed value in input elements.
2.It turns out, that any input element has a strong reference on a parent form, that makes things worse, since one element can keep alive whole form with all input elements.

One of the simple mitigation may be removing keeping elements on focus and leaving it only on user typing. 

The possible full solutions:
1.Using MutationObserver to finding when the form is removed from DOM and if it's happen to release all elements.
2.Using weak pointers on input elements, instead of strong ones. Not clear how easy to implement this, since Blink C++ API doesn't expose weak pointers.

I'll investigate more what can be done.
It's strange that's memory is not released on a page reload. I've checked Password Manager code, input elements are released on a page reload.
It is technically possible to implement a weak version of WebNode but I think we should try the other options first.

Regarding memory that is not released on page load, I think it is a separate issue and I am looking into it.

Comment 26 by yuzus@chromium.org, Jul 18 2017

It seems like element_ in AutofillAgent never gets cleared and it is causing the leak, too.

Labels: Performance-Memory
Cc: hajimehoshi@chromium.org
It looks like that AutofillAgent::FocusedNodeChanged updates |element_|, and when a new input form is focused, the document that is held by the previous input element will be deleted. So, I guess AutofillAgent leaks at most one document object.
Project Member

Comment 30 by bugdroid1@chromium.org, Aug 7 2017

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

commit 3b236983f49cd29b183e446f0bad908ac3cab88b
Author: Hajime Hoshi <hajimehoshi@chromium.org>
Date: Mon Aug 07 07:24:17 2017

Fix a leak of a document that is held by AutofillAgent

A document is leaked because AutofillAgent hold a lastly used element
even after navigation happens, and the document of the element remains
until a new (input) element is focused. This CL fixes this issue by
resetting the element in AutofillAgent when navigation happens.

Bug: 734427
Change-Id: Id8a849958ba54fefd4f223d98a79486ab07b5c70
Reviewed-on: https://chromium-review.googlesource.com/597482
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Commit-Queue: Hajime Hoshi <hajimehoshi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492278}
[modify] https://crrev.com/3b236983f49cd29b183e446f0bad908ac3cab88b/components/autofill/content/renderer/autofill_agent.cc

Project Member

Comment 31 by bugdroid1@chromium.org, Aug 14 2017

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

commit 2c01263f421bdf2ce677049317d83c06bdac5c77
Author: Hajime Hoshi <hajimehoshi@chromium.org>
Date: Mon Aug 14 06:36:57 2017

Revert "Fix a leak of a document that is held by AutofillAgent"

This reverts commit 3b236983f49cd29b183e446f0bad908ac3cab88b.

Reason for revert: This causes regression reported at  crbug.com/753071 

Original change's description:
> Fix a leak of a document that is held by AutofillAgent
> 
> A document is leaked because AutofillAgent hold a lastly used element
> even after navigation happens, and the document of the element remains
> until a new (input) element is focused. This CL fixes this issue by
> resetting the element in AutofillAgent when navigation happens.
> 
> Bug: 734427
> Change-Id: Id8a849958ba54fefd4f223d98a79486ab07b5c70
> Reviewed-on: https://chromium-review.googlesource.com/597482
> Reviewed-by: Evan Stade <estade@chromium.org>
> Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
> Commit-Queue: Hajime Hoshi <hajimehoshi@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#492278}

TBR=estade@chromium.org,hajimehoshi@chromium.org,keishi@chromium.org,dvadym@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 734427
Change-Id: I667d8dff26f605c0cc1cb1cb8f9275a55a662dc7
Reviewed-on: https://chromium-review.googlesource.com/611752
Reviewed-by: Hajime Hoshi <hajimehoshi@chromium.org>
Commit-Queue: Hajime Hoshi <hajimehoshi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494018}
[modify] https://crrev.com/2c01263f421bdf2ce677049317d83c06bdac5c77/components/autofill/content/renderer/autofill_agent.cc

Blockedon: 755489
Project Member

Comment 33 by bugdroid1@chromium.org, Aug 17 2017

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

commit 53820e225c939a5a061a5e55a140a8ff5310997e
Author: Hajime Hoshi <hajimehoshi@chromium.org>
Date: Thu Aug 17 02:28:11 2017

Finch testing for document leak fix in AutofillAgent

The document leak in AutofillAgent was fixed once at https://chromium-review.googlesource.com/c/597482,
but reverted because of performance regression. The fix changed the
lifetime of documents and resources.

This CL introduces Finch testing for document leak fix in AutofillAgent
to compare UMAs like TTFMP or memory usages. If TTFMP is not regressed
so much, we can ship the fix again. If not, we might need to consider
to introduce a more explicit resource life-time manager.

Bug:  753071 , 734427, 755489
Change-Id: Ica05258f730a74e7832e171fb4172fc53baac9f2
Reviewed-on: https://chromium-review.googlesource.com/612939
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Maxim Kolosovskiy <kolos@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Commit-Queue: Hajime Hoshi <hajimehoshi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495054}
[modify] https://crrev.com/53820e225c939a5a061a5e55a140a8ff5310997e/components/autofill/content/renderer/autofill_agent.cc

Blockedon: 812182
Project Member

Comment 35 by bugdroid1@chromium.org, Feb 26 2018

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

commit d49a775061c3d506d9cb869d8c087e0779a4d912
Author: Hajime Hoshi <hajimehoshi@chromium.org>
Date: Mon Feb 26 10:41:13 2018

Reland: Fix a leak of a document that is held by AutofillAgent

This CL is just relanding https://chromium-review.googlesource.com/c/chromium/src/+/597482,
after confirming that there are no statistical significant regressions
in the real world by Finch experiments (crbug.com/755489).

Note that this might still cause Telemetry regressions reported at
 crbug.com/753071  before, but this is expected.

A document is leaked because AutofillAgent hold a lastly used element
even after navigation happens, and the document of the element remains
until a new (input) element is focused. This CL fixes this issue by
resetting the element in AutofillAgent when navigation happens.

Bug: 734427, 755489
Change-Id: Ie3ee7f202abf56e22102e3d7f68cf2778f85ca87
Reviewed-on: https://chromium-review.googlesource.com/908371
Commit-Queue: Hajime Hoshi <hajimehoshi@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539105}
[modify] https://crrev.com/d49a775061c3d506d9cb869d8c087e0779a4d912/components/autofill/content/renderer/autofill_agent.cc

Project Member

Comment 36 by bugdroid1@chromium.org, Feb 26 2018

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

commit 6b79cf6fad0d1bf2162513da8ea69184253b4e08
Author: Nektarios Paisios <nektar@chromium.org>
Date: Mon Feb 26 16:49:36 2018

Revert "Reland: Fix a leak of a document that is held by AutofillAgent"

This reverts commit d49a775061c3d506d9cb869d8c087e0779a4d912.

Reason for revert: Very likely broke PasswordManagerBrowserTestBase.PasswordOverridenUpdateBubbleShown in browser_tests 

Original change's description:
> Reland: Fix a leak of a document that is held by AutofillAgent
> 
> This CL is just relanding https://chromium-review.googlesource.com/c/chromium/src/+/597482,
> after confirming that there are no statistical significant regressions
> in the real world by Finch experiments (crbug.com/755489).
> 
> Note that this might still cause Telemetry regressions reported at
>  crbug.com/753071  before, but this is expected.
> 
> A document is leaked because AutofillAgent hold a lastly used element
> even after navigation happens, and the document of the element remains
> until a new (input) element is focused. This CL fixes this issue by
> resetting the element in AutofillAgent when navigation happens.
> 
> Bug: 734427, 755489
> Change-Id: Ie3ee7f202abf56e22102e3d7f68cf2778f85ca87
> Reviewed-on: https://chromium-review.googlesource.com/908371
> Commit-Queue: Hajime Hoshi <hajimehoshi@chromium.org>
> Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Reviewed-by: Evan Stade <estade@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#539105}

TBR=estade@chromium.org,hajimehoshi@chromium.org,haraken@chromium.org,kouhei@chromium.org

Change-Id: Ibe5a7c4d6c35c19bd1ff1140fc0f9afcbb4fea10
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 734427, 755489
Reviewed-on: https://chromium-review.googlesource.com/937762
Reviewed-by: Nektarios Paisios <nektar@chromium.org>
Commit-Queue: Nektarios Paisios <nektar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539163}
[modify] https://crrev.com/6b79cf6fad0d1bf2162513da8ea69184253b4e08/components/autofill/content/renderer/autofill_agent.cc

Labels: Needs-Feedback
Tested the issue on latest chrome version 66.0.3356.0 using Mac 10.12.6 with steps mentioned below:
1) Launched chrome version, dragged and dropped the file provided in comment#1 and opened chrome task manager
2) Opened Devtools > Performance tab and clicked on start
3) Clicked on element in the page for few times and clicked on stop
4) Observed the nodes and GPU values in performance tab

@Nektar:
Please find the attached screen cast for your reference and let us know if we missed anything in verifying the fix, please provide your feedback on it which helps us in verifying the fix.

Thanks!
734427.mp4
2.9 MB View Download
Project Member

Comment 38 by bugdroid1@chromium.org, Mar 14 2018

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

commit 83c3f50b87c1b4e89be296a5d2adae9777bb0805
Author: Hajime Hoshi <hajimehoshi@chromium.org>
Date: Wed Mar 14 05:49:01 2018

Re^2-land: Fix a leak of a document that is held by AutofillAgent

This is a retry of https://chromium-review.googlesource.com/c/chromium/src/+/908371,
with fixing the DCHECK (NOTREACHED) error. We were not sure when the
NOTREACHED is reached before the fix, but now this case becomes
deterministic (see the discussion at crbug.com/816533)

Note that this might still cause Telemetry regressions reported at
 crbug.com/753071  before, but this is expected.

A document is leaked because AutofillAgent hold a lastly used element
even after navigation happens, and the document of the element remains
until a new (input) element is focused. This CL fixes this issue by
resetting the element in AutofillAgent when navigation happens.

Bug: 734427, 755489, 816533
Change-Id: I7eb2f9fb879d826297247c4c66e366ec0b73067c
Reviewed-on: https://chromium-review.googlesource.com/939224
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Hajime Hoshi <hajimehoshi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543015}
[modify] https://crrev.com/83c3f50b87c1b4e89be296a5d2adae9777bb0805/components/autofill/content/renderer/autofill_agent.cc

Tested the issue on chrome version 67.0.3371.0 using Mac 10.12.6 with steps mentioned below:
1) Launched chrome version, dragged and dropped the file provided in comment#1 and opened Performance tab in Devtools
2) Opened chrome task manager from three dot menu > More tools > Taskmanager
3) Clicked record on performance tab and click button on opened html and observed the memory increase in task manager, after multiple clicking on button, clicked on stop on performance and clicked on "Collect garbage" icon, memory value didn't get decreased.
Observations: Tested the issue on chrome reported, observed the same behaviour in that.

@Hajime Hoshi: Please find the attached screen cast for your reference and let us know if we missed anything in reproducing the issue, please helps us in confirming and verifying the fix.

Thanks!
734427.mp4
2.4 MB View Download

Comment 40 by gaby....@gmail.com, Mar 17 2018

Just retested this issue on 
Version 65.0.3325.162 (Official Build) (64-bit)
Windows 10 Home 64 bit

the leak is still there...
There were two bugs that were causing this leak.
The issue with PasswordAutofillAgent::field_value_and_properties_map_ still exists.

This bug is serious for websites that generates many form controls, and it is more complicated as discussed in c#23 so the Autofill team should look into it.
Cc: kolos@chromium.org vasi...@chromium.org
CCing some more folks

Comment 43 by kolos@chromium.org, Mar 19 2018

Another cause is |PasswordGenerationAgent::last_focused_password_element_|
Project Member

Comment 44 by bugdroid1@chromium.org, Mar 20 2018

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

commit cb950738b5abba1e4a971becdf60fe855dfff21b
Author: Alexander Timokhin <atimoxin@yandex-team.ru>
Date: Tue Mar 20 16:43:32 2018

Fixed more Document leaks in Autofill

Some Autofill-related code can hold persistent reference to last
changed or focused <input> element that cause leak of whole Documents
containing such element after navigation to other page.

Some leaks fixed in https://chromium-review.googlesource.com/939224
This CL fixes other related leaks.

Bug: 734427
Change-Id: Ic084d0670cccb9d11595d0bde8cf0d605bfa8911
Reviewed-on: https://chromium-review.googlesource.com/970582
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544404}
[modify] https://crrev.com/cb950738b5abba1e4a971becdf60fe855dfff21b/components/autofill/content/renderer/autofill_agent.cc
[modify] https://crrev.com/cb950738b5abba1e4a971becdf60fe855dfff21b/components/autofill/content/renderer/form_tracker.cc
[modify] https://crrev.com/cb950738b5abba1e4a971becdf60fe855dfff21b/components/autofill/content/renderer/password_generation_agent.cc
[modify] https://crrev.com/cb950738b5abba1e4a971becdf60fe855dfff21b/components/autofill/content/renderer/password_generation_agent.h

Project Member

Comment 45 by bugdroid1@chromium.org, Mar 27 2018

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

commit da22cad86e597d7f2bf2d4854ea832a23bacd022
Author: Evan Stade <estade@chromium.org>
Date: Tue Mar 27 17:18:50 2018

Revert "Fixed more Document leaks in Autofill"

This reverts commit cb950738b5abba1e4a971becdf60fe855dfff21b.

Reason for revert: suspected of performance regression:  crbug.com/825005 

Original change's description:
> Fixed more Document leaks in Autofill
> 
> Some Autofill-related code can hold persistent reference to last
> changed or focused <input> element that cause leak of whole Documents
> containing such element after navigation to other page.
> 
> Some leaks fixed in https://chromium-review.googlesource.com/939224
> This CL fixes other related leaks.
> 
> Bug: 734427
> Change-Id: Ic084d0670cccb9d11595d0bde8cf0d605bfa8911
> Reviewed-on: https://chromium-review.googlesource.com/970582
> Reviewed-by: Evan Stade <estade@chromium.org>
> Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
> Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#544404}

TBR=estade@chromium.org,dvadym@chromium.org,atimoxin@yandex-team.ru

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 734427
Change-Id: I57bd57e50af5d8e4f4d97b731a354fd7b0957966
Reviewed-on: https://chromium-review.googlesource.com/981292
Reviewed-by: Evan Stade <estade@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546146}
[modify] https://crrev.com/da22cad86e597d7f2bf2d4854ea832a23bacd022/components/autofill/content/renderer/autofill_agent.cc
[modify] https://crrev.com/da22cad86e597d7f2bf2d4854ea832a23bacd022/components/autofill/content/renderer/form_tracker.cc
[modify] https://crrev.com/da22cad86e597d7f2bf2d4854ea832a23bacd022/components/autofill/content/renderer/password_generation_agent.cc
[modify] https://crrev.com/da22cad86e597d7f2bf2d4854ea832a23bacd022/components/autofill/content/renderer/password_generation_agent.h

Project Member

Comment 46 by bugdroid1@chromium.org, Mar 28 2018

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

commit 3c45d1a6de13288d9d11afea2d2d169f12c8574a
Author: Alexander Timokhin <atimoxin@yandex-team.ru>
Date: Wed Mar 28 17:49:28 2018

Reland "Fixed more Document leaks in Autofill"

This is a reland of cb950738b5abba1e4a971becdf60fe855dfff21b

Original change's description:
> Fixed more Document leaks in Autofill
>
> Some Autofill-related code can hold persistent reference to last
> changed or focused <input> element that cause leak of whole Documents
> containing such element after navigation to other page.
>
> Some leaks fixed in https://chromium-review.googlesource.com/939224
> This CL fixes other related leaks.
>
> Bug: 734427
> Change-Id: Ic084d0670cccb9d11595d0bde8cf0d605bfa8911
> Reviewed-on: https://chromium-review.googlesource.com/970582
> Reviewed-by: Evan Stade <estade@chromium.org>
> Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
> Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#544404}

TBR=dvadym@chromium.org

Bug: 734427
Change-Id: Iceff496bdeb16cc1ea5ffc8208d8b8d1a600857b
Reviewed-on: https://chromium-review.googlesource.com/984472
Reviewed-by: Evan Stade <estade@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546540}
[modify] https://crrev.com/3c45d1a6de13288d9d11afea2d2d169f12c8574a/components/autofill/content/renderer/autofill_agent.cc
[modify] https://crrev.com/3c45d1a6de13288d9d11afea2d2d169f12c8574a/components/autofill/content/renderer/form_tracker.cc
[modify] https://crrev.com/3c45d1a6de13288d9d11afea2d2d169f12c8574a/components/autofill/content/renderer/password_generation_agent.cc
[modify] https://crrev.com/3c45d1a6de13288d9d11afea2d2d169f12c8574a/components/autofill/content/renderer/password_generation_agent.h

Cc: dvadym@chromium.org
Owner: ----
Status: Available (was: Assigned)
Cc: alph@chromium.org sindhu.chelamcherla@chromium.org
 Issue 834673  has been merged into this issue.
Project Member

Comment 49 by bugdroid1@chromium.org, Jun 8 2018

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

commit afc4c45a5d903e2dedc5edb4262350cd511b05c3
Author: Vadym Doroshenko <dvadym@chromium.org>
Date: Fri Jun 08 13:58:56 2018

Using unique WebInput ids instead of WebInput elements.

Password Manager keeps the map from WebFormControlElement to last
non-JavaScript value in FieldValueAndPropertiesMaskMap. That's bad from
memory consumption perspective since it keeps input elements in memory
even if they were removed from DOM (and WebFormControlElement keep
s whole WebForm). Now we have reliable mechanism - unique renderer ids,
which might be used instead of WebFormControlElement. This CL replaces
usages of WebFormControlElement with unique renderer ids.

Also this CL contains small clean-up in password_autofill_agent.h:
removing not used typedef and typedef to using conversion.

Bug: 831123, 734427
Change-Id: I12a7b53859fccb9c5398997c61250ccf1a4f5c89
Reviewed-on: https://chromium-review.googlesource.com/1085460
Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565623}
[modify] https://crrev.com/afc4c45a5d903e2dedc5edb4262350cd511b05c3/components/autofill/content/renderer/form_autofill_util.cc
[modify] https://crrev.com/afc4c45a5d903e2dedc5edb4262350cd511b05c3/components/autofill/content/renderer/password_autofill_agent.cc
[modify] https://crrev.com/afc4c45a5d903e2dedc5edb4262350cd511b05c3/components/autofill/content/renderer/password_autofill_agent.h
[modify] https://crrev.com/afc4c45a5d903e2dedc5edb4262350cd511b05c3/components/autofill/content/renderer/password_form_conversion_utils.h
[modify] https://crrev.com/afc4c45a5d903e2dedc5edb4262350cd511b05c3/components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc

We have recently discovered that this issue is seriously impacting our single page application.  Our users often keep an instance of the application open all day, and after a couple of hours the GC passes are taking 100s of ms and eventually the browser tab will crash.  

We have confirmed that the issue is fixed in Canary (69.0.3473.0) on windows.  I was wondering if you expect this change to be included as M69 moves to dev/beta?

Thanks
jeremy_b...@  The fix from the comment #49 will be in M-69 dev/beta/stable. If this fix is what that fixed the issue on your page, then yes.

Sign in to add a comment