[Streams API] Shield implementation from user mangling Promise.reject and resolve...
authorcalvaris@igalia.com <calvaris@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 5 Nov 2015 14:01:13 +0000 (14:01 +0000)
committercalvaris@igalia.com <calvaris@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 5 Nov 2015 14:01:13 +0000 (14:01 +0000)
https://bugs.webkit.org/show_bug.cgi?id=150895

Reviewed by Youenn Fablet.

Source/JavaScriptCore:

Keep Promise.resolve and reject also as internal slots for the Promise constructor given that there is no way to
retrieve the former implementation if the user decides to replace it. This allows to safely create vended
promises even if the user changes the constructor methods.

* runtime/JSPromiseConstructor.h:
* runtime/JSPromiseConstructor.cpp:
(JSC::JSPromiseConstructor::addOwnInternalSlots): Added to include @reject and @resolve.
(JSC::JSPromiseConstructor::create): Call addOwnInternalSlots.

Source/WebCore:

Replace all calls to @Promise.resolve and @Promise.reject with their internal slot counterparts. This way we
ensure that if the user replaces those constructor methods, our implementation still works.

Test: streams/streams-promises.html.

* Modules/streams/ReadableStream.js:
(initializeReadableStream):
(cancel):
* Modules/streams/ReadableStreamInternals.js:
(privateInitializeReadableStreamReader):
(cancelReadableStream):
(readFromReadableStreamReader):
* Modules/streams/ReadableStreamReader.js:
(cancel):
(read):
(closed):
* Modules/streams/StreamInternals.js:
(promiseInvokeOrNoop):
(promiseInvokeOrFallbackOrNoop):
* Modules/streams/WritableStream.js:
(initializeWritableStream):
(abort):
(close):
(write):
(closed):
(ready):

LayoutTests:

* streams/streams-promises.html: Improved some style issues. Added tests about changing Promise.resolve and
reject.
* streams/streams-promises-expected.txt: Added expectations.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@192057 268f45cc-cd09-0410-ab3c-d52691b4dbfc

12 files changed:
LayoutTests/ChangeLog
LayoutTests/streams/streams-promises-expected.txt
LayoutTests/streams/streams-promises.html
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/JSPromiseConstructor.cpp
Source/JavaScriptCore/runtime/JSPromiseConstructor.h
Source/WebCore/ChangeLog
Source/WebCore/Modules/streams/ReadableStream.js
Source/WebCore/Modules/streams/ReadableStreamInternals.js
Source/WebCore/Modules/streams/ReadableStreamReader.js
Source/WebCore/Modules/streams/StreamInternals.js
Source/WebCore/Modules/streams/WritableStream.js

index b559f22..2d4597a 100644 (file)
@@ -1,3 +1,14 @@
+2015-11-05  Xabier Rodriguez Calvar  <calvaris@igalia.com>
+
+        [Streams API] Shield implementation from user mangling Promise.reject and resolve methods
+        https://bugs.webkit.org/show_bug.cgi?id=150895
+
+        Reviewed by Youenn Fablet.
+
+        * streams/streams-promises.html: Improved some style issues. Added tests about changing Promise.resolve and
+        reject.
+        * streams/streams-promises-expected.txt: Added expectations.
+
 2015-11-05  Manuel Rego Casasnovas  <rego@igalia.com>
 
         [css-grid] Support positioned grid children
index d5dc4b9..204aaea 100644 (file)
@@ -1,3 +1,5 @@
 
-PASS Streams can be built even if Promise constructor is replaced 
+PASS Streams implementation is not affected if Promise constructor is replaced 
+PASS Streams implementation is not affected if Promise.resolve is replaced 
+PASS Streams implementation is not affected if Promise.reject is replaced 
 
index 7394c9b..e559b1b 100644 (file)
@@ -2,10 +2,43 @@
 <script src='../resources/testharness.js'></script>
 <script src='../resources/testharnessreport.js'></script>
 <script>
