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

Issue 735860 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug



Sign in to add a comment

Print preview is probably leaking WebLocalFrames

Project Member Reported by dcheng@chromium.org, Jun 22 2017

Issue description

https://codereview.chromium.org/2369613003 changed how we cleaned up frames on detach, to avoid a bunch of boilerplate in tests. One of these changes was pushing boilerplate frame cleanup into the base interface.

https://codereview.chromium.org/2874133002 deleted the default implementation but print web view helper did not get the default methods added back.
 

Comment 1 by dcheng@chromium.org, Jun 22 2017

Labels: Performance-Memory
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 23 2017

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

commit ba508351c5ea1cfdc468bb1c4a92ea4bf38c0556
Author: Daniel Cheng <dcheng@chromium.org>
Date: Fri Jun 23 08:49:36 2017

Don't leak WebLocalFrame in print preview.

Unfortunately, Blink currently relies on the embedder to make sure these
objects don't leak.

Bug:  735860 
Change-Id: Ia213d4360c5f95627de70a1dfe343880144f9ff6
Reviewed-on: https://chromium-review.googlesource.com/544367
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481829}
[modify] https://crrev.com/ba508351c5ea1cfdc468bb1c4a92ea4bf38c0556/components/printing/renderer/print_web_view_helper.cc

Comment 3 by dcheng@chromium.org, Jun 23 2017

Labels: -Pri-3 M-60 Merge-Request-60 OS-All Pri-1
I'm not sure how significant the leak is, but it's a simple fix and doesn't hurt.

Comment 4 by dcheng@chromium.org, Jun 23 2017

Status: Fixed (was: Assigned)
Project Member

Comment 5 by sheriffbot@chromium.org, Jun 24 2017

Labels: -Merge-Request-60 Hotlist-Merge-Review Merge-Review-60
This bug requires manual review: M60 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-60 Merge-Approved-60
Approving merge to M60. Regression, simple fix. 
Project Member

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

Cc: abdulsyed@chromium.org
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 8 by sheriffbot@chromium.org, Jul 3 2017

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Please merge this to M60 ASAP. branch:3112
Sorry. I missed the email because I was OOO. Merging now.
Project Member

Comment 11 by bugdroid1@chromium.org, Jul 17 2017

Labels: -merge-approved-60 merge-merged-3112
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/09d8b19232cec8603cc3618f977d91103277e571

commit 09d8b19232cec8603cc3618f977d91103277e571
Author: Daniel Cheng <dcheng@chromium.org>
Date: Mon Jul 17 10:07:34 2017

Don't leak WebLocalFrame in print preview.

Unfortunately, Blink currently relies on the embedder to make sure these
objects don't leak.

TBR=dcheng@chromium.org

(cherry picked from commit ba508351c5ea1cfdc468bb1c4a92ea4bf38c0556)

Bug:  735860 
Change-Id: Ia213d4360c5f95627de70a1dfe343880144f9ff6
Reviewed-on: https://chromium-review.googlesource.com/544367
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#481829}
Reviewed-on: https://chromium-review.googlesource.com/573881
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/branch-heads/3112@{#616}
Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}
[modify] https://crrev.com/09d8b19232cec8603cc3618f977d91103277e571/components/printing/renderer/print_web_view_helper.cc

Cc: krajshree@chromium.org
Labels: Needs-Feedback
dcheng@ - Could you please provide reproducible steps with expected and actual results to verify the issue from TE-end.

Thanks...!!
I don't know that there's good repro steps. I suspect that if you open and close print preview enough times on one page with headers and footers enabled, you'll be able to see memory usage increasing over time (before my change) and hopefully remaining roughly constant afterwards.

Sign in to add a comment