<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[265420] trunk</title>
</head>
<body>

<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt;  }
#msg dl a { font-weight: bold}
#msg dl a:link    { color:#fc3; }
#msg dl a:active  { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff  {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/265420">265420</a></dd>
<dt>Author</dt> <dd>wenson_hsieh@apple.com</dd>
<dt>Date</dt> <dd>2020-08-09 19:18:44 -0700 (Sun, 09 Aug 2020)</dd>
</dl>

<h3>Log Message</h3>
<pre>REGRESSION (<a href="http://trac.webkit.org/projects/webkit/changeset/260831">r260831</a>): 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, <a href="http://trac.webkit.org/projects/webkit/changeset/260831">r260831</a> 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/<a href="http://trac.webkit.org/projects/webkit/changeset/150291">r150291</a>> 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:</pre>

<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceWebCoreChangeLog">trunk/Source/WebCore/ChangeLog</a></li>
<li><a href="#trunkSourceWebCoreeditingEditorcpp">trunk/Source/WebCore/editing/Editor.cpp</a></li>
<li><a href="#trunkSourceWebCoreeditingEditorh">trunk/Source/WebCore/editing/Editor.h</a></li>
<li><a href="#trunkSourceWebCoreloaderFrameLoadercpp">trunk/Source/WebCore/loader/FrameLoader.cpp</a></li>
<li><a href="#trunkSourceWebCoreloaderFrameLoaderh">trunk/Source/WebCore/loader/FrameLoader.h</a></li>
<li><a href="#trunkToolsChangeLog">trunk/Tools/ChangeLog</a></li>
<li><a href="#trunkToolsTestWebKitAPITestsmacWKWebViewMacEditingTestsmm">trunk/Tools/TestWebKitAPI/Tests/mac/WKWebViewMacEditingTests.mm</a></li>
</ul>