+
+test(function() {
+    const PromiseBackup = Promise;
+
+    try {
+        Promise = function() { assert_unreached("streams should not use this Promise object"); };
+
+        new ReadableStream();
+        new WritableStream();
+    } finally {
+        Promise = PromiseBackup;
+    }
+}, 'Streams implementation is not affected if Promise constructor is replaced');
+
 test(function() {
-    Promise = function() { throw new Error("nasty things"); };
+    const PromiseResolveBackup = Promise.resolve;
+
+    try {
+        Promise.resolve = function() { assert_unreached("streams should not use this Promise.resolve method"); };
+
+        new ReadableStream();
+        new WritableStream();
+    } finally {
+        Promise.resolve = PromiseResolveBackup;
+    }
+}, 'Streams implementation is not affected if Promise.resolve is replaced');
+
+test(function() {
+    const PromiseRejectBackup = Promise.reject;
+
+    try {
+        Promise.reject = function() { assert_unreached("streams should not use this Promise.reject method"); };
 
-    const rs = new ReadableStream(); // Does not throw.
-    const ws = new WritableStream(); // Does not throw.
-}, 'Streams can be built even if Promise constructor is replaced');
+        ReadableStream.prototype.cancel.call({}, "reason");
+        WritableStream.prototype.abort.call({}, "reason");
+    } finally {
+        Promise.reject = PromiseRejectBackup;
+    }
+}, 'Streams implementation is not affected if Promise.reject is replaced');
 </script>
index 9786bf4..4f222ca 100644 (file)
@@ -1,3 +1,19 @@
+2015-11-05  Xabier Rodriguez Calvar  <calvaris@igalia.com>
+
+        [Streams API] Shield implementation from user mangling Promise.reject and resolve methods
+        https://bugs.webkit.org/show_bug.cgi?id=150895
+
+        Reviewed by Youenn Fablet.
+
+        Keep Promise.resolve and reject also as internal slots for the Promise constructor given that there is no way to
+        retrieve the former implementation if the user decides to replace it. This allows to safely create vended
+        promises even if the user changes the constructor methods.
+
+        * runtime/JSPromiseConstructor.h:
+        * runtime/JSPromiseConstructor.cpp:
+        (JSC::JSPromiseConstructor::addOwnInternalSlots): Added to include @reject and @resolve.
+        (JSC::JSPromiseConstructor::create): Call addOwnInternalSlots.
+
 2015-11-04  Benjamin Poulain  <bpoulain@apple.com>
 
         [JSC] Add B3-to-Air lowering for the shift opcodes
index a9ba1c5..097054d 100644 (file)
@@ -33,6 +33,7 @@
 #include "JSCBuiltins.h"
 #include "JSCJSValueInlines.h"
 #include "JSCellInlines.h"
+#include "JSFunction.h"
 #include "JSPromise.h"
 #include "JSPromisePrototype.h"
 #include "Lookup.h"
@@ -64,6 +65,7 @@ JSPromiseConstructor* JSPromiseConstructor::create(VM& vm, Structure* structure,
 {
     JSPromiseConstructor* constructor = new (NotNull, allocateCell<JSPromiseConstructor>(vm.heap)) JSPromiseConstructor(vm, structure);
     constructor->finishCreation(vm, promisePrototype);
+    constructor->addOwnInternalSlots(vm, structure->globalObject());
     return constructor;
 }
 
@@ -84,6 +86,12 @@ void JSPromiseConstructor::finishCreation(VM& vm, JSPromisePrototype* promisePro
     putDirectWithoutTransition(vm, vm.propertyNames->length, jsNumber(1), ReadOnly | DontEnum | DontDelete);
 }
 
+void JSPromiseConstructor::addOwnInternalSlots(VM& vm, JSGlobalObject* globalObject)
+{
+    JSC_BUILTIN_FUNCTION(vm.propertyNames->builtinNames().resolvePrivateName(), promiseConstructorResolveCodeGenerator, DontEnum | DontDelete | ReadOnly);
+    JSC_BUILTIN_FUNCTION(vm.propertyNames->builtinNames().rejectPrivateName(), promiseConstructorRejectCodeGenerator, DontEnum | DontDelete | ReadOnly);
+}
+
 static EncodedJSValue JSC_HOST_CALL constructPromise(ExecState* exec)
 {
     JSGlobalObject* globalObject = exec->callee()->globalObject();
index dae52d3..d849344 100644 (file)
@@ -52,6 +52,8 @@ protected:
 private:
     static ConstructType getConstructData(JSCell*, ConstructData&);
     static CallType getCallData(JSCell*, CallData&);
+
+    void addOwnInternalSlots(VM&, JSGlobalObject*);
 };
 
 } // namespace JSC
