Issue 29394585: Issue 5027 - Use updated webRequest API for WebSocket blocking (Closed)
|
Created:
March 24, 2017, 1 p.m. by Jon Sonesen Modified:
April 24, 2017, 7:15 a.m. Visibility:
Public. |
DescriptionIssue 5027 - Removes CSP workaround for websockets on Chrome >= 58
Patch Set 1 #
Total comments: 6
Patch Set 2 : fixed request blocker logic and added changes to csp.js #
Total comments: 3
Patch Set 3 : fixed logic at websocket wrapper #
Total comments: 9
Patch Set 4 : adds comments, change comment to fix lint error from added indentation #
Total comments: 7
Patch Set 5 : addresses bad comments, and actually fixes requestBlocker logic #
Total comments: 8
Patch Set 6 : change comment grammer, fix line wrapping #
Total comments: 3
Patch Set 7 : change 'see' statements to be consistent #Patch Set 8 : change commit message, add chrome version to include.preload.js comments #
MessagesTotal messages: 36
https://codereview.adblockplus.org/29394585/diff/29394586/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29394585/diff/29394586/lib/requestBlocker.... lib/requestBlocker.js:158: return; This will cause connections created in shared workers to no longer be blocked due to the previous work around as discussed recently. This will probably need to be concluded before landing these changes. https://codereview.adblockplus.org/29394585/diff/29394586/metadata.chrome File metadata.chrome (right): https://codereview.adblockplus.org/29394585/diff/29394586/metadata.chrome#new... metadata.chrome:10: wss://*/* Please note, this will cause a warning on chrome versions <= 57 when loading unpacked extensions.
https://codereview.adblockplus.org/29394585/diff/29394586/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29394585/diff/29394586/lib/requestBlocker.... lib/requestBlocker.js:157: if (ext.webRequest.hasOwnProperty("RequestType.WEBSOCKET")) Does that even work? I think this should rather be: 'WEBSOCKET' in ext.webRequest.ResourceType Also, do we even need to register that listener, if there is no WebSocket support? How about this: if ('WEBSOCKET' in ext.webRequest.ResourceType) { port.on("request.websocket", (msg, sender) => { ... }); } https://codereview.adblockplus.org/29394585/diff/29394586/lib/requestBlocker.... lib/requestBlocker.js:158: return; On 2017/03/24 13:04:43, Jon Sonesen wrote: > This will cause connections created in shared workers to no longer be blocked > due to the previous work around as discussed recently. This will probably need > to be concluded before landing these changes. Is that so? I'd rather assume that the code in lib/csp.js (that should be changed as well) would cause WebSockets created in SharedWorkers to no longer being blocked. As for the general question whether this is acceptable, let's move this discussion to the issue tracker, where I just replied. https://codereview.adblockplus.org/29394585/diff/29394586/metadata.chrome File metadata.chrome (right): https://codereview.adblockplus.org/29394585/diff/29394586/metadata.chrome#new... metadata.chrome:10: wss://*/* On 2017/03/24 13:04:43, Jon Sonesen wrote: > Please note, this will cause a warning on chrome versions <= 57 when loading > unpacked extensions. For reference, we already have a similar warning (on all Chrome versions) for minimum_opera_version, and as it turned out those warnings are only shown when loading Adblock Plus as unpacked extension during development. So we already established (when we added minimum_opera_version) that this is acceptable.
One thing to mention and I will also try to disclaim this when uploading patch sets. I do not know a ton of javascript and I am still learning how to debug and test our extension, since I lack understanding in certain aspects of the extension and/or javascript it may seem that I have not tested certain things I have uploaded or written when in reality I have tested it to the best of my knowledge. Yet my knowledge may be lacking causing me to not test certain cases. I will try to be really careful about this but also please try to be understanding that I am still learning how everything works. Thanks! https://codereview.adblockplus.org/29394585/diff/29394586/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29394585/diff/29394586/lib/requestBlocker.... lib/requestBlocker.js:157: if (ext.webRequest.hasOwnProperty("RequestType.WEBSOCKET")) On 2017/03/24 17:45:02, Sebastian Noack wrote: > Does that even work? I think this should rather be: > > 'WEBSOCKET' in ext.webRequest.ResourceType > > Also, do we even need to register that listener, if there is no WebSocket > support? How about this: > > if ('WEBSOCKET' in ext.webRequest.ResourceType) > { > port.on("request.websocket", (msg, sender) => > { > ... > }); > } Thanks for pointing that out. I had tested this in a console but also, in retrospect I did not check true and false expected results so it was always giving me false but I never realized this. My apologies.
On 2017/03/26 12:38:55, Jon Sonesen wrote: > One thing to mention and I will also try to disclaim this when uploading patch > sets. I do not know a ton of javascript and I am still learning how to debug and > test our extension, since I lack understanding in certain aspects of the > extension and/or javascript it may seem that I have not tested certain things I > have uploaded or written when in reality I have tested it to the best of my > knowledge. Yet my knowledge may be lacking causing me to not test certain cases. > I will try to be really careful about this but also please try to be > understanding that I am still learning how everything works. > > Thanks! > > https://codereview.adblockplus.org/29394585/diff/29394586/lib/requestBlocker.js > File lib/requestBlocker.js (right): > > https://codereview.adblockplus.org/29394585/diff/29394586/lib/requestBlocker.... > lib/requestBlocker.js:157: if > (ext.webRequest.hasOwnProperty("RequestType.WEBSOCKET")) > On 2017/03/24 17:45:02, Sebastian Noack wrote: > > Does that even work? I think this should rather be: > > > > 'WEBSOCKET' in ext.webRequest.ResourceType > > > > Also, do we even need to register that listener, if there is no WebSocket > > support? How about this: > > > > if ('WEBSOCKET' in ext.webRequest.ResourceType) > > { > > port.on("request.websocket", (msg, sender) => > > { > > ... > > }); > > } > > Thanks for pointing that out. I had tested this in a console but also, in > retrospect I did not check true and false expected results so it was always > giving me false but I never realized this. My apologies. Oh one thing to mention is that I get an error on the background page for "ResourceType" undefined.
On 2017/03/27 06:55:38, Jon Sonesen wrote: > Oh one thing to mention is that I get an error on the background page for > "ResourceType" undefined. That is (probably) because there is actually no ext.webRequest.ResourceType, but chrome.webRequest.ResourceType.
Here is my next stab at it, to test these changes I have used the devtools to set breakpoints and ensure that my changes evaluate as expected on both Chrome 57 and 58. Previously I did not realize that the fallbacks were making it appear as though my code was working when in fact it wasn't. I will be more considerate in the future. https://codereview.adblockplus.org/29394585/diff/29395668/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29394585/diff/29395668/lib/csp.js#newcode20 lib/csp.js:20: if (!("WEBSOCKET" in chrome.webRequest.ResourceType)) I am not sure whether the imports should be in or out of this statement and do not really have an opinion.
https://codereview.adblockplus.org/29394585/diff/29395668/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29394585/diff/29395668/lib/requestBlocker.... lib/requestBlocker.js:155: if ("WEBSOCKET" in chrome.webRequest.ResourceType) It seems this check should follow the same logic as the check in lib/csp.js: if (!("WEBSOCKET" in chrome.webRequest.ResourceType)) Otherwise, it seems, we redundantly process the WebSocket connections reported by the WebSocket wrapper workaround when WebSockets are already handled by the webRequest API, and vice-versa if WebSockets cannot be blocked by the webRequest API (on Chrome <58) the WebSocket wrapper we rely on never gets a response.
https://codereview.adblockplus.org/29394585/diff/29395668/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29394585/diff/29395668/lib/requestBlocker.... lib/requestBlocker.js:155: if ("WEBSOCKET" in chrome.webRequest.ResourceType) On 2017/03/27 12:13:36, Sebastian Noack wrote: > It seems this check should follow the same logic as the check in lib/csp.js: > > if (!("WEBSOCKET" in chrome.webRequest.ResourceType)) > > Otherwise, it seems, we redundantly process the WebSocket connections reported > by the WebSocket wrapper workaround when WebSockets are already handled by the > webRequest API, and vice-versa if WebSockets cannot be blocked by the webRequest > API (on Chrome <58) the WebSocket wrapper we rely on never gets a response. Yeah, I see that was an oversight. For some reason I had that logic inverted here thinking we still wanted to wrap websocket connections even if websocket support exists and csp injection is disabled. Sorry.
LGTM
https://codereview.adblockplus.org/29394585/diff/29395673/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29394585/diff/29395673/lib/csp.js#newcode20 lib/csp.js:20: if (!("WEBSOCKET" in chrome.webRequest.ResourceType)) Mind adding a comment to explain why you're making this check? Include something like "...versions of Chrome before 58..." so that it's clear when the code can be removed in the future. https://codereview.adblockplus.org/29394585/diff/29395673/lib/csp.js#newcode43 lib/csp.js:43: // not inherited from the parent for documents with data: and blob: URLs. Please run ESLint on your changes to catch over-long lines like this. (Yes until my changes land for that you'll have to diff the output before and after your changes to see what new errors you've caused.) https://codereview.adblockplus.org/29394585/diff/29395673/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29394585/diff/29395673/lib/requestBlocker.... lib/requestBlocker.js:155: if (!("WEBSOCKET" in chrome.webRequest.ResourceType)) Again please leave a comment here explaining that why this code is necessary for versions of Chrome before 58. Ideally link the Chromium issue too. That way when we increase the minimum Chrome version in the future it will be obvious that we can remove this code. https://codereview.adblockplus.org/29394585/diff/29395673/metadata.chrome File metadata.chrome (right): https://codereview.adblockplus.org/29394585/diff/29395673/metadata.chrome#new... metadata.chrome:9: ws://*/* I wonder if adding new permissions is OK, I don't know how it works. Is the user prompted about it, or is the extension disabled, or does something else happen?
https://codereview.adblockplus.org/29394585/diff/29395673/metadata.chrome File metadata.chrome (right): https://codereview.adblockplus.org/29394585/diff/29395673/metadata.chrome#new... metadata.chrome:9: ws://*/* On 2017/03/28 05:38:13, kzar wrote: > I wonder if adding new permissions is OK, I don't know how it works. Is the user > prompted about it, or is the extension disabled, or does something else happen? There are only certain permissions changes, that needs to be confirmed by the user. According to the documentation, this doesn't seem to be any of them: https://developer.chrome.com/extensions/permission_warnings But we probably should double check, therefore however, we first need to land this change, and then watch what's going to happen when the devbuild is updated from the Chrome Web Store.
On 2017/03/28 05:58:19, Sebastian Noack wrote: > But we probably should double check, therefore however, we first need to land > this change, and then watch what's going to happen when the devbuild is updated > from the Chrome Web Store. Yea, mind adding a paragraph to the "How to test" section of the issue Jon? We should mention that the permissions are being added and that they need to test how upgrades work.
On 2017/03/28 06:20:22, kzar wrote: > On 2017/03/28 05:58:19, Sebastian Noack wrote: > > But we probably should double check, therefore however, we first need to land > > this change, and then watch what's going to happen when the devbuild is > updated > > from the Chrome Web Store. > > Yea, mind adding a paragraph to the "How to test" section of the issue Jon? We > should mention that the permissions are being added and that they need to test > how upgrades work. Yeah I will update the ticket. Also will be uploading another patch set in a bit, I am trying to configure syntastic to use our eslint conf
https://codereview.adblockplus.org/29394585/diff/29395673/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29394585/diff/29395673/lib/csp.js#newcode20 lib/csp.js:20: if (!("WEBSOCKET" in chrome.webRequest.ResourceType)) On 2017/03/28 05:38:13, kzar wrote: > Mind adding a comment to explain why you're making this check? Include something > like "...versions of Chrome before 58..." so that it's clear when the code can > be removed in the future. Acknowledged. https://codereview.adblockplus.org/29394585/diff/29395673/lib/csp.js#newcode43 lib/csp.js:43: // not inherited from the parent for documents with data: and blob: URLs. On 2017/03/28 05:38:13, kzar wrote: > Please run ESLint on your changes to catch over-long lines like this. (Yes until > my changes land for that you'll have to diff the output before and after your > changes to see what new errors you've caused.) No problem, additionally what is the opinion on this line? It goes over only two characters...do you think it is aceptable to just remove the colons in 'blob:' and 'data:'? https://codereview.adblockplus.org/29394585/diff/29395673/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29394585/diff/29395673/lib/requestBlocker.... lib/requestBlocker.js:155: if (!("WEBSOCKET" in chrome.webRequest.ResourceType)) On 2017/03/28 05:38:13, kzar wrote: > Again please leave a comment here explaining that why this code is necessary for > versions of Chrome before 58. Ideally link the Chromium issue too. > > That way when we increase the minimum Chrome version in the future it will be > obvious that we can remove this code. No problem
https://codereview.adblockplus.org/29394585/diff/29395673/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29394585/diff/29395673/lib/csp.js#newcode43 lib/csp.js:43: // not inherited from the parent for documents with data: and blob: URLs. On 2017/03/28 13:03:14, Jon Sonesen wrote: > On 2017/03/28 05:38:13, kzar wrote: > > Please run ESLint on your changes to catch over-long lines like this. (Yes > until > > my changes land for that you'll have to diff the output before and after your > > changes to see what new errors you've caused.) > > No problem, additionally what is the opinion on this line? It goes over only two > characters...do you think it is aceptable to just remove the colons in 'blob:' > and 'data:'? Without the colons, I find this rather confusing. The colon indicates that we are talking about a known/standard URL scheme. Without them you have to think more when deriving the context. Also the line above isn' any shorter, so this alone wouldn't be sufficient anyway. If you don't want to rewrap this paragraph for some reason, another options would be going with "frame-src/object-src" and "data:/blob:", not sure if this is any better though. https://codereview.adblockplus.org/29394585/diff/29396613/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29394585/diff/29396613/lib/requestBlocker.... lib/requestBlocker.js:156: // Versions of Chrome < 58 do not support WebSockets since the release of On Chrome <58, the webRequest API doesn't intercept WebSocket connections, see https://crbug.com/129353. Hence we patched the WebSocket object as a workaround. WebSocket connections detected by this workaround are handled below. https://codereview.adblockplus.org/29394585/diff/29396613/lib/requestBlocker.... lib/requestBlocker.js:159: // SharedWorkers will not be blocked by this workaround being disabled a WebSocket connections opened by SharedWorkers are not blocked, due to https://crbug.com/705931. On Chrome <58, however, those are covered by the workaround in lib/csp.js.
https://codereview.adblockplus.org/29394585/diff/29396613/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29394585/diff/29396613/lib/csp.js#newcode20 lib/csp.js:20: // Versions of Chrome < 58 do not support WebSockets since the release of On Chrome 58, the webRequest API doesn't intercept WebSocket connections, see https://crbug.com/129353. Hence, we inject CSP headers, below, as workaround. https://codereview.adblockplus.org/29394585/diff/29396613/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29394585/diff/29396613/lib/requestBlocker.... lib/requestBlocker.js:159: // SharedWorkers will not be blocked by this workaround being disabled a On 2017/03/28 13:48:08, Sebastian Noack wrote: > WebSocket connections opened by SharedWorkers are not blocked, due to > https://crbug.com/705931. On Chrome <58, however, those are covered by the > workaround in lib/csp.js. Nevermind. This paragraph should rather just go away. The new Chrome bug you filed is off-topic, and pointing it out here is rather confusing.
Sorry for the oversights on the comments, also realized I never even changed the logic in requestBlocker.js :/ https://codereview.adblockplus.org/29394585/diff/29396613/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29394585/diff/29396613/lib/csp.js#newcode20 lib/csp.js:20: // Versions of Chrome < 58 do not support WebSockets since the release of On 2017/03/28 14:33:44, Sebastian Noack wrote: > On Chrome 58, the webRequest API doesn't intercept WebSocket connections, see > https://crbug.com/129353. Hence, we inject CSP headers, below, as workaround. Acknowledged. https://codereview.adblockplus.org/29394585/diff/29396613/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29394585/diff/29396613/lib/requestBlocker.... lib/requestBlocker.js:156: // Versions of Chrome < 58 do not support WebSockets since the release of On 2017/03/28 13:48:08, Sebastian Noack wrote: > On Chrome <58, the webRequest API doesn't intercept WebSocket connections, see > https://crbug.com/129353. Hence we patched the WebSocket object as a workaround. > WebSocket connections detected by this workaround are handled below. Sorry, thank you for clarifying the comments here. https://codereview.adblockplus.org/29394585/diff/29396613/lib/requestBlocker.... lib/requestBlocker.js:159: // SharedWorkers will not be blocked by this workaround being disabled a On 2017/03/28 14:33:44, Sebastian Noack wrote: > On 2017/03/28 13:48:08, Sebastian Noack wrote: > > WebSocket connections opened by SharedWorkers are not blocked, due to > > https://crbug.com/705931. On Chrome <58, however, those are covered by the > > workaround in lib/csp.js. > > Nevermind. This paragraph should rather just go away. The new Chrome bug you > filed is off-topic, and pointing it out here is rather confusing. Acknowledged.
https://codereview.adblockplus.org/29394585/diff/29396638/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29394585/diff/29396638/lib/csp.js#newcode20 lib/csp.js:20: // Versions of Chrome < 58 the webRequest API does not intercept WebSocket This seems grammatically wrong. This should rather be: On versions <58 of Chrome, ... Or I'd prefer just: On Chrome 58, ... https://codereview.adblockplus.org/29394585/diff/29396638/lib/csp.js#newcode23 lib/csp.js:23: // Hence we inject CSP headers, below, as a workaround. IMHO, this shouldn't be a new paragraph, as we even refer to the previous sentence with "hence". https://codereview.adblockplus.org/29394585/diff/29396638/lib/csp.js#newcode46 lib/csp.js:46: // We also need the frame-src and object-src restrictions since CSPs are This line still seems too long. https://codereview.adblockplus.org/29394585/diff/29396638/lib/csp.js#newcode47 lib/csp.js:47: // not inherited from the parent for documents with data and blob URLs. As I explained before, please don't remove the colons just to avoid wrapping.
Patch Set 6 Hopefully this is better, noticed colon which seems wrong in the CSP comment which I noted. Thanks https://codereview.adblockplus.org/29394585/diff/29396638/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29394585/diff/29396638/lib/csp.js#newcode20 lib/csp.js:20: // Versions of Chrome < 58 the webRequest API does not intercept WebSocket On 2017/03/28 19:32:29, Sebastian Noack wrote: > This seems grammatically wrong. This should rather be: > > On versions <58 of Chrome, ... > > Or I'd prefer just: > > On Chrome 58, ... Done. https://codereview.adblockplus.org/29394585/diff/29396638/lib/csp.js#newcode23 lib/csp.js:23: // Hence we inject CSP headers, below, as a workaround. On 2017/03/28 19:32:29, Sebastian Noack wrote: > IMHO, this shouldn't be a new paragraph, as we even refer to the previous > sentence with "hence". Done. https://codereview.adblockplus.org/29394585/diff/29396638/lib/csp.js#newcode46 lib/csp.js:46: // We also need the frame-src and object-src restrictions since CSPs are On 2017/03/28 19:32:29, Sebastian Noack wrote: > This line still seems too long. Done. https://codereview.adblockplus.org/29394585/diff/29396638/lib/csp.js#newcode47 lib/csp.js:47: // not inherited from the parent for documents with data and blob URLs. On 2017/03/28 19:32:29, Sebastian Noack wrote: > As I explained before, please don't remove the colons just to avoid wrapping. Done. https://codereview.adblockplus.org/29394585/diff/29396650/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29394585/diff/29396650/lib/csp.js#newcode21 lib/csp.js:21: // connections see: https://crbug.com/129353. Hence we inject CSP headers, Perhaps I should remove the colon.
https://codereview.adblockplus.org/29394585/diff/29396650/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29394585/diff/29396650/lib/csp.js#newcode21 lib/csp.js:21: // connections see: https://crbug.com/129353. Hence we inject CSP headers, On 2017/03/28 20:10:31, Jon Sonesen wrote: > Perhaps I should remove the colon. I wouldn't insist, but personally I'd omit the colon, but perhaps add parentheses: (see https://crbug.com/129353) You can also omit the parantheses, but then there should be a comma, before "see". It seems this is how you did it in lib/contentBlocker.js. Either way I would do it consistent.
Patch Set 7 I updated the comments to have consistent chromium links https://codereview.adblockplus.org/29394585/diff/29396650/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29394585/diff/29396650/lib/csp.js#newcode21 lib/csp.js:21: // connections see: https://crbug.com/129353. Hence we inject CSP headers, On 2017/03/28 20:20:43, Sebastian Noack wrote: > On 2017/03/28 20:10:31, Jon Sonesen wrote: > > Perhaps I should remove the colon. > > I wouldn't insist, but personally I'd omit the colon, but perhaps add > parentheses: > > (see https://crbug.com/129353) > > You can also omit the parantheses, but then there should be a comma, before > "see". It seems this is how you did it in lib/contentBlocker.js. Either way I > would do it consistent. I prefer the way the parentheses look too, so I have added then
LGTM
> Issue 5027 - Removes CSP workaround for websockets on Chrome >= 58 The commit message (review title) isn't too accurate: 1. We're not actually removing the CSP workaround yet. 2. The injected WebSocket wrapper is also involved. IMO the important part is that we've begun using the webRequest API for WebSockets. How about this? Issue 5027 - Use the updated webRequest API for WebSocket blocking
What about the WebSocket wrapper in include.preload.js? That is the code that sends the "request.websocket" message to lib/requestBlocker.js. I see you've avoided listening for that message for newer versions of Chrome, but you'll still be wrapping WebSocket and sending those messages so far right?
On 2017/03/29 01:04:09, kzar wrote: > > Issue 5027 - Removes CSP workaround for websockets on Chrome >= 58 > > The commit message (review title) isn't too accurate: > > 1. We're not actually removing the CSP workaround yet. > 2. The injected WebSocket wrapper is also involved. > > IMO the important part is that we've begun using the webRequest API for > WebSockets. How about this? > > Issue 5027 - Use the updated webRequest API for WebSocket blocking Thanks for pointing that out I will update that.
On 2017/03/29 01:07:31, kzar wrote: > What about the WebSocket wrapper in include.preload.js? That is the code that > sends the "request.websocket" message to lib/requestBlocker.js. I see you've > avoided listening for that message for newer versions of Chrome, but you'll > still be wrapping WebSocket and sending those messages so far right? Yeah, it looks like we still wrap them there. I can add logic to avoid wrapping if websocket intercepts are supported.
On 2017/03/29 06:29:36, Jon Sonesen wrote: > On 2017/03/29 01:07:31, kzar wrote: > > What about the WebSocket wrapper in include.preload.js? That is the code that > > sends the "request.websocket" message to lib/requestBlocker.js. I see you've > > avoided listening for that message for newer versions of Chrome, but you'll > > still be wrapping WebSocket and sending those messages so far right? > > Yeah, it looks like we still wrap them there. I can add logic to avoid wrapping > if websocket intercepts are supported. It's not possible to do feature detection in the content script, as chrome.webRequest is a background-only API. Neither can we use asynchronous messaging to ask the background page to check chrome.webRequest.ResourceType, because as we wait for a response the web page might already use the WebSocket API, or even backup the original object before we get to wrap it. So we'd be down checking navigator.userAgent, which is a bad idea since the user can change the user agent string. Also considering Firefox (which does support WebSockets), Microsoft Edge (which doesn't) and Opera (which does in the respective version) it is unnecessary complicated to get a check based on the user agent string right. That is why I suggested in the first place (as specified in the issue), to ignore WebSocket connections reported by the WebSocket wrapper, rather than disabling the WebSocket wrapper which doesn't seem possible in a reliable way as long as we till rely on it for older Chrome versions.
Jon wrote: > Thanks for pointing that out I will update that. Cool I'll wait for the review to be renamed. On 2017/03/29 07:09:23, Sebastian Noack wrote: > That is why I suggested in the first place (as specified in the issue), to > ignore WebSocket connections reported by the WebSocket wrapper, rather than > disabling the WebSocket wrapper which doesn't seem possible in a reliable way as > long as we till rely on it for older Chrome versions. Fair enough, I guess leaving the wrapper in place is OK. It's pretty much solid at this point.
On 2017/03/29 09:59:04, kzar wrote: > Fair enough, I guess leaving the wrapper in place is OK. It's pretty much solid > at this point. Oh, Jon you should probably add a code comment above the wrapper that explains why it's there and when it can be removed. Same as the other comments you've added I guess.
On 2017/03/29 09:59:57, kzar wrote: > On 2017/03/29 09:59:04, kzar wrote: > > Fair enough, I guess leaving the wrapper in place is OK. It's pretty much > solid > > at this point. > > Oh, Jon you should probably add a code comment above the wrapper that explains > why it's there and when it can be removed. Same as the other comments you've > added I guess. There is also a comment for this purpose in include.preload.js: // Chrome doesn't allow us to intercept WebSockets[1], and therefore // some ad networks are misusing them as a way to serve adverts and circumvent // us. As a workaround we wrap WebSocket, preventing blocked WebSocket // connections from being opened. // [1] - https://bugs.chromium.org/p/chromium/issues/detail?id=129353 However, we can mention the specific Chrome version there now.
On 2017/03/29 10:02:47, Sebastian Noack wrote: > There is also a comment for this purpose in include.preload.js: s/also/already/
On 2017/03/29 10:02:47, Sebastian Noack wrote: > On 2017/03/29 09:59:57, kzar wrote: > > On 2017/03/29 09:59:04, kzar wrote: > > > Fair enough, I guess leaving the wrapper in place is OK. It's pretty much > > solid > > > at this point. > > > > Oh, Jon you should probably add a code comment above the wrapper that explains > > why it's there and when it can be removed. Same as the other comments you've > > added I guess. > > There is also a comment for this purpose in include.preload.js: > > // Chrome doesn't allow us to intercept WebSockets[1], and therefore > // some ad networks are misusing them as a way to serve adverts and circumvent > // us. As a workaround we wrap WebSocket, preventing blocked WebSocket > // connections from being opened. > // [1] - https://bugs.chromium.org/p/chromium/issues/detail?id=129353 > > However, we can mention the specific Chrome version there now. Will update the comment to include the chrome version and also change the commit message/review title
LGTM
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
