Bug 150934 – [Streams API] Shield implementation from mangling then and catch promise methods

• Component: New Bugs
• Status: RESOLVED FIXED

[Xabier Rodríguez Calvar]

[Streams API] Shield implementation from mangling then and catch promise methods

[Xabier Rodríguez Calvar]

Created attachment 264866 [details]
Patch

[Xabier Rodríguez Calvar]

Created attachment 264931 [details]
Patch

Added another test for Object.setPrototypeOf. The test was already working before but it might be interesting to keep it just in case.

[youenn fablet]

Comment on attachment 264931 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=264931&action=review

> LayoutTests/streams/streams-promises.html:60
> +        Object.setPrototypeOf(promise, { then: function() { assert_unreached("streams should not use this promise then method"); } });

I am surprised the test is working before the patch.
Can you try:  Object.setPrototypeOf(promise, { constructor: Promise, then: ... });
It may be the case that Promise.resolve is wrapping the "promise" object because its constructor is not Promise.

[youenn fablet]

Comment on attachment 264931 [details]
Patch

Additional comments.

View in context: https://bugs.webkit.org/attachment.cgi?id=264931&action=review

> LayoutTests/streams/streams-promises.html:65
> +}, 'Streams implementation is not affected if an attempt to replace promise prototype is made');

The two tests have the same name.
Might be good to precise it here.

> LayoutTests/streams/streams-promises.html:87
> +        rs.tee();

Are we sure closed.catch is actually triggered?
We should probably beef up the test here so that we ensure this in some way.

> LayoutTests/streams/streams-promises.html:101
> +}, 'Streams implementation is not affected if promise then method is replaced');

Same name as above test.
May be good to precise it.

[Xabier Rodríguez Calvar]

Created attachment 265040 [details]
Patch

[youenn fablet]

Comment on attachment 265040 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=265040&action=review

r=me

Test names look much better to me. Some small suggestions below.

> LayoutTests/streams/streams-promises-expected.txt:5
> +PASS Streams and promises: attempt to replace Promise prototype 

Not sure we should keep this test

> LayoutTests/streams/streams-promises-expected.txt:6
> +FAIL Streams and promises: replace a single promise prototype undefined is not a function

replace prototype of a Promise object

> LayoutTests/streams/streams-promises-expected.txt:7
> +PASS Streams and promises: replace then prototype method 

replace then function in Promise prototype

> LayoutTests/streams/streams-promises-expected.txt:8
> +PASS Streams and promises: replace catch prototype method 

replace catch function in Promise prototype

> LayoutTests/streams/streams-promises-expected.txt:9
> +PASS Streams and promises: replace then method in a single promise 

replace then function in Promise object
Also, should we have a test for "replace catch function in Promise object"?

[Xabier Rodríguez Calvar]

Created attachment 265046 [details]
Patch for landing

[Xabier Rodríguez Calvar]

(In reply to comment #6)
> > LayoutTests/streams/streams-promises-expected.txt:5
> > +PASS Streams and promises: attempt to replace Promise prototype 
> 
> Not sure we should keep this test

Me neither, removed.

 
> replace prototype of a Promise object
> 
> replace then function in Promise prototype
> 
> replace catch function in Promise prototype
> 
> replace then function in Promise object

Changed all this names too, used method instead of function to be more OO.

> Also, should we have a test for "replace catch function in Promise object"?

Currently we don't need it because the only place were we use catch is in tee, with a promise that never leaves the code so currently there is not way to test it either.

[WebKit Commit Bot]

Comment on attachment 265046 [details]
Patch for landing

Clearing flags on attachment: 265046

Committed r192157: <http://trac.webkit.org/changeset/192157>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.