index 7d3ebb0..c5b63dc 100644 (file)
@@ -1,3 +1,37 @@
+2015-11-05  Xabier Rodriguez Calvar  <calvaris@igalia.com>
+
+        [Streams API] Shield implementation from user mangling Promise.reject and resolve methods
+        https://bugs.webkit.org/show_bug.cgi?id=150895
+
+        Reviewed by Youenn Fablet.
+
+        Replace all calls to @Promise.resolve and @Promise.reject with their internal slot counterparts. This way we
+        ensure that if the user replaces those constructor methods, our implementation still works.
+
+        Test: streams/streams-promises.html.
+
+        * Modules/streams/ReadableStream.js:
+        (initializeReadableStream):
+        (cancel):
+        * Modules/streams/ReadableStreamInternals.js:
+        (privateInitializeReadableStreamReader):
+        (cancelReadableStream):
+        (readFromReadableStreamReader):
+        * Modules/streams/ReadableStreamReader.js:
+        (cancel):
+        (read):
+        (closed):
+        * Modules/streams/StreamInternals.js:
+        (promiseInvokeOrNoop):
+        (promiseInvokeOrFallbackOrNoop):
+        * Modules/streams/WritableStream.js:
+        (initializeWritableStream):
+        (abort):
+        (close):
+        (write):
+        (closed):
+        (ready):
+
 2015-11-05  Andreas Kling  <akling@apple.com>
 
         Give ResourceUsageOverlay a stacked chart for dirty memory per category.
index 785a55d..686b356 100644 (file)
@@ -56,7 +56,7 @@ function initializeReadableStream(underlyingSource, strategy)
 
     var result = @invokeOrNoop(underlyingSource, "start", [this.@controller]);
     var _this = this;
-    @Promise.resolve(result).then(function() {
+    @Promise.@resolve(result).then(function() {
         _this.@started = true;
         @requestReadableStreamPull(_this);
     }, function(error) {
@@ -72,10 +72,10 @@ function cancel(reason)
     "use strict";
 
     if (!@isReadableStream(this))
-        return @Promise.reject(new @TypeError("Function should be called on a ReadableStream"));
+        return @Promise.@reject(new @TypeError("Function should be called on a ReadableStream"));
 
     if (@isReadableStreamLocked(this))
-        return @Promise.reject(new @TypeError("ReadableStream is locked"));
+        return @Promise.@reject(new @TypeError("ReadableStream is locked"));
 
     return @cancelReadableStream(this, reason);
 }
index f0f738b..000c0a6 100644 (file)
@@ -48,13 +48,13 @@ function privateInitializeReadableStreamReader(stream)
     if (stream.@state === @streamClosed) {
         this.@ownerReadableStream = null;
         this.@storedError = undefined;
-        this.@closedPromiseCapability = { @promise: @Promise.resolve(undefined) };
+        this.@closedPromiseCapability = { @promise: @Promise.@resolve() };
         return this;
     }
     // FIXME: ASSERT(stream.@state === @streamErrored);
     this.@ownerReadableStream = null;
     this.@storedError = stream.@storedError;
-    this.@closedPromiseCapability = { @promise: @Promise.reject(stream.@storedError) };
+    this.@closedPromiseCapability = { @promise: @Promise.@reject(stream.@storedError) };
 
     return this;
 }
@@ -275,9 +275,9 @@ function cancelReadableStream(stream, reason)
     "use strict";
 
     if (stream.@state === @streamClosed)
-        return @Promise.resolve();
+        return @Promise.@resolve();
     if (stream.@state === @streamErrored)
-        return @Promise.reject(stream.@storedError);
+        return @Promise.@reject(stream.@storedError);
     stream.@queue = @newQueue();
     @finishClosingReadableStream(stream);
     return @promiseInvokeOrNoop(stream.@underlyingSource, "cancel", [reason]).then(function() { });
@@ -354,9 +354,9 @@ function readFromReadableStreamReader(reader)
     "use strict";
 
     if (reader.@state === @streamClosed)
-        return @Promise.resolve({value: undefined, done: true});
+        return @Promise.@resolve({value: undefined, done: true});
     if (reader.@state === @streamErrored)
-        return @Promise.reject(reader.@storedError);
+        return @Promise.@reject(reader.@storedError);
     // FIXME: ASSERT(!!reader.@ownerReadableStream);
     // FIXME: ASSERT(reader.@ownerReadableStream.@state === @streamReadable);
     var stream = reader.@ownerReadableStream;
@@ -366,7 +366,7 @@ function readFromReadableStreamReader(reader)
             @requestReadableStreamPull(stream);
         else if (!stream.@queue.content.length)
             @finishClosingReadableStream(stream);
-        return @Promise.resolve({value: chunk, done: false});
+        return @Promise.@resolve({value: chunk, done: false});
     }
     var readPromiseCapability = @newPromiseCapability(@Promise);
     reader.@readRequests.push(readPromiseCapability);
index c75adb2..95d32a3 100644 (file)
@@ -30,13 +30,13 @@ function cancel(reason)
     "use strict";
 
     if (!@isReadableStreamReader(this))
-        return @Promise.reject(new @TypeError("Function should be called on a ReadableStreamReader"));
+        return @Promise.@reject(new @TypeError("Function should be called on a ReadableStreamReader"));
 
     if (this.@state === @streamClosed)
-        return @Promise.resolve();
+        return @Promise.@resolve();
 
     if (this.@state === @streamErrored)
-        return @Promise.reject(this.@storedError);
+        return @Promise.@reject(this.@storedError);
 
     // FIXME: ASSERT(@isReadableStream(this.@ownerReadableStream));
     // FIXME: ASSERT(this.@ownerReadableStream.@state === @streamReadable);
@@ -48,7 +48,7 @@ function read()
     "use strict";
 
     if (!@isReadableStreamReader(this))
-        return @Promise.reject(new @TypeError("Function should be called on a ReadableStreamReader"));
+        return @Promise.@reject(new @TypeError("Function should be called on a ReadableStreamReader"));
 
     return @readFromReadableStreamReader(this);
 }
