Title: [278486] branches/safari-611-branch
- Revision
- 278486
- Author
- alanc...@apple.com
- Date
- 2021-06-04 13:24:08 -0700 (Fri, 04 Jun 2021)
Log Message
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=""
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
Modified Paths
Diff
Modified: branches/safari-611-branch/Source/WebKit/ChangeLog (278485 => 278486)
--- 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)
@@ -1,3 +1,99 @@
+2021-06-04 Alan Coon <alanc...@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=""
+ 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 <cdu...@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=""
+ 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):
+
2021-05-20 Alan Coon <alanc...@apple.com>
Cherry-pick r277713. rdar://problem/78264364
Modified: branches/safari-611-branch/Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm (278485 => 278486)
--- 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)
@@ -64,7 +64,7 @@
API::Array* _objectStream;
API::Dictionary* _currentDictionary;
- RetainPtr<NSMutableSet> _objectsBeingEncoded; // Used to detect cycles.
+ HashSet<NSObject *> _objectsBeingEncoded; // Used to detect cycles.
}
- (id)init
@@ -74,7 +74,6 @@
_rootDictionary = API::Dictionary::create();
_currentDictionary = _rootDictionary.get();
- _objectsBeingEncoded = adoptNS([[NSMutableSet alloc] init]);
return self;
}
@@ -249,7 +248,12 @@
id value;
[invocation getArgument:&value atIndex:i];
- encodeToObjectStream(encoder, value);
+ @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");
+ }
+
break;
}
@@ -424,20 +428,15 @@
if (!objectClass)
[NSException raise:NSInvalidArgumentException format:@"-classForCoder returned nil for %@", object];
- if ([encoder->_objectsBeingEncoded containsObject:object]) {
+ if (encoder->_objectsBeingEncoded.contains(object)) {
RELEASE_LOG_FAULT(IPC, "WKRemoteObjectCode::encodeObject: Object of type '%{private}s' contains a cycle", class_getName(object_getClass(object)));
- @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))];
- }
+ [NSException raise:NSInvalidArgumentException format:@"Object of type '%s' contains a cycle", class_getName(object_getClass(object))];
+ return;
}
- [encoder->_objectsBeingEncoded addObject:object];
+ encoder->_objectsBeingEncoded.add(object);
auto exitScope = makeScopeExit([encoder, object] {
- [encoder->_objectsBeingEncoded removeObject:object];
+ encoder->_objectsBeingEncoded.remove(object);
});
encoder->_currentDictionary->set(classNameKey, API::String::create(class_getName(objectClass)));
Modified: branches/safari-611-branch/Tools/TestWebKitAPI/Tests/WebKitCocoa/RemoteObjectRegistry.mm (278485 => 278486)
--- 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)
@@ -152,18 +152,7 @@
[child setValue:dictionaryWithCycle forKey:@"parent"]; // Creates a cycle.
@try {
[object takeDictionary:dictionaryWithCycle completionHandler:^(NSDictionary* value) {
- 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]);
+ EXPECT_TRUE(!value.count);
isDone = true;
}];
TestWebKitAPI::Util::run(&isDone);
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
