Bug 121798 – Consider dropping the render tree while in page cache.

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Andreas Kling]

We can make some decent memory savings by losing the render tree when putting pages into the page cache.
Render tree sizes vary depending on content. For scale, the average Wikipedia article uses around ~1.5 MB.

More importantly, this would allow us to tighten things up by not having to worry about the suspended render tree hanging off of the page cache. We currently have numerous short-circuit hacks to avoid badness when Document::inPageCache() returns true. Most of this could go away.

The tradeoff (cost) is that we now have to rebuild the render tree from scratch if/when the user navigates back to a page in page cache. This should already be a very fast operation, and one that we will continue to optimize.

[Andreas Kling]

Created attachment 212391 [details]
Patch for EWS

[Build Bot]

Comment on attachment 212391 [details]
Patch for EWS

Attachment 212391 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1885408

New failing tests:
compositing/iframes/page-cache-layer-tree.html
fast/loader/scroll-position-restored-on-back.html

[Build Bot]

Created attachment 212396 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 212391 [details]
Patch for EWS

Attachment 212391 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2025077

New failing tests:
compositing/iframes/page-cache-layer-tree.html
fast/loader/scroll-position-restored-on-back.html

[Build Bot]

Created attachment 212413 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 212391 [details]
Patch for EWS

Attachment 212391 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2091076

New failing tests:
compositing/iframes/page-cache-layer-tree.html
fast/loader/scroll-position-restored-on-back.html

[Build Bot]

Created attachment 212416 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Andreas Kling]

Created attachment 218511 [details]
Patch II for EWS

[Build Bot]

Comment on attachment 218511 [details]
Patch II for EWS

Attachment 218511 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/43608043

New failing tests:
compositing/iframes/page-cache-layer-tree.html

[Build Bot]

Created attachment 218514 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 218511 [details]
Patch II for EWS

Attachment 218511 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/40728071

New failing tests:
compositing/iframes/page-cache-layer-tree.html

[Build Bot]

Created attachment 218517 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Andreas Kling]

Created attachment 297406 [details]
Patch III for EWS

Okay, it's been 3 years, let's try this again.

[WebKit Commit Bot]

Attachment 297406 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:10:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 297406 [details]
Patch III for EWS

Attachment 297406 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2744113

New failing tests:
fast/scrolling/iframe-scrollable-after-back.html

[Build Bot]

Created attachment 297408 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Andreas Kling]

Created attachment 297498 [details]
Patch

I think this can actually work.

[Build Bot]

Comment on attachment 297498 [details]
Patch

Attachment 297498 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2756933

New failing tests:
fast/scrolling/iframe-scrollable-after-back.html

[Build Bot]

Created attachment 297501 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Andreas Kling]

Created attachment 297511 [details]
Patch

[Build Bot]

Comment on attachment 297511 [details]
Patch

Attachment 297511 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2757826

New failing tests:
canvas/philip/tests/2d.canvas.reference.html

[Build Bot]

Created attachment 297513 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Andreas Kling]

Created attachment 297832 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 297832 [details]
Patch

Clearing flags on attachment: 297832

Committed r210206: <http://trac.webkit.org/changeset/210206>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 166621

[Andreas Kling]

(In reply to comment #26)
> Re-opened since this is blocked by bug 166621

Rolling out this iteration due to this crash seen on a PLT bot:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x0000000106d05991 WebCore::RenderView::imageQualityController() + 17 (memory:2710)
1   com.apple.WebCore             	0x0000000106b9d06d WebCore::RenderBoxModelObject::willBeDestroyed() + 317 (ImageQualityController.h:50)
2   com.apple.WebCore             	0x0000000106c689b6 WebCore::RenderObject::destroy() + 22 (RenderWidget.h:120)
3   com.apple.WebCore             	0x0000000106c73493 WebCore::RenderScrollbar::setParent(WebCore::ScrollView*) + 83 (HashTable.h:1155)
4   com.apple.WebCore             	0x0000000106d8ae98 WebCore::ScrollView::removeChild(WebCore::Widget&) + 24 (ScrollView.cpp:79)
5   com.apple.WebCore             	0x0000000106d8b019 WebCore::ScrollView::setHasScrollbarInternal(WTF::RefPtr<WebCore::Scrollbar>&, WebCore::ScrollbarOrientation, bool, bool*) + 105 (utility:753)
6   com.apple.WebCore             	0x00000001062a9d8a WebCore::FrameView::detachCustomScrollbars() + 42 (RefPtr.h:64)
7   com.apple.WebCore             	0x0000000106121fdc WebCore::Document::destroyRenderTree() + 172 (Document.cpp:2236)
8   com.apple.WebCore             	0x000000010612d481 WebCore::Document::setPageCacheState(WebCore::Document::PageCacheState) + 65 (Document.cpp:4533)

Looks like a subframe's RenderScrollbar is trying to access its RenderView after said RenderView has been destroyed. Something about destruction order got messed up here.

Meanwhile, this looks like a 2% progression on iOS memory tests, so that part's great :)

[Andreas Kling]

Created attachment 297892 [details]
Patch

The render tree needs to be destroyed in depth-first order, or we run into trouble when tearing down pages with subframes. This was caught by bots.

[Antti Koivisto]

Comment on attachment 297892 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297892&action=review

> Source/WebCore/history/PageCache.cpp:373
> +static void destroyRenderTree(Frame* frame)
> +{
> +    if (!frame)
> +        return;

I would null check in caller.

[Antti Koivisto]

Comment on attachment 297892 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297892&action=review

> Source/WebCore/history/PageCache.cpp:374
> +static void destroyRenderTree(Frame* frame)
> +{
> +    if (!frame)
> +        return;
> +    destroyRenderTree(frame->tree().traverseNext());

Actually it would be nicer to do this without recursion, with a Vector.

[Andreas Kling]

Created attachment 297901 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 297901 [details]
Patch for landing

Clearing flags on attachment: 297901

Committed r210226: <http://trac.webkit.org/changeset/210226>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/30588027>

[Simon Fraser (smfr)]

This change made this code a lie:

inline bool RenderObject::documentBeingDestroyed() const
{
    return document().renderTreeBeingDestroyed();
}