@@ -74,7 +74,7 @@ function closed()
     "use strict";
 
     if (!@isReadableStreamReader(this))
-        return @Promise.reject(new @TypeError("Callee of closed is not a ReadableStreamReader"));
+        return @Promise.@reject(new @TypeError("Callee of closed is not a ReadableStreamReader"));
 
     return this.@closedPromiseCapability.@promise;
 }
index 0475c51..09797bb 100644 (file)
@@ -44,12 +44,12 @@ function promiseInvokeOrNoop(object, key, args)
     try {
         var method = object[key];
         if (typeof method === "undefined")
-            return @Promise.resolve();
+            return @Promise.@resolve();
         var result = method.@apply(object, args);
-        return @Promise.resolve(result);
+        return @Promise.@resolve(result);
     }
     catch(error) {
-        return @Promise.reject(error);
+        return @Promise.@reject(error);
     }
 
 }
@@ -63,10 +63,10 @@ function promiseInvokeOrFallbackOrNoop(object, key1, args1, key2, args2)
         if (typeof method === "undefined")
             return @promiseInvokeOrNoop(object, key2, args2);
         const result = method.@apply(object, args1);
-        return @Promise.resolve(result);
+        return @Promise.@resolve(result);
     }
     catch(error) {
-        return @Promise.reject(error);
+        return @Promise.@reject(error);
     }
 }
 
index 4767a9d..ad9d110 100644 (file)
@@ -43,7 +43,7 @@ function initializeWritableStream(underlyingSink, strategy)
 
     this.@underlyingSink = underlyingSink;
     this.@closedPromiseCapability = @newPromiseCapability(@Promise);
-    this.@readyPromiseCapability = { @promise: @Promise.resolve(undefined) };
+    this.@readyPromiseCapability = { @promise: @Promise.@resolve() };
     this.@queue = @newQueue();
     this.@state = @streamWritable;
     this.@started = false;
@@ -55,7 +55,7 @@ function initializeWritableStream(underlyingSink, strategy)
 
     var error = @errorWritableStream.bind(this);
     var startResult = @invokeOrNoop(underlyingSink, "start", [error]);
