<!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>[278486] branches/safari-611-branch</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/278486">278486</a></dd>
<dt>Author</dt> <dd>alancoon@apple.com</dd>
<dt>Date</dt> <dd>2021-06-04 13:24:08 -0700 (Fri, 04 Jun 2021)</dd>
</dl>

<h3>Log Message</h3>
<pre>Cherry-pick <a href="http://trac.webkit.org/projects/webkit/changeset/273141">r273141</a>. rdar://problem/78875378

    Norton Safe Web extension is causing crashes / hangs under [WKRemoteObjectEncoder encodeObject:forKey:]
    https://bugs.webkit.org/show_bug.cgi?id=222172

    Reviewed by Alex Christensen.

    The extension appears to be trying to send a JSValue that is a DOM Node. WebKit makes the following
    call to convert it into a NSDictionary:
    `[[JSValue valueWithJSValueRef:value inContext:[JSContext contextWithJSGlobalContextRef:JSContextGetGlobalContext(context)]] toObject]`

    JSC very aggressively iterates over all of the properties of the DOM Node and recursively ends up
    converting the whole DOM tree with all their properties. This leads to a lot of cycles to as JSC
    maintains the JSObject <-> NSObject identity during the conversion (Each time the JSDocument is
    serialized, the same NSDictionary* pointer is used to represent it).

    The logic introduced in <a href="http://trac.webkit.org/projects/webkit/changeset/270559">r270559</a> to detect cycles was flawed because it relied on a NSSet of
    NSObject* and [NSSet containsObject:] to detect the cycles. The issue is that [NSSet containsObject:]
    doesn't do a simple pointer comparison but instead calls [NSObject isEqual:] which is very
    expensive for types like NSDictionary and leads to trouble when the dictionary contains a cycle.
    To address this I replaced the NSSet with a WTF::HashSet<NSObject *> so that key lookup ends up
    doing a simple pointer comparison.

    Even after the previous fix, the extension would still cause massive hangs because it would take
    a very long time to try and encode the whole DOM tree with all the properties of each Node (even
    without cycles). To address this, we now abort encoding when detecting a cycle instead of encoding
    an empty object to break the cycle.

    After this change, Safari becomes usable with this extension again. However, there are still much
    shorter hangs that occur due to the converting of the JSNode into a JSDictionary via
    [JSValue toObject]. We should probably improve this in a follow-up.

    Easy way to reproduce the crash / hang:
    1. Install Norton Safe Web & Norton Password Manager extension (may require a subscription)
    2. Make sure the extensions are activated and turned on by clicking on their icons next to the
       URL bar
    3. Go to https://bugs.webkit.org/attachment.cgi?id=420530&action=edit
    4. Click on the combo box next to "Review" -> Hang / Crash

    No new tests, covered by WebKit.RemoteObjectRegistry API test.

    * Shared/API/Cocoa/WKRemoteObjectCoder.mm:
    (-[WKRemoteObjectEncoder init]):
    (encodeInvocationArguments):
    (encodeObject):

    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@273141 268f45cc-cd09-0410-ab3c-d52691b4dbfc</pre>

<h3>Modified Paths</h3>
<ul>
<li><a href="#branchessafari611branchSourceWebKitChangeLog">branches/safari-611-branch/Source/WebKit/ChangeLog</a></li>
<li><a href="#branchessafari611branchSourceWebKitSharedAPICocoaWKRemoteObjectCodermm">branches/safari-611-branch/Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm</a></li>
<li><a href="#branchessafari611branchToolsTestWebKitAPITestsWebKitCocoaRemoteObjectRegistrymm">branches/safari-611-branch/Tools/TestWebKitAPI/Tests/WebKitCocoa/RemoteObjectRegistry.mm</a></li>
</ul>

