[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 b559f2210a9daee6863f6db7397f308f22f0e14e..2d4597a91d23b1aadfb0904dc8cdac8623195a2d 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 d5dc4b9dbe170ed783070bcbb2403fb6b2be96d0..204aaea248cbf6f156dc90b8bd5b2abad816b40d 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 7394c9be517d394e782e4e35278fa7d56122be0f..e559b1b215ae881145ae0ef0a43a9b020d247ba0 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 9786bf4007248fd2e939f841f09ba5fe0b284689..4f222ca41def916f1db640d6bbdef0e76585a989 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 a9ba1c592670ce1f65512452cbc3830f79100adf..097054d01820fb38a3a72466badc6a912b96b501 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 dae52d3a8cf50bd2d62972f16b72567bfd0c35d7..d84934497d5b5383f0761170f18020fc7207e681 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 7d3ebb0515702571d0f1cc2fcb9383c69f9ef277..c5b63dc59cb39bcdb40ae8c453eb841c5499ce4d 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 785a55d1edc7f9271ebc5357b884d17c354b17a4..686b3565c97621c5d0e97c8eef58088ecc8c8960 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 f0f738b9ee455aedd4727544653e498f54f3a4f3..000c0a6d2ea531594fe6249adec59ebb9fefb8e6 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 c75adb2aa3810eed5da09244fe12b0ac785bb46d..95d32a3c5a7e63eedea95281737e97121850036b 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 0475c510e7f0dd2e95088fb45c657ab8b5a06ec2..09797bb092b0f2bd0b1d0730ef7d3901ab661294 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 4767a9d381319a6aaec48fcdf543e7af4d3024dc..ad9d110212e5add1ccc21ee60cfc5c2e1eb48d0c 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;
 }