Document leak on pages with text input forms such as google.com #13764

cdumez · 2023-05-11T19:13:54Z

`b9a32bf`

Document leak on pages with text input forms such as google.com
https://bugs.webkit.org/show_bug.cgi?id=256404
rdar://108975202

Reviewed by Wenson Hsieh and Ryosuke Niwa.

When typing test in a text input field and then navigating away, the
text field's document would leak. It would be kept alive via a
WebUndoStep stored in the WebPage::m_undoStepMap map.

FrameLoader::closeURL() was calling Editor::clearUndoRedoOperations()
to clear those WebUndoSteps on the WebPage. However, it ended up being
a no-op because Editor::client() would return null because the document
was already detached from the frame and the EditorClient is stored on
the Page. This happens in particular when the previous page was put in
the back/forward cache.

To address the issue, I updated Editor to store a WeakPtr to the
EditorClient object so that it is always able to tell the client to
clear operations if the Page/EditorClient are still alive.

* Source/WebCore/editing/Editor.cpp:
(WebCore::Editor::client const):
(WebCore::Editor::Editor):
* Source/WebCore/editing/Editor.h:

Canonical link: https://commits.webkit.org/264022@main

54b53fa

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch
✅ 🛠 🧪 unsafe-merge	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-05-11T19:14:07Z

EWS run on previous version of this PR (hash aff8850)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	~~🧪 mac-wk1~~	~~🧪 gtk-wk2~~
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

cdumez · 2023-05-11T19:14:29Z

I will try to write a layout test.

webkit-early-warning-system · 2023-05-11T20:40:03Z

EWS run on previous version of this PR (hash 36a95c9)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch
	✅ 🛠 watch-sim

cdumez · 2023-05-11T20:40:26Z

Added a layout test.

rreno · 2023-05-11T20:49:13Z

Should we also clear the state kept in the UIProcess? WebPageProxy::registerEditCommand adds a command proxy object for the undo steps we'll be clearing in the Web process. I didn't see the code path for clearing commands get hit in my manual tests.

cdumez · 2023-05-11T20:51:33Z

Should we also clear the state kept in the UIProcess? WebPageProxy::registerEditCommand adds a command proxy object for the undo steps we'll be clearing in the Web process. I didn't see the code path for clearing commands get hit in my manual tests.

I'll check but Editor::clearUndoRedoOperations() actually ends up sending an IPC to the WebPageProxy in the UIProcess and then UIProcess then sends an IPC back to the WebProcess asking the WebPage to clear the undoSteps. I would assume the WebPageProxy clears them too given that we told it too.

cdumez · 2023-05-11T20:59:35Z

Should we also clear the state kept in the UIProcess? WebPageProxy::registerEditCommand adds a command proxy object for the undo steps we'll be clearing in the Web process. I didn't see the code path for clearing commands get hit in my manual tests.

I'll check but Editor::clearUndoRedoOperations() actually ends up sending an IPC to the WebPageProxy in the UIProcess and then UIProcess then sends an IPC back to the WebProcess asking the WebPage to clear the undoSteps. I would assume the WebPageProxy clears them too given that we told it too.

With my patch, I see the WebEditCommandProxy destructor getting called in the UIProcess.

webkit-early-warning-system · 2023-05-11T23:48:53Z

EWS run on previous version of this PR (hash 62f95d5)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	❌ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-05-12T05:09:18Z

EWS run on previous version of this PR (hash be62b9c)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch
	✅ 🛠 watch-sim

cdumez · 2023-05-12T14:37:09Z

Re-requesting review as I had to make a few changes to make all tests pass.

rreno · 2023-05-12T16:34:49Z

Classic. Reverse member order destruction.
LGTM but you'll need an official r+ since I'm not a reviewer.

cdumez · 2023-05-12T16:43:13Z

Classic. Reverse member order destruction. LGTM but you'll need an official r+ since I'm not a reviewer.

It is actually about construction. We need to make sure the EditorClient gets constructed before the main frame and thus its document. This is because the Document constructor will construct the "Editor" which now grabs the EditorClient from the Page.

rreno · 2023-05-12T16:46:30Z

Classic. Reverse member order destruction. LGTM but you'll need an official r+ since I'm not a reviewer.

It is actually about construction. We need to make sure the EditorClient gets constructed before the main frame and thus its document. This is because the Document constructor will construct the "Editor" which now grabs the EditorClient from the Page.

Ah I was looking at the crashes which looked like they asserted on the EditorClient having already been destroyed before we got to the Frame destructor.

whsieh

LGTM with a question.

Source/WebCore/editing/Editor.cpp

webkit-early-warning-system · 2023-05-12T17:31:28Z

EWS run on current version of this PR (hash 54b53fa)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch
✅ 🛠 🧪 unsafe-merge	✅ 🛠 watch-sim

cdumez · 2023-05-12T17:34:58Z

Source/WebCore/editing/Editor.cpp

-    if (Page* page = m_document.page())
-        return &page->editorClient();
-    return nullptr;
+    ASSERT(!m_client || !m_document.page() || m_client == &m_document.page()->editorClient());


@whsieh I added the assertion here.

Makes sense — thanks!