</div>
<div id="patch">
<h3>Diff</h3>
<a id="branchessafari611branchSourceWebKitChangeLog"></a>
<div class="modfile"><h4>Modified: branches/safari-611-branch/Source/WebKit/ChangeLog (278485 => 278486)</h4>
<pre class="diff"><span>
<span class="info">--- branches/safari-611-branch/Source/WebKit/ChangeLog       2021-06-04 20:24:05 UTC (rev 278485)
+++ branches/safari-611-branch/Source/WebKit/ChangeLog  2021-06-04 20:24:08 UTC (rev 278486)
</span><span class="lines">@@ -1,3 +1,99 @@
</span><ins>+2021-06-04  Alan Coon  <alancoon@apple.com>
+
+        Cherry-pick r273141. rdar://problem/78875378
+
+    Norton Safe Web extension is causing crashes / hangs under [WKRemoteObjectEncoder encodeObject:forKey:]
+    https://bugs.webkit.org/show_bug.cgi?id=222172
+    
+    Reviewed by Alex Christensen.
+    
+    The extension appears to be trying to send a JSValue that is a DOM Node. WebKit makes the following
+    call to convert it into a NSDictionary:
+    `[[JSValue valueWithJSValueRef:value inContext:[JSContext contextWithJSGlobalContextRef:JSContextGetGlobalContext(context)]] toObject]`
+    
+    JSC very aggressively iterates over all of the properties of the DOM Node and recursively ends up
+    converting the whole DOM tree with all their properties. This leads to a lot of cycles to as JSC
+    maintains the JSObject <-> NSObject identity during the conversion (Each time the JSDocument is
+    serialized, the same NSDictionary* pointer is used to represent it).
+    
+    The logic introduced in r270559 to detect cycles was flawed because it relied on a NSSet of
+    NSObject* and [NSSet containsObject:] to detect the cycles. The issue is that [NSSet containsObject:]
+    doesn't do a simple pointer comparison but instead calls [NSObject isEqual:] which is very
+    expensive for types like NSDictionary and leads to trouble when the dictionary contains a cycle.
+    To address this I replaced the NSSet with a WTF::HashSet<NSObject *> so that key lookup ends up
+    doing a simple pointer comparison.
+    
+    Even after the previous fix, the extension would still cause massive hangs because it would take
+    a very long time to try and encode the whole DOM tree with all the properties of each Node (even
+    without cycles). To address this, we now abort encoding when detecting a cycle instead of encoding
+    an empty object to break the cycle.
+    
+    After this change, Safari becomes usable with this extension again. However, there are still much
+    shorter hangs that occur due to the converting of the JSNode into a JSDictionary via
+    [JSValue toObject]. We should probably improve this in a follow-up.
+    
+    Easy way to reproduce the crash / hang:
+    1. Install Norton Safe Web & Norton Password Manager extension (may require a subscription)
+    2. Make sure the extensions are activated and turned on by clicking on their icons next to the
+       URL bar
+    3. Go to https://bugs.webkit.org/attachment.cgi?id=420530&action=edit
+    4. Click on the combo box next to "Review" -> Hang / Crash
+    
+    No new tests, covered by WebKit.RemoteObjectRegistry API test.
+    
+    * Shared/API/Cocoa/WKRemoteObjectCoder.mm:
+    (-[WKRemoteObjectEncoder init]):
+    (encodeInvocationArguments):
+    (encodeObject):
+    
+    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@273141 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+    2021-02-19  Chris Dumez  <cdumez@apple.com>
+
+            Norton Safe Web extension is causing crashes / hangs under [WKRemoteObjectEncoder encodeObject:forKey:]
+            https://bugs.webkit.org/show_bug.cgi?id=222172
+
+            Reviewed by Alex Christensen.
+
+            The extension appears to be trying to send a JSValue that is a DOM Node. WebKit makes the following
+            call to convert it into a NSDictionary:
+            `[[JSValue valueWithJSValueRef:value inContext:[JSContext contextWithJSGlobalContextRef:JSContextGetGlobalContext(context)]] toObject]`
+
+            JSC very aggressively iterates over all of the properties of the DOM Node and recursively ends up
+            converting the whole DOM tree with all their properties. This leads to a lot of cycles to as JSC
+            maintains the JSObject <-> NSObject identity during the conversion (Each time the JSDocument is
+            serialized, the same NSDictionary* pointer is used to represent it).
+
+            The logic introduced in r270559 to detect cycles was flawed because it relied on a NSSet of
+            NSObject* and [NSSet containsObject:] to detect the cycles. The issue is that [NSSet containsObject:]
+            doesn't do a simple pointer comparison but instead calls [NSObject isEqual:] which is very
+            expensive for types like NSDictionary and leads to trouble when the dictionary contains a cycle.
+            To address this I replaced the NSSet with a WTF::HashSet<NSObject *> so that key lookup ends up
+            doing a simple pointer comparison.
+
+            Even after the previous fix, the extension would still cause massive hangs because it would take
+            a very long time to try and encode the whole DOM tree with all the properties of each Node (even
+            without cycles). To address this, we now abort encoding when detecting a cycle instead of encoding
+            an empty object to break the cycle.
+
+            After this change, Safari becomes usable with this extension again. However, there are still much
+            shorter hangs that occur due to the converting of the JSNode into a JSDictionary via
+            [JSValue toObject]. We should probably improve this in a follow-up.
+
+            Easy way to reproduce the crash / hang:
+            1. Install Norton Safe Web & Norton Password Manager extension (may require a subscription)
+            2. Make sure the extensions are activated and turned on by clicking on their icons next to the
+               URL bar
+            3. Go to https://bugs.webkit.org/attachment.cgi?id=420530&action=edit
+            4. Click on the combo box next to "Review" -> Hang / Crash
+
+            No new tests, covered by WebKit.RemoteObjectRegistry API test.
+
+            * Shared/API/Cocoa/WKRemoteObjectCoder.mm:
+            (-[WKRemoteObjectEncoder init]):
+            (encodeInvocationArguments):
+            (encodeObject):
+
</ins><span class="cx"> 2021-05-20  Alan Coon  <alancoon@apple.com>
</span><span class="cx"> 
</span><span class="cx">         Cherry-pick r277713. rdar://problem/78264364
</span></span></pre></div>
<a id="branchessafari611branchSourceWebKitSharedAPICocoaWKRemoteObjectCodermm"></a>
<div class="modfile"><h4>Modified: branches/safari-611-branch/Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm (278485 => 278486)</h4>
<pre class="diff"><span>
<span class="info">--- branches/safari-611-branch/Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm 2021-06-04 20:24:05 UTC (rev 278485)
+++ branches/safari-611-branch/Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm    2021-06-04 20:24:08 UTC (rev 278486)
</span><span class="lines">@@ -64,7 +64,7 @@
</span><span class="cx">     API::Array* _objectStream;
</span><span class="cx"> 
</span><span class="cx">     API::Dictionary* _currentDictionary;
</span><del>-    RetainPtr<NSMutableSet> _objectsBeingEncoded; // Used to detect cycles.
</del><ins>+    HashSet<NSObject *> _objectsBeingEncoded; // Used to detect cycles.
</ins><span class="cx"> }
</span><span class="cx"> 
</span><span class="cx"> - (id)init
</span><span class="lines">@@ -74,7 +74,6 @@
</span><span class="cx"> 
</span><span class="cx">     _rootDictionary = API::Dictionary::create();
</span><span class="cx">     _currentDictionary = _rootDictionary.get();
</span><del>-    _objectsBeingEncoded = adoptNS([[NSMutableSet alloc] init]);
</del><span class="cx"> 
</span><span class="cx">     return self;
</span><span class="cx"> }
</span><span class="lines">@@ -249,7 +248,12 @@
</span><span class="cx">             id value;
</span><span class="cx">             [invocation getArgument:&value atIndex:i];
</span><span class="cx"> 
</span><del>-            encodeToObjectStream(encoder, value);
</del><ins>+            @try {
+                encodeToObjectStream(encoder, value);
+            } @catch (NSException *e) {
+                RELEASE_LOG_ERROR(IPC, "WKRemoteObjectCode::encodeInvocationArguments: Exception caught when trying to encode an argument of type ObjC Object");
+            }
+
</ins><span class="cx">             break;
</span><span class="cx">         }
</span><span class="cx"> 
</span><span class="lines">@@ -424,20 +428,15 @@
</span><span class="cx">     if (!objectClass)
</span><span class="cx">         [NSException raise:NSInvalidArgumentException format:@"-classForCoder returned nil for %@", object];
</span><span class="cx"> 
</span><del>-    if ([encoder->_objectsBeingEncoded containsObject:object]) {
</del><ins>+    if (encoder->_objectsBeingEncoded.contains(object)) {
</ins><span class="cx">         RELEASE_LOG_FAULT(IPC, "WKRemoteObjectCode::encodeObject: Object of type '%{private}s' contains a cycle", class_getName(object_getClass(object)));
</span><del>-        @try {
-            // Try to encode a newly initialized object instead.
-            id newObject = [[[[object class] alloc] init] autorelease];
-            object = newObject;
-        } @catch (NSException *e) {
-            [NSException raise:NSInvalidArgumentException format:@"Object of type '%s' contains a cycle", class_getName(object_getClass(object))];
-        }
</del><ins>+        [NSException raise:NSInvalidArgumentException format:@"Object of type '%s' contains a cycle", class_getName(object_getClass(object))];
+        return;
</ins><span class="cx">     }
</span><span class="cx"> 
</span><del>-    [encoder->_objectsBeingEncoded addObject:object];
</del><ins>+    encoder->_objectsBeingEncoded.add(object);
</ins><span class="cx">     auto exitScope = makeScopeExit([encoder, object] {
</span><del>-        [encoder->_objectsBeingEncoded removeObject:object];
</del><ins>+        encoder->_objectsBeingEncoded.remove(object);
</ins><span class="cx">     });
</span><span class="cx"> 
</span><span class="cx">     encoder->_currentDictionary->set(classNameKey, API::String::create(class_getName(objectClass)));
</span></span></pre></div>
<a id="branchessafari611branchToolsTestWebKitAPITestsWebKitCocoaRemoteObjectRegistrymm"></a>
<div class="modfile"><h4>Modified: branches/safari-611-branch/Tools/TestWebKitAPI/Tests/WebKitCocoa/RemoteObjectRegistry.mm (278485 => 278486)</h4>
<pre class="diff"><span>
<span class="info">--- branches/safari-611-branch/Tools/TestWebKitAPI/Tests/WebKitCocoa/RemoteObjectRegistry.mm 2021-06-04 20:24:05 UTC (rev 278485)
+++ branches/safari-611-branch/Tools/TestWebKitAPI/Tests/WebKitCocoa/RemoteObjectRegistry.mm    2021-06-04 20:24:08 UTC (rev 278486)
</span><span class="lines">@@ -152,18 +152,7 @@
</span><span class="cx">         [child setValue:dictionaryWithCycle forKey:@"parent"]; // Creates a cycle.
</span><span class="cx">         @try {
</span><span class="cx">             [object takeDictionary:dictionaryWithCycle completionHandler:^(NSDictionary* value) {
</span><del>-                NSString *name = [value objectForKey:@"name"];
-                EXPECT_WK_STREQ(@"root", name);
-                NSDictionary *child = [value objectForKey:@"child"];
-                EXPECT_TRUE(!!child);
-                NSString* childName = [child objectForKey:@"name"];
-                EXPECT_WK_STREQ(@"foo", childName);
-                NSNumber *childValue = [child objectForKey:@"value"];
-                EXPECT_EQ(1, [childValue integerValue]);
-                // We should have encoded parent as an empty NSDictionary.
-                NSDictionary *childParent = [child objectForKey:@"parent"];
-                EXPECT_TRUE(!!childParent);
-                EXPECT_EQ(0U, [childParent count]);
</del><ins>+                EXPECT_TRUE(!value.count);
</ins><span class="cx">                 isDone = true;
</span><span class="cx">             }];
</span><span class="cx">             TestWebKitAPI::Util::run(&isDone);
</span></span></pre>
</div>
</div>

</body>
</html>