discord website is broken on canary |
|||||||||||||||
Issue descriptionChrome Version: 72.0.3625.0 (Official Build) canary (64-bit) (cohort: Clang-64) OS: Windows 10 What steps will reproduce the problem? (1) Visit discordapp.com and login (2) (3) What is the expected result? Loads What happens instead? Does not load. Gives javascript error: e34e3f36e1e3cecf9644.js:62 Uncaught RangeError: Too many arguments in function call (only 65534 allowed) at u (e34e3f36e1e3cecf9644.js:62) at Object.t.buf2string (e34e3f36e1e3cecf9644.js:62) at f.push (e34e3f36e1e3cecf9644.js:82) at t.feed (e34e3f36e1e3cecf9644.js:82) at WebSocket.<anonymous> (e34e3f36e1e3cecf9644.js:82) VM64:1 Uncaught SyntaxError: Unexpected token = in JSON at position 0 at JSON.parse (<anonymous>) at e.unpack (e34e3f36e1e3cecf9644.js:82) at t.r [as _onDataReady] (e34e3f36e1e3cecf9644.js:82) at t.handleFlushEnd (e34e3f36e1e3cecf9644.js:82) at f.push (e34e3f36e1e3cecf9644.js:82) at t.feed (e34e3f36e1e3cecf9644.js:82) at WebSocket.<anonymous> (e34e3f36e1e3cecf9644.js:82) Works on dev 72.0.3622.0 - does not work on canary 72.0.3625.0 Please use labels and text to provide additional information. If this is a regression (i.e., worked before), please consider using the bisect tool (https://www.chromium.org/developers/bisect-builds-py) to help us identify the root cause and more rapidly triage the issue. For graphics-related bugs, please copy/paste the contents of the about:gpu page at the end of this report.
,
Nov 29
Seems to be related to logging into discord with an email with a "+" sign in it e.g. discorduser+test@gmail.com. When I narrow it down to that, I can repro on beta 71.0.3578.75 and have no repro on stable 70.0.3538.110 so possibly an M71 regression, combined with changes on the discordapp.com website? Not sure what is going on here.
,
Nov 29
Do you have Adblock installed? If yes, maybe the reason is comment 43/44 in issue 804179 ?
,
Nov 29
no, have no ad blocker installed. also been testing in guest profile. this is happening on two separate machines.
,
Nov 29
Thanks for your feedback. Adding Needs-Bisect label so that the Bisect Team can try to find the regression range.
,
Nov 29
I'm seeing the same thing in other applications. Some change in 71.0.3578.75 has altered the maximum number of arguments from at least 65535 to 65534. See this JS fiddle: http://jsfiddle.net/nob30r2c/2/ It was reported to me that this was not the case in 71.0.3578.62, but I was not able to confirm. Attached are screenshots for the latest stable channel and 71.0.3578.75 from the beta channel.
,
Nov 29
Well, I can't repro on 71.0.3578.0 (Official Build) (64-bit) 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034} from a bisect, but could on beta 71.0.3578.75, so I would guess this might be linked to some experiment.
,
Nov 29
ah great, thanks for the fiddle. I will try and bisect up the 3578 branch...
,
Nov 29
,
Nov 29
You are probably looking for a change made after 610639 (known good), but no later than 610640 (first known bad). CHANGELOG URL: The script might not always return single CL as suspect as some perf builds might get missing due to failure. https://chromium.googlesource.com/chromium/src/+log/36c2c24e7ed3de8f0ee8aa18500450d2cd9f68cd..dc6d7c4a8740e10fa6612e60bcee8e7455ee90f1 https://chromium.googlesource.com/chromium/src/+/dc6d7c4a8740e10fa6612e60bcee8e7455ee90f1 v8 revision range is: https://chromium.googlesource.com/v8/v8/+log/01b0845a..500be6dd?pretty=fuller - not sure how to do a v8 bisect - CCed some people from V8 to take a look.
,
Nov 29
Assuming #7 is correct, this must mean it was merged to M71. Therefore, looking at log for M71 merges: https://chromium.googlesource.com/v8/v8/+log/7.1.302.26 and comparing with the log in #10 - the only CLs that is in both is: https://chromium.googlesource.com/v8/v8/+/8867892c50db13e0bd861d3c98b1520247a69daf "Merged: [runtime] Reduce spread/apply call max arguments" original CL: https://chromium.googlesource.com/v8/v8/+/4e3a17d0408627517d4a81b3bf5daf85e416e9ac "[runtime] Reduce spread/apply call max arguments" -> petermarshall@chromium.org
,
Nov 29
,
Nov 29
Thank you wfh@. + hablich@ (V8 TPM) & awhalley@ (Security TPM). We're planning to cut M71 stable RC tomorrow morning PT. https://chromium.googlesource.com/v8/v8/+/8867892c50db13e0bd861d3c98b1520247a69daf is a merge for security bug https://bugs.chromium.org/p/chromium/issues/detail?id=906043#c39.
,
Nov 29
This is definitely the error you will get if we spread/apply more than 65534 args - previously the limit was 125k
,
Nov 29
Can anyone provide more reliable repro instructions? Or if you can repro on discord (or another website), what is the size of the array being passed to apply/spread? The limit on Safari is 65536 (using the jsfiddle provided in #6)
,
Nov 29
The repro is reliable for me for the discord account I have but I am unable to repro for other ones, perhaps it's specific to the account or the discord server...? I do not know how to debug the javascript to work out the function parameters, but I do see error nearby in console: Uncaught SyntaxError: Unexpected token t in JSON at position 65539 so perhaps... 65539? Same page and account and server works fine with Edge 17.17134
,
Nov 29
This looks related: https://github.com/nodeca/pako/pull/150 Developer making the change is the lead backend developer for discordapp, though the function name in the change doesn't quite match the error in the report here.
,
Nov 29
that's definitely the same function as the minimized code I am seeing is the same, inside the zlib library. I don't think we can be sure that: 1. All users of pako will update to the newest version before we launch M71 2. There aren't other javascript libraries or code snippets that are relying on this behavior.
,
Nov 29
So far I wasn't able to reproduce the issue with Chrome 71.0.3578.75 on Windows10 and Mac by logging into "discordapp.com"
,
Nov 29
yes it seems specific to a particular server which perhaps has some zlib compressed assets? adding me@jh.gg
,
Nov 29
Ohhhh. I wondered how code like that would possibly work in other browsers when Safari has a limit of 65536 - you'd have to hardcode the limit above 65534 and below (or equal) 65536. Which is what pako seems to have done. I think that limits the risk a bit. But agreed - anyone using pako could be affected and would have to update very quickly.
,
Nov 29
,
Nov 29
jakedevine@gmail.com - can you confirm what 'other applications' you are seeing this in? are they all using pako, or does code like this exist in other places?
,
Nov 29
New pako release should not crash. But new restriction in Chrome 72 looks strange (i don't know details, probably you had serious reasons to do so). 65534 looks magical.
,
Nov 29
,
Nov 29
,
Nov 30
Culprit CL has been reverted from M71 here - https://bugs.chromium.org/p/chromium/issues/detail?id=906043#c49. Can this be marked as fixed now?
,
Nov 30
Sorry, revert from M71 is here - https://bugs.chromium.org/p/chromium/issues/detail?id=906043#c48
,
Nov 30
Revert has landed , this is no longer a blocker
,
Dec 3
Thanks all, marking as fixed. We've reverted this from all channels for now. This limit could come back again but it would not be merged back so there will be more time to work out any issues.
,
Dec 3
Hi, yeah, a bit late to the party here was wondering about this. Upstreamed a fix in pako to get it to work again on chrome 72. I agree the fix though is a total hack.
,
Dec 3
Able to reproduce the issue on chrome version# 71.0.3578.75(Build without fix) Verified the fix on Mac 10.12.6, Windows-10 & Ubuntu 17.10 on Chrome version #71.0.3578.80 as per the comment#6(using jsfiddle and screenshots) Attaching screenshot for reference. Observed "Able to see response in jsfiddle output" Hence, the fix is working as expected. Adding the verified label. Note: As the fix is already landed, hence removing Needs-Bisect label Thanks! |
|||||||||||||||
►
Sign in to add a comment |
|||||||||||||||
Comment 1 by wfh@chromium.org
, Nov 29