[webkit-reviews] review requested: [Bug 65209] Make MessagePort Transferable. : [Attachment 103302] Address comments.

[bugzilla-daemon at webkit.org]

Luke Zarko <lukezarko+bugzilla at gmail.com> has asked  for review:
Bug 65209: Make MessagePort Transferable.
https://bugs.webkit.org/show_bug.cgi?id=65209

Attachment 103302: Address comments.
https://bugs.webkit.org/attachment.cgi?id=103302&action=review

------- Additional Comments from Luke Zarko <lukezarko+bugzilla at gmail.com>
In this revision I changed some conditions which would raise INVALID_STATE_ERR
to instead raise DATA_CLONE_ERR to be in line with the spec. These were in
particular:

--- a/Source/WebCore/bindings/js/JSMessagePortCustom.cpp
+++ b/Source/WebCore/bindings/js/JSMessagePortCustom.cpp
@@ -88,7 +88,7 @@ void fillTransferableArray(JSC::ExecState* exec, JSC::JSValue
value, Transferabl
	     return;
	 // Validation of non-null objects, per HTML5 spec 8.3.3.
	 if (value.isUndefinedOrNull()) {
-	     setDOMException(exec, INVALID_STATE_ERR);
+	     setDOMException(exec, DATA_CLONE_ERR);
	     return;
	 }
 
diff --git a/Source/WebCore/bindings/v8/custom/V8MessagePortCustom.cpp
b/Source/WebCore/bindings/v8/custom/V8MessagePortCustom.cpp
index e8b63b3..f087236 100644
--- a/Source/WebCore/bindings/v8/custom/V8MessagePortCustom.cpp
+++ b/Source/WebCore/bindings/v8/custom/V8MessagePortCustom.cpp
@@ -106,7 +106,7 @@ bool getTransferableArray(v8::Local<v8::Value> value,
TransferableArray& transfe
	 v8::Local<v8::Value> transferable = ports->Get(i);
	 // Validation of non-null objects, per HTML5 spec 8.3.3.
	 if (isUndefinedOrNull(transferable)) {
-	     throwError(INVALID_STATE_ERR);
+	     throwError(DATA_CLONE_ERR);
	     return false;
	 }
	 // Validation of Objects implementing an interface, per WebIDL spec
4.1.15.
diff --git a/Source/WebCore/dom/MessagePort.cpp
b/Source/WebCore/dom/MessagePort.cpp
index 59e5a0a..3d2eb4f 100644
--- a/Source/WebCore/dom/MessagePort.cpp
+++ b/Source/WebCore/dom/MessagePort.cpp
@@ -85,7 +85,7 @@ void
MessagePort::postMessage(PassRefPtr<SerializedScriptValue> message, Transfe
	     Transferable* transferable = (*transferables)[i].get();
	     if (MessagePort* dataPort = transferable->asMessagePort()) {
		 if (dataPort == this ||
m_entangledChannel->isConnectedTo(dataPort)) {
-		     ec = INVALID_STATE_ERR;
+		     ec = DATA_CLONE_ERR;
		     return;
		 }
	     }
diff --git a/Source/WebCore/dom/Transferable.cpp
b/Source/WebCore/dom/Transferable.cpp
index 892e77e..972ee74 100644
--- a/Source/WebCore/dom/Transferable.cpp
+++ b/Source/WebCore/dom/Transferable.cpp
@@ -46,7 +46,7 @@ PassOwnPtr<TransferableReceiptArray>
Transferable::transferAllOut(TransferableAr
     for (unsigned int i = 0; i < transferables->size(); ++i) {
	 Transferable* transferable = (*transferables)[i].get();
	 if (!transferable || transferable->isTransferClosed() ||
transferableSet.contains(transferable)) {
-	     ec = INVALID_STATE_ERR;
+	     ec = DATA_CLONE_ERR;
	     return nullptr;
	 }
	 transferableSet.add(transferable);

The no-release() bug in the JSC postMessage path appears to exist in the code
prior to this patch; I've looked around the code for similar problems, but it's
still worth watching for. What's the motivation for the implicit RefPtr<T> ->
PassRefPtr<T> conversion?