New issue
Advanced search Search tips

Issue 693178 link

Starred by 1 user
Status: Fixed
Owner:
olivierrobin@chromium.org
Closed: Feb 2017
Cc:
mard...@chromium.org
marq@chromium.org
stkhapugin@chromium.org
vbarig...@chromium.org
eugene...@chromium.org
pkl@chromium.org
ios-bugs@chromium.org
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Bug



Sign in to add a comment

Error pages are leaked

Project Member Reported by olivierrobin@chromium.org, Feb 16 2017 
There is a retain cycle between
ErrorPageContent<HTMLGenerator> <--> StaticHtmlViewController

https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/static_content/static_html_native_content.mm?q=statichtmlna&l=28
https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/static_content/static_html_view_controller.mm?l=70

This cause all the Error pages to be leaked, keeping alive the WKWebView.

This bug seems to have been in the code for a long time (at least since branch 2403).
The consequences will likely increase a lot with M57 and the release of Offline pages as people will more likely navigate in bad network conditions, and every time an offline page is displayed, an error page is created (and leaked).

Consequences are an increasing number of WKWebView in Chrome process, likely leading of memory consumption in Webkit process and to sad tabs.
crbug.com/692622 is also likely caused by this leak.

Many thanks to stk for having debugged this.

Comment 1 by cma...@chromium.org, Feb 17 2017 

This is amazing! Thanks for catching this!
Project Member

Comment 2 by bugdroid1@chromium.org, Feb 20 2017 

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

commit fc155c2704ad95817d98efb4c3dbec239c4c17cb
Author: olivierrobin <olivierrobin@chromium.org>
Date: Mon Feb 20 12:59:19 2017

Error pages: Break retain cycle NativeContent/StaticHTMLViewController

StaticHTMLViewController retains the generator.
The generator is implementated by ErrorPageContent, which retain the
StaticHTMLViewController.
This leads to a leak and the WKWebView is never released.

BUG= 693178 

Review-Url: https://codereview.chromium.org/2695593011
Cr-Commit-Position: refs/heads/master@{#451609}

[modify] https://crrev.com/fc155c2704ad95817d98efb4c3dbec239c4c17cb/ios/chrome/browser/ui/static_content/static_html_view_controller.h
[modify] https://crrev.com/fc155c2704ad95817d98efb4c3dbec239c4c17cb/ios/chrome/browser/web/BUILD.gn
[modify] https://crrev.com/fc155c2704ad95817d98efb4c3dbec239c4c17cb/ios/chrome/browser/web/DEPS
[modify] https://crrev.com/fc155c2704ad95817d98efb4c3dbec239c4c17cb/ios/chrome/browser/web/error_page_content.h
[modify] https://crrev.com/fc155c2704ad95817d98efb4c3dbec239c4c17cb/ios/chrome/browser/web/error_page_content.mm
[delete] https://crrev.com/5509091b50695c6a3a9a1967d3e2bc8df615863e/ios/chrome/browser/web/error_page_content_unittest.mm
[add] https://crrev.com/fc155c2704ad95817d98efb4c3dbec239c4c17cb/ios/chrome/browser/web/error_page_generator.h
[add] https://crrev.com/fc155c2704ad95817d98efb4c3dbec239c4c17cb/ios/chrome/browser/web/error_page_generator.mm
[add] https://crrev.com/fc155c2704ad95817d98efb4c3dbec239c4c17cb/ios/chrome/browser/web/error_page_generator_unittest.mm

Comment 3 by olivierrobin@chromium.org, Feb 20 2017

Labels: Merge-Request-57

Comment 4 by olivierrobin@chromium.org, Feb 20 2017

Status: Fixed (was: Started)
Project Member

Comment 5 by sheriffbot@chromium.org, Feb 20 2017

Labels: -Merge-Request-57 Hotlist-Merge-Review Merge-Review-57
This bug requires manual review: Less than 18 days to go before AppStore submit on M57
Please contact the milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

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

Comment 6 by cma...@chromium.org, Feb 21 2017

Labels: -Hotlist-Merge-Review -Merge-Review-57 Merge-Approved-57
Project Member

Comment 7 by bugdroid1@chromium.org, Feb 22 2017

Labels: -merge-approved-57 merge-merged-2987
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b8d01bfd581d8285158819ab99ebbbf59afa6704

commit b8d01bfd581d8285158819ab99ebbbf59afa6704
Author: Olivier Robin <olivierrobin@chromium.org>
Date: Wed Feb 22 08:36:08 2017

Error pages: Break retain cycle NativeContent/StaticHTMLViewController

StaticHTMLViewController retains the generator.
The generator is implementated by ErrorPageContent, which retain the
StaticHTMLViewController.
This leads to a leak and the WKWebView is never released.

BUG= 693178 

Review-Url: https://codereview.chromium.org/2695593011
Cr-Commit-Position: refs/heads/master@{#451609}
(cherry picked from commit fc155c2704ad95817d98efb4c3dbec239c4c17cb)

Review-Url: https://codereview.chromium.org/2704383003 .
Cr-Commit-Position: refs/branch-heads/2987@{#635}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/b8d01bfd581d8285158819ab99ebbbf59afa6704/ios/chrome/browser/ui/static_content/static_html_view_controller.h
[modify] https://crrev.com/b8d01bfd581d8285158819ab99ebbbf59afa6704/ios/chrome/browser/web/BUILD.gn
[modify] https://crrev.com/b8d01bfd581d8285158819ab99ebbbf59afa6704/ios/chrome/browser/web/DEPS
[modify] https://crrev.com/b8d01bfd581d8285158819ab99ebbbf59afa6704/ios/chrome/browser/web/error_page_content.h
[modify] https://crrev.com/b8d01bfd581d8285158819ab99ebbbf59afa6704/ios/chrome/browser/web/error_page_content.mm
[delete] https://crrev.com/5070d7ed4439285fb38cd460de268ed60c467f52/ios/chrome/browser/web/error_page_content_unittest.mm
[add] https://crrev.com/b8d01bfd581d8285158819ab99ebbbf59afa6704/ios/chrome/browser/web/error_page_generator.h
[add] https://crrev.com/b8d01bfd581d8285158819ab99ebbbf59afa6704/ios/chrome/browser/web/error_page_generator.mm
[add] https://crrev.com/b8d01bfd581d8285158819ab99ebbbf59afa6704/ios/chrome/browser/web/error_page_generator_unittest.mm

Comment 8 by vbarig...@chromium.org, Feb 28 2017 

Hi Olivier, Can you please provide test steps to verify this issue if any. thx

Comment 9 by vbarig...@chromium.org, Feb 28 2017

Cc: vbarig...@chromium.org
►

Sign in to add a comment