- Revision
- 211238
- Author
- fpi...@apple.com
- Date
- 2017-01-26 16:11:54 -0800 (Thu, 26 Jan 2017)
Log Message
EventTarget should visit the JSEventListeners using visitAdditionalChildren
https://bugs.webkit.org/show_bug.cgi?id=167462
Reviewed by Michael Saboff.
No new tests because this is already caught by existing testing. This would show up as ASSERTs
in debug, and we suspect it might be at fault for null deref crashes.
Previously, EventTarget would have its event listeners visited by its subclasses' visitChildren
methods. Every subclass of EventTarget would call EventTarget's visitJSEventListeners. For
example, this means that if JSFoo has seven classes between it and JSEventTarget in the JSCell
class hierarchy, then JSFoo::visitChildren would end up calling visitJSEventListeners seven extra
times.
Also, the weird way that visitJSEventListeners was called meant that it was not part of the GC's
output constraint processing. This meant that it would not be called when the GC tried to
terminate. So, if something about the event listener changes during a GC cycle, the GC would
potentially fail to mark one of the references.
Both problems can be solved by simply moving the call to visitJSEventListeners into
visitAdditionalChildren.
* bindings/js/JSDOMWindowCustom.cpp:
(WebCore::JSDOMWindow::visitAdditionalChildren):
* bindings/js/JSEventTargetCustom.cpp:
(WebCore::JSEventTarget::visitAdditionalChildren):
* bindings/scripts/CodeGeneratorJS.pm:
(GenerateImplementation):
* dom/EventTarget.idl:
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (211237 => 211238)
--- trunk/Source/WebCore/ChangeLog 2017-01-26 23:50:58 UTC (rev 211237)
+++ trunk/Source/WebCore/ChangeLog 2017-01-27 00:11:54 UTC (rev 211238)
@@ -1,3 +1,35 @@
+2017-01-26 Filip Pizlo <fpi...@apple.com>
+
+ EventTarget should visit the JSEventListeners using visitAdditionalChildren
+ https://bugs.webkit.org/show_bug.cgi?id=167462
+
+ Reviewed by Michael Saboff.
+
+ No new tests because this is already caught by existing testing. This would show up as ASSERTs
+ in debug, and we suspect it might be at fault for null deref crashes.
+
+ Previously, EventTarget would have its event listeners visited by its subclasses' visitChildren
+ methods. Every subclass of EventTarget would call EventTarget's visitJSEventListeners. For
+ example, this means that if JSFoo has seven classes between it and JSEventTarget in the JSCell
+ class hierarchy, then JSFoo::visitChildren would end up calling visitJSEventListeners seven extra
+ times.
+
+ Also, the weird way that visitJSEventListeners was called meant that it was not part of the GC's
+ output constraint processing. This meant that it would not be called when the GC tried to
+ terminate. So, if something about the event listener changes during a GC cycle, the GC would
+ potentially fail to mark one of the references.
+
+ Both problems can be solved by simply moving the call to visitJSEventListeners into
+ visitAdditionalChildren.
+
+ * bindings/js/JSDOMWindowCustom.cpp:
+ (WebCore::JSDOMWindow::visitAdditionalChildren):
+ * bindings/js/JSEventTargetCustom.cpp:
+ (WebCore::JSEventTarget::visitAdditionalChildren):
+ * bindings/scripts/CodeGeneratorJS.pm:
+ (GenerateImplementation):
+ * dom/EventTarget.idl:
+
2017-01-26 Andy Estes <aes...@apple.com>
[QuickLook] Create temporary files with NSFileProtectionCompleteUnlessOpen
Modified: trunk/Source/WebCore/bindings/js/JSDOMWindowCustom.cpp (211237 => 211238)
--- trunk/Source/WebCore/bindings/js/JSDOMWindowCustom.cpp 2017-01-26 23:50:58 UTC (rev 211237)
+++ trunk/Source/WebCore/bindings/js/JSDOMWindowCustom.cpp 2017-01-27 00:11:54 UTC (rev 211238)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2007-2010, 2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2007-2017 Apple Inc. All rights reserved.
* Copyright (C) 2011 Google Inc. All rights reserved.
*
* This library is free software; you can redistribute it and/or
@@ -52,6 +52,11 @@
{
if (Frame* frame = wrapped().frame())
visitor.addOpaqueRoot(frame);
+
+ // Normally JSEventTargetCustom.cpp's JSEventTarget::visitAdditionalChildren() would call this. But
+ // even though DOMWindow is an EventTarget, JSDOMWindow does not subclass JSEventTarget, so we need
+ // to do this here.
+ wrapped().visitJSEventListeners(visitor);
}
#if ENABLE(USER_MESSAGE_HANDLERS)
Modified: trunk/Source/WebCore/bindings/js/JSEventTargetCustom.cpp (211237 => 211238)
--- trunk/Source/WebCore/bindings/js/JSEventTargetCustom.cpp 2017-01-26 23:50:58 UTC (rev 211237)
+++ trunk/Source/WebCore/bindings/js/JSEventTargetCustom.cpp 2017-01-27 00:11:54 UTC (rev 211238)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2008, 2016 Apple Inc. All Rights Reserved.
+ * Copyright (C) 2008-2017 Apple Inc. All Rights Reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -82,4 +82,9 @@
return nullptr;
}
+void JSEventTarget::visitAdditionalChildren(SlotVisitor& visitor)
+{
+ wrapped().visitJSEventListeners(visitor);
+}
+
} // namespace WebCore
Modified: trunk/Source/WebCore/bindings/js/JSWorkerGlobalScopeCustom.cpp (211237 => 211238)
--- trunk/Source/WebCore/bindings/js/JSWorkerGlobalScopeCustom.cpp 2017-01-26 23:50:58 UTC (rev 211237)
+++ trunk/Source/WebCore/bindings/js/JSWorkerGlobalScopeCustom.cpp 2017-01-27 00:11:54 UTC (rev 211238)
@@ -42,6 +42,11 @@
visitor.addOpaqueRoot(navigator);
ScriptExecutionContext& context = wrapped();
visitor.addOpaqueRoot(&context);
+
+ // Normally JSEventTargetCustom.cpp's JSEventTarget::visitAdditionalChildren() would call this. But
+ // even though WorkerGlobalScope is an EventTarget, JSWorkerGlobalScope does not subclass
+ // JSEventTarget, so we need to do this here.
+ wrapped().visitJSEventListeners(visitor);
}
JSValue JSWorkerGlobalScope::setTimeout(ExecState& state)
Modified: trunk/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm (211237 => 211238)
--- trunk/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm 2017-01-26 23:50:58 UTC (rev 211237)
+++ trunk/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm 2017-01-27 00:11:54 UTC (rev 211238)
@@ -4158,9 +4158,6 @@
push(@implContent, " auto* thisObject = jsCast<${className}*>(cell);\n");
push(@implContent, " ASSERT_GC_OBJECT_INHERITS(thisObject, info());\n");
push(@implContent, " Base::visitChildren(thisObject, visitor);\n");
- if ($codeGenerator->InheritsInterface($interface, "EventTarget")) {
- push(@implContent, " thisObject->wrapped().visitJSEventListeners(visitor);\n");
- }
push(@implContent, " thisObject->visitAdditionalChildren(visitor);\n") if $interface->extendedAttributes->{JSCustomMarkFunction};
if ($interface->extendedAttributes->{ReportExtraMemoryCost}) {
push(@implContent, " visitor.reportExtraMemoryVisited(thisObject->wrapped().memoryCost());\n");
Modified: trunk/Source/WebCore/dom/EventTarget.idl (211237 => 211238)
--- trunk/Source/WebCore/dom/EventTarget.idl 2017-01-26 23:50:58 UTC (rev 211237)
+++ trunk/Source/WebCore/dom/EventTarget.idl 2017-01-27 00:11:54 UTC (rev 211238)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2006, 2007 Apple Inc. All rights reserved.
+ * Copyright (C) 2006-2017 Apple Inc. All rights reserved.
* Copyright (C) 2006 Samuel Weinig <sam.wei...@gmail.com>
*
* This library is free software; you can redistribute it and/or
@@ -23,6 +23,7 @@
Exposed=(Window,Worker),
IsImmutablePrototypeExoticObjectOnPrototype,
JSCustomHeader,
+ JSCustomMarkFunction,
JSCustomToNativeObject,
] interface EventTarget {
[ImplementedAs=addEventListenerForBindings] void addEventListener([AtomicString] DOMString type, EventListener? callback, optional (AddEventListenerOptions or boolean) options = false);
