- Revision
- 265693
- Author
- [email protected]
- Date
- 2020-08-14 12:48:29 -0700 (Fri, 14 Aug 2020)
Log Message
Cherry-pick r265420. rdar://problem/67083903
REGRESSION (r260831): Web process crashes under Editor::setComposition() after navigating with marked text
https://bugs.webkit.org/show_bug.cgi?id=215315
<rdar://problem/64740092>
Reviewed by Darin Adler.
Source/WebCore:
To address a variety of crashes due to frames changing (or otherwise losing) their document while executing
editing commands, r260831 refactored the Editor class such that it extends the functionality of the Document
class, rather than the Frame class. In nearly all scenarios, this either leads to no behavior change or prevents
null pointer crashes, since a document is almost always attached to a frame when applying any editing commands.
However, there is one scenario where a document that has not yet been attached to its frame (and therefore does
not have a browsing context) will cause a null deref when trying to confirm an existing IME composition. The
logic added in <https://trac.webkit.org/r150291> will try and confirm any existing composition range on a
document right before committing provisional navigation. In the case where we are navigating back to a
previously visited page, `m_frame`'s document in `FrameLoader::commitProvisionalLoad()` will not be attached
until the cached page's mainframe is opened underneath `CachedPage::restore()`. Since the call to
`Editor::confirmComposition()` currently happens before this step, we end up crashing while attempting to create
a `UserTypingGestureIndicator`. Note that even if we avoid this with a null check, we'll still end up crashing
shortly thereafter, underneath `Editor::insertTextForConfirmedComposition`. And even if this second crash is
avoided with another null check, we'll just end up with some version of webkit.org/b/59121, where the
composition range is present after navigation, but is out of sync with platform UI.
To fix the crash (and also not bring back bug #59121), we refactor this composition confirmation logic so that
it lives in Editor, and is also robust against the case where the document is not attached to a frame; we then
invoke this call after we're done committing the provisional load, so that any frame that is not yet attached
before commiting the load still has a chance to confirm its composition.
Test: WKWebViewMacEditingTests.ProcessSwapAfterSettingMarkedText
* editing/Editor.cpp:
(WebCore::Editor::confirmCompositionAndNotifyClient):
Move functionality from `willTransitionToCommitted` to `confirmCompositionAndNotifyClient`, a helper method that
will bail if the document is not attached, but otherwise confirm the active composition (if it exists).
* editing/Editor.h:
* loader/FrameLoader.cpp:
(WebCore::FrameLoader::commitProvisionalLoad):
Add a call to confirm the editor's current composition after we're done committing the load. Note that in the
case where we had a composition before committing the load, we'll end up confirming the composition earlier (in
the first call site), rather than confirming after the load has been committed. This means that this second call
will be a no-op, due to the editor not having any composition.
(WebCore::FrameLoader::willTransitionToCommitted): Deleted.
* loader/FrameLoader.h:
Tools:
Add a new API that exercises the crash by:
- Enabling PSON.
- Navigating to page A and inserting some marked text.
- Navigating to page B with a process swap (without confirming the marked text).
- Navigating back to page A, and verifying that the previoulsy marked text is now committed.
* TestWebKitAPI/Tests/mac/WKWebViewMacEditingTests.mm:
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@265420 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Modified Paths
Diff
Modified: branches/safari-610.1-branch/Source/WebCore/ChangeLog (265692 => 265693)
--- branches/safari-610.1-branch/Source/WebCore/ChangeLog 2020-08-14 19:13:21 UTC (rev 265692)
+++ branches/safari-610.1-branch/Source/WebCore/ChangeLog 2020-08-14 19:48:29 UTC (rev 265693)
@@ -1,3 +1,120 @@
+2020-08-14 Alan Coon <[email protected]>
+
+ Cherry-pick r265420. rdar://problem/67083903
+
+ REGRESSION (r260831): Web process crashes under Editor::setComposition() after navigating with marked text
+ https://bugs.webkit.org/show_bug.cgi?id=215315
+ <rdar://problem/64740092>
+
+ Reviewed by Darin Adler.
+
+ Source/WebCore:
+
+ To address a variety of crashes due to frames changing (or otherwise losing) their document while executing
+ editing commands, r260831 refactored the Editor class such that it extends the functionality of the Document
+ class, rather than the Frame class. In nearly all scenarios, this either leads to no behavior change or prevents
+ null pointer crashes, since a document is almost always attached to a frame when applying any editing commands.
+
+ However, there is one scenario where a document that has not yet been attached to its frame (and therefore does
+ not have a browsing context) will cause a null deref when trying to confirm an existing IME composition. The
+ logic added in <https://trac.webkit.org/r150291> will try and confirm any existing composition range on a
+ document right before committing provisional navigation. In the case where we are navigating back to a
+ previously visited page, `m_frame`'s document in `FrameLoader::commitProvisionalLoad()` will not be attached
+ until the cached page's mainframe is opened underneath `CachedPage::restore()`. Since the call to
+ `Editor::confirmComposition()` currently happens before this step, we end up crashing while attempting to create
+ a `UserTypingGestureIndicator`. Note that even if we avoid this with a null check, we'll still end up crashing
+ shortly thereafter, underneath `Editor::insertTextForConfirmedComposition`. And even if this second crash is
+ avoided with another null check, we'll just end up with some version of webkit.org/b/59121, where the
+ composition range is present after navigation, but is out of sync with platform UI.
+
+ To fix the crash (and also not bring back bug #59121), we refactor this composition confirmation logic so that
+ it lives in Editor, and is also robust against the case where the document is not attached to a frame; we then
+ invoke this call after we're done committing the provisional load, so that any frame that is not yet attached
+ before commiting the load still has a chance to confirm its composition.
+
+ Test: WKWebViewMacEditingTests.ProcessSwapAfterSettingMarkedText
+
+ * editing/Editor.cpp:
+ (WebCore::Editor::confirmCompositionAndNotifyClient):
+
+ Move functionality from `willTransitionToCommitted` to `confirmCompositionAndNotifyClient`, a helper method that
+ will bail if the document is not attached, but otherwise confirm the active composition (if it exists).
+
+ * editing/Editor.h:
+ * loader/FrameLoader.cpp:
+ (WebCore::FrameLoader::commitProvisionalLoad):
+
+ Add a call to confirm the editor's current composition after we're done committing the load. Note that in the
+ case where we had a composition before committing the load, we'll end up confirming the composition earlier (in
+ the first call site), rather than confirming after the load has been committed. This means that this second call
+ will be a no-op, due to the editor not having any composition.
+
+ (WebCore::FrameLoader::willTransitionToCommitted): Deleted.
+ * loader/FrameLoader.h:
+
+ Tools:
+
+ Add a new API that exercises the crash by:
+ - Enabling PSON.
+ - Navigating to page A and inserting some marked text.
+ - Navigating to page B with a process swap (without confirming the marked text).
+ - Navigating back to page A, and verifying that the previoulsy marked text is now committed.
+
+ * TestWebKitAPI/Tests/mac/WKWebViewMacEditingTests.mm:
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@265420 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2020-08-09 Wenson Hsieh <[email protected]>
+
+ REGRESSION (r260831): Web process crashes under Editor::setComposition() after navigating with marked text
+ https://bugs.webkit.org/show_bug.cgi?id=215315
+ <rdar://problem/64740092>
+
+ Reviewed by Darin Adler.
+
+ To address a variety of crashes due to frames changing (or otherwise losing) their document while executing
+ editing commands, r260831 refactored the Editor class such that it extends the functionality of the Document
+ class, rather than the Frame class. In nearly all scenarios, this either leads to no behavior change or prevents
+ null pointer crashes, since a document is almost always attached to a frame when applying any editing commands.
+
+ However, there is one scenario where a document that has not yet been attached to its frame (and therefore does
+ not have a browsing context) will cause a null deref when trying to confirm an existing IME composition. The
+ logic added in <https://trac.webkit.org/r150291> will try and confirm any existing composition range on a
+ document right before committing provisional navigation. In the case where we are navigating back to a
+ previously visited page, `m_frame`'s document in `FrameLoader::commitProvisionalLoad()` will not be attached
+ until the cached page's mainframe is opened underneath `CachedPage::restore()`. Since the call to
+ `Editor::confirmComposition()` currently happens before this step, we end up crashing while attempting to create
+ a `UserTypingGestureIndicator`. Note that even if we avoid this with a null check, we'll still end up crashing
+ shortly thereafter, underneath `Editor::insertTextForConfirmedComposition`. And even if this second crash is
+ avoided with another null check, we'll just end up with some version of webkit.org/b/59121, where the
+ composition range is present after navigation, but is out of sync with platform UI.
+
+ To fix the crash (and also not bring back bug #59121), we refactor this composition confirmation logic so that
+ it lives in Editor, and is also robust against the case where the document is not attached to a frame; we then
+ invoke this call after we're done committing the provisional load, so that any frame that is not yet attached
+ before commiting the load still has a chance to confirm its composition.
+
+ Test: WKWebViewMacEditingTests.ProcessSwapAfterSettingMarkedText
+
+ * editing/Editor.cpp:
+ (WebCore::Editor::confirmCompositionAndNotifyClient):
+
+ Move functionality from `willTransitionToCommitted` to `confirmCompositionAndNotifyClient`, a helper method that
+ will bail if the document is not attached, but otherwise confirm the active composition (if it exists).
+
+ * editing/Editor.h:
+ * loader/FrameLoader.cpp:
+ (WebCore::FrameLoader::commitProvisionalLoad):
+
+ Add a call to confirm the editor's current composition after we're done committing the load. Note that in the
+ case where we had a composition before committing the load, we'll end up confirming the composition earlier (in
+ the first call site), rather than confirming after the load has been committed. This means that this second call
+ will be a no-op, due to the editor not having any composition.
+
+ (WebCore::FrameLoader::willTransitionToCommitted): Deleted.
+ * loader/FrameLoader.h:
+
2020-08-13 Russell Epstein <[email protected]>
Cherry-pick r265280. rdar://problem/66945503
Modified: branches/safari-610.1-branch/Source/WebCore/editing/Editor.cpp (265692 => 265693)
--- branches/safari-610.1-branch/Source/WebCore/editing/Editor.cpp 2020-08-14 19:13:21 UTC (rev 265692)
+++ branches/safari-610.1-branch/Source/WebCore/editing/Editor.cpp 2020-08-14 19:48:29 UTC (rev 265693)
@@ -1939,6 +1939,23 @@
setComposition(m_compositionNode->data().substring(m_compositionStart, m_compositionEnd - m_compositionStart), ConfirmComposition);
}
+void Editor::confirmCompositionAndNotifyClient()
+{
+ if (!hasComposition())
+ return;
+
+ auto frame = makeRefPtr(m_document.frame());
+ if (!frame)
+ return;
+
+ confirmComposition();
+
+ if (auto editorClient = client()) {
+ editorClient->respondToChangedSelection(frame.get());
+ editorClient->discardedComposition(frame.get());
+ }
+}
+
void Editor::cancelComposition()
{
if (!m_compositionNode)
Modified: branches/safari-610.1-branch/Source/WebCore/editing/Editor.h (265692 => 265693)
--- branches/safari-610.1-branch/Source/WebCore/editing/Editor.h 2020-08-14 19:13:21 UTC (rev 265692)
+++ branches/safari-610.1-branch/Source/WebCore/editing/Editor.h 2020-08-14 19:48:29 UTC (rev 265693)
@@ -379,6 +379,7 @@
WEBCORE_EXPORT void setComposition(const String&, const Vector<CompositionUnderline>&, const Vector<CompositionHighlight>&, unsigned selectionStart, unsigned selectionEnd);
WEBCORE_EXPORT void confirmComposition();
WEBCORE_EXPORT void confirmComposition(const String&); // if no existing composition, replaces selection
+ void confirmCompositionAndNotifyClient();
WEBCORE_EXPORT void cancelComposition();
bool cancelCompositionIfSelectionIsInvalid();
WEBCORE_EXPORT Optional<SimpleRange> compositionRange() const;
Modified: branches/safari-610.1-branch/Source/WebCore/loader/FrameLoader.cpp (265692 => 265693)
--- branches/safari-610.1-branch/Source/WebCore/loader/FrameLoader.cpp 2020-08-14 19:13:21 UTC (rev 265692)
+++ branches/safari-610.1-branch/Source/WebCore/loader/FrameLoader.cpp 2020-08-14 19:48:29 UTC (rev 265693)
@@ -547,23 +547,6 @@
}
}
-void FrameLoader::willTransitionToCommitted()
-{
- // This function is called when a frame is still fully in place (not cached, not detached), but will be replaced.
- Document* document = m_frame.document();
- if (!document)
- return;
-
- if (document->editor().hasComposition()) {
- // The text was already present in DOM, so it's better to confirm than to cancel the composition.
- document->editor().confirmComposition();
- if (EditorClient* editorClient = document->editor().client()) {
- editorClient->respondToChangedSelection(&m_frame);
- editorClient->discardedComposition(&m_frame);
- }
- }
-}
-
void FrameLoader::closeURL()
{
history().saveDocumentState();
@@ -2008,7 +1991,13 @@
m_frame.document() ? m_frame.document()->url().stringCenterEllipsizedToLength().utf8().data() : "",
pdl ? pdl->url().stringCenterEllipsizedToLength().utf8().data() : "<no provisional DocumentLoader>", cachedPage.get());
- willTransitionToCommitted();
+ if (auto document = makeRefPtr(m_frame.document())) {
+ // In the case where we're restoring from a cached page, our document will not
+ // be connected to its frame yet, so the following call with be a no-op. We will
+ // attempt to confirm any active composition once again in this scenario after we
+ // finish restoring from the cached page.
+ document->editor().confirmCompositionAndNotifyClient();
+ }
if (!m_frame.tree().parent() && history().currentItem() && history().currentItem() != history().provisionalItem()) {
// Check to see if we need to cache the page we are navigating away from into the back/forward cache.
@@ -2087,6 +2076,9 @@
} else
didOpenURL();
+ if (auto document = makeRefPtr(m_frame.document()))
+ document->editor().confirmCompositionAndNotifyClient();
+
LOG(Loading, "WebCoreLoading %s: Finished committing provisional load to URL %s", m_frame.tree().uniqueName().string().utf8().data(),
m_frame.document() ? m_frame.document()->url().stringCenterEllipsizedToLength().utf8().data() : "");
Modified: branches/safari-610.1-branch/Source/WebCore/loader/FrameLoader.h (265692 => 265693)
--- branches/safari-610.1-branch/Source/WebCore/loader/FrameLoader.h 2020-08-14 19:13:21 UTC (rev 265692)
+++ branches/safari-610.1-branch/Source/WebCore/loader/FrameLoader.h 2020-08-14 19:48:29 UTC (rev 265693)
@@ -398,7 +398,6 @@
void prepareForLoadStart();
void provisionalLoadStarted();
- void willTransitionToCommitted();
bool didOpenURL();
void scheduleCheckCompleted();
Modified: branches/safari-610.1-branch/Tools/ChangeLog (265692 => 265693)
--- branches/safari-610.1-branch/Tools/ChangeLog 2020-08-14 19:13:21 UTC (rev 265692)
+++ branches/safari-610.1-branch/Tools/ChangeLog 2020-08-14 19:48:29 UTC (rev 265693)
@@ -1,3 +1,86 @@
+2020-08-14 Alan Coon <[email protected]>
+
+ Cherry-pick r265420. rdar://problem/67083903
+
+ REGRESSION (r260831): Web process crashes under Editor::setComposition() after navigating with marked text
+ https://bugs.webkit.org/show_bug.cgi?id=215315
+ <rdar://problem/64740092>
+
+ Reviewed by Darin Adler.
+
+ Source/WebCore:
+
+ To address a variety of crashes due to frames changing (or otherwise losing) their document while executing
+ editing commands, r260831 refactored the Editor class such that it extends the functionality of the Document
+ class, rather than the Frame class. In nearly all scenarios, this either leads to no behavior change or prevents
+ null pointer crashes, since a document is almost always attached to a frame when applying any editing commands.
+
+ However, there is one scenario where a document that has not yet been attached to its frame (and therefore does
+ not have a browsing context) will cause a null deref when trying to confirm an existing IME composition. The
+ logic added in <https://trac.webkit.org/r150291> will try and confirm any existing composition range on a
+ document right before committing provisional navigation. In the case where we are navigating back to a
+ previously visited page, `m_frame`'s document in `FrameLoader::commitProvisionalLoad()` will not be attached
+ until the cached page's mainframe is opened underneath `CachedPage::restore()`. Since the call to
+ `Editor::confirmComposition()` currently happens before this step, we end up crashing while attempting to create
+ a `UserTypingGestureIndicator`. Note that even if we avoid this with a null check, we'll still end up crashing
+ shortly thereafter, underneath `Editor::insertTextForConfirmedComposition`. And even if this second crash is
+ avoided with another null check, we'll just end up with some version of webkit.org/b/59121, where the
+ composition range is present after navigation, but is out of sync with platform UI.
+
+ To fix the crash (and also not bring back bug #59121), we refactor this composition confirmation logic so that
+ it lives in Editor, and is also robust against the case where the document is not attached to a frame; we then
+ invoke this call after we're done committing the provisional load, so that any frame that is not yet attached
+ before commiting the load still has a chance to confirm its composition.
+
+ Test: WKWebViewMacEditingTests.ProcessSwapAfterSettingMarkedText
+
+ * editing/Editor.cpp:
+ (WebCore::Editor::confirmCompositionAndNotifyClient):
+
+ Move functionality from `willTransitionToCommitted` to `confirmCompositionAndNotifyClient`, a helper method that
+ will bail if the document is not attached, but otherwise confirm the active composition (if it exists).
+
+ * editing/Editor.h:
+ * loader/FrameLoader.cpp:
+ (WebCore::FrameLoader::commitProvisionalLoad):
+
+ Add a call to confirm the editor's current composition after we're done committing the load. Note that in the
+ case where we had a composition before committing the load, we'll end up confirming the composition earlier (in
+ the first call site), rather than confirming after the load has been committed. This means that this second call
+ will be a no-op, due to the editor not having any composition.
+
+ (WebCore::FrameLoader::willTransitionToCommitted): Deleted.
+ * loader/FrameLoader.h:
+
+ Tools:
+
+ Add a new API that exercises the crash by:
+ - Enabling PSON.
+ - Navigating to page A and inserting some marked text.
+ - Navigating to page B with a process swap (without confirming the marked text).
+ - Navigating back to page A, and verifying that the previoulsy marked text is now committed.
+
+ * TestWebKitAPI/Tests/mac/WKWebViewMacEditingTests.mm:
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@265420 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2020-08-09 Wenson Hsieh <[email protected]>
+
+ REGRESSION (r260831): Web process crashes under Editor::setComposition() after navigating with marked text
+ https://bugs.webkit.org/show_bug.cgi?id=215315
+ <rdar://problem/64740092>
+
+ Reviewed by Darin Adler.
+
+ Add a new API that exercises the crash by:
+ - Enabling PSON.
+ - Navigating to page A and inserting some marked text.
+ - Navigating to page B with a process swap (without confirming the marked text).
+ - Navigating back to page A, and verifying that the previoulsy marked text is now committed.
+
+ * TestWebKitAPI/Tests/mac/WKWebViewMacEditingTests.mm:
+
2020-08-14 Russell Epstein <[email protected]>
Cherry-pick r265271. rdar://problem/66945210
Modified: branches/safari-610.1-branch/Tools/TestWebKitAPI/Tests/mac/WKWebViewMacEditingTests.mm (265692 => 265693)
--- branches/safari-610.1-branch/Tools/TestWebKitAPI/Tests/mac/WKWebViewMacEditingTests.mm 2020-08-14 19:13:21 UTC (rev 265692)
+++ branches/safari-610.1-branch/Tools/TestWebKitAPI/Tests/mac/WKWebViewMacEditingTests.mm 2020-08-14 19:48:29 UTC (rev 265693)
@@ -29,7 +29,11 @@
#import "AppKitSPI.h"
#import "PlatformUtilities.h"
+#import "TestNavigationDelegate.h"
+#import "TestProtocol.h"
#import "TestWKWebView.h"
+#import <WebKit/WKProcessPoolPrivate.h>
+#import <WebKit/_WKProcessPoolConfiguration.h>
#import <pal/spi/mac/NSTextInputContextSPI.h>
#import <wtf/BlockPtr.h>
#import <wtf/RetainPtr.h>
@@ -137,4 +141,41 @@
TestWebKitAPI::Util::run(&isDone);
}
+TEST(WKWebViewMacEditingTests, ProcessSwapAfterSettingMarkedText)
+{
+ [TestProtocol registerWithScheme:@"https"];
+
+ auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
+ [processPoolConfiguration setProcessSwapsOnNavigation:YES];
+
+ auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
+ auto configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
+ [configuration setProcessPool:processPool.get()];
+
+ auto navigationDelegate = adoptNS([[TestNavigationDelegate alloc] init]);
+ auto webView = adoptNS([[TestWKWebView<NSTextInputClient, NSTextInputClient_Async> alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:configuration.get()]);
+ [webView setNavigationDelegate:navigationDelegate.get()];
+ [webView _setEditable:YES];
+
+ [webView loadRequest:[NSURLRequest requestWithURL:[NSURL URLWithString:@"https://bundle-file/simple.html"]]];
+ [navigationDelegate waitForDidFinishNavigation];
+
+ [webView selectAll:nil];
+ [webView setMarkedText:@"Simple" selectedRange:NSMakeRange(0, 6) replacementRange:NSMakeRange(0, 6)];
+ [webView waitForNextPresentationUpdate];
+
+ [webView loadRequest:[NSURLRequest requestWithURL:[NSURL URLWithString:@"https://webkit.org"]]];
+ [navigationDelegate waitForDidFinishNavigation];
+
+ [webView goBack];
+ [navigationDelegate waitForDidFinishNavigation];
+
+ __block bool done = false;
+ [webView hasMarkedTextWithCompletionHandler:^(BOOL hasMarkedText) {
+ EXPECT_FALSE(hasMarkedText);
+ done = true;
+ }];
+ TestWebKitAPI::Util::run(&done);
+}
+
#endif // PLATFORM(MAC)
