* SUMMARY Looking over the different WebCore builtins, it seems there are a few builtins that can use user overridable code. <script> <!-- Fetch --> Array.prototype.forEach = function() { throw "User overriden"; }; new Headers({a:1}) // throws exception </script> <script> <!-- Streams --> Object.defineProperty = function() { throw "User overriden"; }; new ByteLengthQueuingStrategy({}) // throws exception </script> <script> <!-- Streams --> Object.defineProperty = function() { throw "User overriden"; }; new CountQueuingStrategy({}) // throws exception </script> Not really sure how we can catch this when JSC first parses these scripts. For constructor functions, it seems like it might be possible (Object.defineProperty => @Object.@defineProperty) but for native prototype methods (an overridden Array.prototype.forEach) I'm not sure how feasible that is. Likewise, a catching this at build time, or scanning the source with RegExps would be brittle.
Created attachment 274981 [details] [PATCH] WIP - Needs Tests Haven't yet written tests. Still have to decide where they should go. Its nice that all the test coverage for this code is in imported w3c tests directories, but these kind of tests seem like they would be ridiculous to have as part of w3c. So it'll take a bunch of boilerplate to setup some LayoutTests directories and stuff for this which I'll get around to doing in a bit.
(In reply to comment #1) > Created attachment 274981 [details] > [PATCH] WIP - Needs Tests > > Haven't yet written tests. Still have to decide where they should go. Its > nice that all the test coverage for this code is in imported w3c tests > directories, but these kind of tests seem like they would be ridiculous to > have as part of w3c. So it'll take a bunch of boilerplate to setup some > LayoutTests directories and stuff for this which I'll get around to doing in > a bit. Thanks for taking care of that issue. In the same vein, it might be good to check WebRTC use of JS built-ins. IIRC, some of WebRTC built-ins are/were using Promise.then directly. As of tests, imported/w3c might not be a very good place since these shielding-tests are really WebKit specific. There are already some shielding-tests in LayoutTests/streams. For fetch, it might make sense to create a new folder.
I wonder if we could write a rule for this. If the base is private will we ever access a public property on it? I.e can we assume that all accesses on @Object should be private? And apply the rule recursively through the access chain? Or do we have places where we really do need a private base and public property?
(In reply to comment #3) > I wonder if we could write a rule for this. > If the base is private will we ever access a > public property on it? > I.e can we assume that all accesses on @Object should > be private? And apply the rule recursively through the > access chain? > > Or do we have places where we really do need a private base > and public property? I think we can assume that all accesses to any @Object-like objects should be strictly private and protected. And IIRC, in the Streams API code we don't access any public property, all code is based on internal slots.
(In reply to comment #3) > I wonder if we could write a rule for this. > If the base is private will we ever access a > public property on it? There are instances of @RegExp.prototype in JavaScriptCore/builtins and @Promise.prototype in WebCore/streams builtins. I'm not sure how safe that is.
> Thanks for taking care of that issue. > In the same vein, it might be good to check WebRTC use of JS built-ins. > IIRC, some of WebRTC built-ins are/were using Promise.then directly. I believe that is actually intentional. Promise chaining works with any Promise shaped object (JS libraries that create their own promises and native Promises). As long as the value returned from a promise has a "then" function, it is treated like a promise and chained.
(In reply to comment #6) > > Thanks for taking care of that issue. > > In the same vein, it might be good to check WebRTC use of JS built-ins. > > IIRC, some of WebRTC built-ins are/were using Promise.then directly. > > I believe that is actually intentional. Promise chaining works with any > Promise shaped object (JS libraries that create their own promises and > native Promises). As long as the value returned from a promise has a "then" > function, it is treated like a promise and chained. There are cases where accessing public properties is fine, like in Promise.catch. In the case of WebRTC, native promises are used internally, which might make the situation a bit different. If a user script changes the "then" property of the Promise prototype, some WebRTC code may break, like in RTCPeerConnection.js: """ peerConnection.@queuedAddIceCandidate(candidate).then(...) """
Created attachment 275061 [details] [PATCH] Proposed Fix
Created attachment 275062 [details] [PATCH] Proposed Fix
> In the case of WebRTC, native promises are used internally, which might make > the situation a bit different. > > If a user script changes the "then" property of the Promise prototype, some > WebRTC code may break, like in RTCPeerConnection.js: > """ > peerConnection.@queuedAddIceCandidate(candidate).then(...) > """ Good catch. I had just expected that this was intentional. Are you able to test this? Should we file another bug?
Comment on attachment 275062 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=275062&action=review Looks good to me. Some suggestions below that could be done as a follow-up > LayoutTests/ChangeLog:11 > + * streams/builtin-overrides.html: Added. We might need to add more tests like these two for fetch or streams API. These two test filenames should probably be made more specific at some point. For streams, the test filename is called streams-promises.html, which is probably not specific enough. For getUserMedia, the test filename is named webkitGetUserMedia-shadowing-then.html, I quite like it. > Source/WebCore/Modules/fetch/FetchHeaders.js:-54 > - @Object.@getOwnPropertyNames(headersInit).forEach((name) => { forEach is also called in line 39 on a Headers object. This case should probably be handled as well.
(In reply to comment #10) > > In the case of WebRTC, native promises are used internally, which might make > > the situation a bit different. > > > > If a user script changes the "then" property of the Promise prototype, some > > WebRTC code may break, like in RTCPeerConnection.js: > > """ > > peerConnection.@queuedAddIceCandidate(candidate).then(...) > > """ > > Good catch. I had just expected that this was intentional. Are you able to > test this? Should we file another bug? Sure, I will file a bug for it. One case to study in details is when chaining then happens, like in RTCPeerConnectionInternals.js/enqueueOperation.
> Sure, I will file a bug for it. > One case to study in details is when chaining then happens, like in > RTCPeerConnectionInternals.js/enqueueOperation. Filed bug 155964.
Comment on attachment 275062 [details] [PATCH] Proposed Fix Clearing flags on attachment: 275062 Committed r198776: <http://trac.webkit.org/changeset/198776>
All reviewed patches have been landed. Closing bug.
(In reply to comment #5) > (In reply to comment #3) > > I wonder if we could write a rule for this. > > If the base is private will we ever access a > > public property on it? > > There are instances of @RegExp.prototype in JavaScriptCore/builtins and > @Promise.prototype in WebCore/streams builtins. I'm not sure how safe that > is. The "prototype" property of @Promise and @RegExp are DontDelete and ReadOnly.
> > There are instances of @RegExp.prototype in JavaScriptCore/builtins and > > @Promise.prototype in WebCore/streams builtins. I'm not sure how safe that > > is. > > The "prototype" property of @Promise and @RegExp are DontDelete and ReadOnly. Thanks for checking that!