-    this.@startedPromise = @Promise.resolve(startResult);
+    this.@startedPromise = @Promise.@resolve(startResult);
     var _this = this;
     this.@startedPromise.then(function() {
         _this.@started = true;
@@ -70,13 +70,13 @@ function abort(reason)
     "use strict";
 
     if (!@isWritableStream(this))
-        return @Promise.reject(new @TypeError("The WritableStream.abort method can only be used on instances of WritableStream"));
+        return @Promise.@reject(new @TypeError("The WritableStream.abort method can only be used on instances of WritableStream"));
 
     if (this.@state === @streamClosed)
-        return @Promise.resolve(undefined);
+        return @Promise.@resolve();
 
     if (this.@state === @streamErrored)
-        return @Promise.reject(this.@storedError);
+        return @Promise.@reject(this.@storedError);
 
     @errorWritableStream.@apply(this, [reason]);
 
@@ -90,13 +90,13 @@ function close()
     "use strict";
 
     if (!@isWritableStream(this))
-        return @Promise.reject(new @TypeError("The WritableStream.close method can only be used on instances of WritableStream"));
+        return @Promise.@reject(new @TypeError("The WritableStream.close method can only be used on instances of WritableStream"));
 
     if (this.@state === @streamClosed || this.@state === @streamClosing)
-        return @Promise.reject(new @TypeError("Cannot close a WritableString that is closed or closing"));
+        return @Promise.@reject(new @TypeError("Cannot close a WritableString that is closed or closing"));
 
     if (this.@state === @streamErrored)
-        return @Promise.reject(this.@storedError);
+        return @Promise.@reject(this.@storedError);
 
     if (this.@state === @streamWaiting)
         this.@readyPromiseCapability.@resolve.@call(undefined, undefined);
@@ -113,13 +113,13 @@ function write(chunk)
     "use strict";
 
     if (!@isWritableStream(this))
-        return @Promise.reject(new @TypeError("The WritableStream.close method can only be used on instances of WritableStream"));
+        return @Promise.@reject(new @TypeError("The WritableStream.close method can only be used on instances of WritableStream"));
 
     if (this.@state === @streamClosed || this.@state === @streamClosing)
-        return @Promise.reject(new @TypeError("Cannot write on a WritableString that is closed or closing"));
+        return @Promise.@reject(new @TypeError("Cannot write on a WritableString that is closed or closing"));
 
     if (this.@state === @streamErrored)
-        return @Promise.reject(this.@storedError);
+        return @Promise.@reject(this.@storedError);
 
     // FIXME
     // assert(this.@state === @streamWritable || this.@state === @streamWaiting);
@@ -130,7 +130,7 @@ function write(chunk)
             chunkSize = this.@strategy.size.@call(undefined, chunk);
         } catch(e) {
             @errorWritableStream.@call(this, e);
-            return @Promise.reject(e);
+            return @Promise.@reject(e);
         }
     }
 
@@ -139,7 +139,7 @@ function write(chunk)
         @enqueueValueWithSize(this.@queue, { promiseCapability: promiseCapability, chunk: chunk }, chunkSize);
     } catch (e) {
         @errorWritableStream.@call(this, e);
-        return @Promise.reject(e);
+        return @Promise.@reject(e);
     }
 
     @syncWritableStreamStateWithQueue(this);
@@ -153,7 +153,7 @@ function closed()
     "use strict";
 
     if (!@isWritableStream(this))
-        return @Promise.reject(new @TypeError("The WritableStream.closed getter can only be used on instances of WritableStream"));
+        return @Promise.@reject(new @TypeError("The WritableStream.closed getter can only be used on instances of WritableStream"));
 
     return this.@closedPromiseCapability.@promise;
 }
@@ -163,7 +163,7 @@ function ready()
     "use strict";
 
     if (!@isWritableStream(this))
-        return @Promise.reject(new @TypeError("The WritableStream.ready getter can only be used on instances of WritableStream"));
+        return @Promise.@reject(new @TypeError("The WritableStream.ready getter can only be used on instances of WritableStream"));
 
     return this.@readyPromiseCapability.@promise;
 }