</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/ChangeLog (265419 => 265420)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/ChangeLog   2020-08-09 20:41:43 UTC (rev 265419)
+++ trunk/Source/WebCore/ChangeLog      2020-08-10 02:18:44 UTC (rev 265420)
</span><span class="lines">@@ -1,3 +1,53 @@
</span><ins>+2020-08-09  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        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:
+
</ins><span class="cx"> 2020-08-09  Ben Nham  <nham@apple.com>
</span><span class="cx"> 
</span><span class="cx">         Preload graphics drivers in Mac WebProcess
</span></span></pre></div>
<a id="trunkSourceWebCoreeditingEditorcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/editing/Editor.cpp (265419 => 265420)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/editing/Editor.cpp  2020-08-09 20:41:43 UTC (rev 265419)
+++ trunk/Source/WebCore/editing/Editor.cpp     2020-08-10 02:18:44 UTC (rev 265420)
</span><span class="lines">@@ -1939,6 +1939,23 @@
</span><span class="cx">     setComposition(m_compositionNode->data().substring(m_compositionStart, m_compositionEnd - m_compositionStart), ConfirmComposition);
</span><span class="cx"> }
</span><span class="cx"> 
</span><ins>+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());
+    }
+}
+
</ins><span class="cx"> void Editor::cancelComposition()
</span><span class="cx"> {
</span><span class="cx">     if (!m_compositionNode)
</span></span></pre></div>
<a id="trunkSourceWebCoreeditingEditorh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/editing/Editor.h (265419 => 265420)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/editing/Editor.h    2020-08-09 20:41:43 UTC (rev 265419)
+++ trunk/Source/WebCore/editing/Editor.h       2020-08-10 02:18:44 UTC (rev 265420)
</span><span class="lines">@@ -379,6 +379,7 @@
</span><span class="cx">     WEBCORE_EXPORT void setComposition(const String&, const Vector<CompositionUnderline>&, const Vector<CompositionHighlight>&, unsigned selectionStart, unsigned selectionEnd);
</span><span class="cx">     WEBCORE_EXPORT void confirmComposition();
</span><span class="cx">     WEBCORE_EXPORT void confirmComposition(const String&); // if no existing composition, replaces selection
</span><ins>+    void confirmCompositionAndNotifyClient();
</ins><span class="cx">     WEBCORE_EXPORT void cancelComposition();
</span><span class="cx">     bool cancelCompositionIfSelectionIsInvalid();
</span><span class="cx">     WEBCORE_EXPORT Optional<SimpleRange> compositionRange() const;
</span></span></pre></div>
<a id="trunkSourceWebCoreloaderFrameLoadercpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/loader/FrameLoader.cpp (265419 => 265420)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/loader/FrameLoader.cpp      2020-08-09 20:41:43 UTC (rev 265419)
+++ trunk/Source/WebCore/loader/FrameLoader.cpp 2020-08-10 02:18:44 UTC (rev 265420)
</span><span class="lines">@@ -547,23 +547,6 @@
</span><span class="cx">     }
</span><span class="cx"> }
</span><span class="cx"> 
</span><del>-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);
-        }
-    }
-}
-
</del><span class="cx"> void FrameLoader::closeURL()
</span><span class="cx"> {
</span><span class="cx">     history().saveDocumentState();
</span><span class="lines">@@ -2008,7 +1991,13 @@
</span><span class="cx">         m_frame.document() ? m_frame.document()->url().stringCenterEllipsizedToLength().utf8().data() : "",
</span><span class="cx">         pdl ? pdl->url().stringCenterEllipsizedToLength().utf8().data() : "<no provisional DocumentLoader>", cachedPage.get());
</span><span class="cx"> 
</span><del>-    willTransitionToCommitted();
</del><ins>+    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();
+    }
</ins><span class="cx"> 
</span><span class="cx">     if (!m_frame.tree().parent() && history().currentItem() && history().currentItem() != history().provisionalItem()) {
</span><span class="cx">         // Check to see if we need to cache the page we are navigating away from into the back/forward cache.
</span><span class="lines">@@ -2087,6 +2076,9 @@
</span><span class="cx">     } else
</span><span class="cx">         didOpenURL();
</span><span class="cx"> 
</span><ins>+    if (auto document = makeRefPtr(m_frame.document()))
+        document->editor().confirmCompositionAndNotifyClient();
+
</ins><span class="cx">     LOG(Loading, "WebCoreLoading %s: Finished committing provisional load to URL %s", m_frame.tree().uniqueName().string().utf8().data(),
</span><span class="cx">         m_frame.document() ? m_frame.document()->url().stringCenterEllipsizedToLength().utf8().data() : "");
</span><span class="cx"> 
</span></span></pre></div>
<a id="trunkSourceWebCoreloaderFrameLoaderh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/loader/FrameLoader.h (265419 => 265420)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/loader/FrameLoader.h        2020-08-09 20:41:43 UTC (rev 265419)
+++ trunk/Source/WebCore/loader/FrameLoader.h   2020-08-10 02:18:44 UTC (rev 265420)
</span><span class="lines">@@ -398,7 +398,6 @@
</span><span class="cx">     void prepareForLoadStart();
</span><span class="cx">     void provisionalLoadStarted();
</span><span class="cx"> 
</span><del>-    void willTransitionToCommitted();
</del><span class="cx">     bool didOpenURL();
</span><span class="cx"> 
</span><span class="cx">     void scheduleCheckCompleted();
</span></span></pre></div>
<a id="trunkToolsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Tools/ChangeLog (265419 => 265420)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Tools/ChangeLog    2020-08-09 20:41:43 UTC (rev 265419)
+++ trunk/Tools/ChangeLog       2020-08-10 02:18:44 UTC (rev 265420)
</span><span class="lines">@@ -1,3 +1,19 @@
</span><ins>+2020-08-09  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        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:
+
</ins><span class="cx"> 2020-08-08  Fujii Hironori  <Hironori.Fujii@sony.com>
</span><span class="cx"> 
</span><span class="cx">         [WinCairo] REGRESSION(r265408): Unreviewed layout test script fix
</span></span></pre></div>
<a id="trunkToolsTestWebKitAPITestsmacWKWebViewMacEditingTestsmm"></a>
<div class="modfile"><h4>Modified: trunk/Tools/TestWebKitAPI/Tests/mac/WKWebViewMacEditingTests.mm (265419 => 265420)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Tools/TestWebKitAPI/Tests/mac/WKWebViewMacEditingTests.mm  2020-08-09 20:41:43 UTC (rev 265419)
+++ trunk/Tools/TestWebKitAPI/Tests/mac/WKWebViewMacEditingTests.mm     2020-08-10 02:18:44 UTC (rev 265420)
</span><span class="lines">@@ -29,7 +29,11 @@
</span><span class="cx"> 
</span><span class="cx"> #import "AppKitSPI.h"
</span><span class="cx"> #import "PlatformUtilities.h"
</span><ins>+#import "TestNavigationDelegate.h"
+#import "TestProtocol.h"
</ins><span class="cx"> #import "TestWKWebView.h"
</span><ins>+#import <WebKit/WKProcessPoolPrivate.h>
+#import <WebKit/_WKProcessPoolConfiguration.h>
</ins><span class="cx"> #import <pal/spi/mac/NSTextInputContextSPI.h>
</span><span class="cx"> #import <wtf/BlockPtr.h>
</span><span class="cx"> #import <wtf/RetainPtr.h>
</span><span class="lines">@@ -137,4 +141,41 @@
</span><span class="cx">     TestWebKitAPI::Util::run(&isDone);
</span><span class="cx"> }
</span><span class="cx"> 
</span><ins>+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);
+}
+
</ins><span class="cx"> #endif // PLATFORM(MAC)
</span></span></pre>
</div>
</div>

</body>
</html>