https://bugs.webkit.org/show_bug.cgi?id=256404 rdar://108975202 Reviewed by Wenson Hsieh and Ryosuke Niwa. When typing test in a text input field and then navigating away, the text field's document would leak. It would be kept alive via a WebUndoStep stored in the WebPage::m_undoStepMap map. FrameLoader::closeURL() was calling Editor::clearUndoRedoOperations() to clear those WebUndoSteps on the WebPage. However, it ended up being a no-op because Editor::client() would return null because the document was already detached from the frame and the EditorClient is stored on the Page. This happens in particular when the previous page was put in the back/forward cache. To address the issue, I updated Editor to store a WeakPtr to the EditorClient object so that it is always able to tell the client to clear operations if the Page/EditorClient are still alive. * Source/WebCore/editing/Editor.cpp: (WebCore::Editor::client const): (WebCore::Editor::Editor): * Source/WebCore/editing/Editor.h: Canonical link: https://commits.webkit.org/264022@main

webkit-commit-queue · 2023-05-12T20:25:08Z

Committed 264022@main (b9a32bf): https://commits.webkit.org/264022@main

Reviewed commits have been landed. Closing PR #13764 and removing active labels.

cdumez self-assigned this May 11, 2023

cdumez added the Forms For bugs specific to form elements (checkboxes, buttons, text fields, etc.) label May 11, 2023

cdumez requested review from whsieh and rreno May 11, 2023 19:14

rniwa approved these changes May 11, 2023

View reviewed changes

cdumez force-pushed the 256404_UndoSteps_doc_leak branch from aff8850 to 36a95c9 Compare May 11, 2023 20:39

cdumez marked this pull request as ready for review May 11, 2023 21:50

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 11, 2023

cdumez removed the merging-blocked Applied to prevent a change from being merged label May 11, 2023

cdumez force-pushed the 256404_UndoSteps_doc_leak branch from 36a95c9 to 62f95d5 Compare May 11, 2023 23:48

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 12, 2023

cdumez removed the merging-blocked Applied to prevent a change from being merged label May 12, 2023

cdumez force-pushed the 256404_UndoSteps_doc_leak branch from 62f95d5 to be62b9c Compare May 12, 2023 05:09

cdumez requested a review from rniwa May 12, 2023 14:36

whsieh approved these changes May 12, 2023

View reviewed changes

whsieh reviewed May 12, 2023

View reviewed changes

Source/WebCore/editing/Editor.cpp Show resolved Hide resolved

cdumez force-pushed the 256404_UndoSteps_doc_leak branch from be62b9c to 54b53fa Compare May 12, 2023 17:31

cdumez commented May 12, 2023

View reviewed changes

rniwa approved these changes May 12, 2023

View reviewed changes

cdumez added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label May 12, 2023

webkit-commit-queue force-pushed the 256404_UndoSteps_doc_leak branch from 54b53fa to b9a32bf Compare May 12, 2023 20:25

webkit-commit-queue merged commit b9a32bf into WebKit:main May 12, 2023

webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label May 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document leak on pages with text input forms such as google.com #13764

Document leak on pages with text input forms such as google.com #13764

cdumez commented May 11, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented May 11, 2023 •

edited

cdumez commented May 11, 2023 •

edited

webkit-early-warning-system commented May 11, 2023 •

edited

cdumez commented May 11, 2023

rreno commented May 11, 2023

cdumez commented May 11, 2023

cdumez commented May 11, 2023

webkit-early-warning-system commented May 11, 2023 •

edited

webkit-early-warning-system commented May 12, 2023 •

edited

cdumez commented May 12, 2023

rreno commented May 12, 2023

cdumez commented May 12, 2023

rreno commented May 12, 2023

whsieh left a comment

webkit-early-warning-system commented May 12, 2023 •

edited

cdumez May 12, 2023

whsieh May 12, 2023

webkit-commit-queue commented May 12, 2023

Document leak on pages with text input forms such as google.com #13764

Document leak on pages with text input forms such as google.com #13764

Conversation

cdumez commented May 11, 2023 • edited by webkit-early-warning-system

b9a32bf

webkit-early-warning-system commented May 11, 2023 • edited

cdumez commented May 11, 2023 • edited

webkit-early-warning-system commented May 11, 2023 • edited

cdumez commented May 11, 2023

rreno commented May 11, 2023

cdumez commented May 11, 2023

cdumez commented May 11, 2023

webkit-early-warning-system commented May 11, 2023 • edited

webkit-early-warning-system commented May 12, 2023 • edited

cdumez commented May 12, 2023

rreno commented May 12, 2023

cdumez commented May 12, 2023

rreno commented May 12, 2023

whsieh left a comment

Choose a reason for hiding this comment

webkit-early-warning-system commented May 12, 2023 • edited

cdumez May 12, 2023

Choose a reason for hiding this comment

whsieh May 12, 2023

Choose a reason for hiding this comment

webkit-commit-queue commented May 12, 2023

cdumez commented May 11, 2023 •

edited by webkit-early-warning-system

`b9a32bf`

webkit-early-warning-system commented May 11, 2023 •

edited

cdumez commented May 11, 2023 •

edited

webkit-early-warning-system commented May 11, 2023 •

edited

webkit-early-warning-system commented May 11, 2023 •

edited

webkit-early-warning-system commented May 12, 2023 •

edited

webkit-early-warning-system commented May 12, 2023 •

edited