Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crash caused by odd escaping in response from http://localhost:8888/lab request #7024

Closed
telamonian opened this issue Aug 15, 2019 · 10 comments · Fixed by #7016
Closed

Crash caused by odd escaping in response from http://localhost:8888/lab request #7024

telamonian opened this issue Aug 15, 2019 · 10 comments · Fixed by #7016

Comments

@telamonian
Copy link
Member

@telamonian telamonian commented Aug 15, 2019

This may be related to #7016, #7023, and other recent changes/discussion about escaping.

After pulling the latest Jlab code from master, I found that I was unable to load the Jlab client. In my browser's console, I see the following error:

VM104:7 Uncaught SyntaxError: Unexpected token & in JSON at position 222
    at JSON.parse (<anonymous>)
    at Object.getOption (pageconfig.js:44)
    at Module.ANye (index.out.js:13)
    at __webpack_require__ (bootstrap:84)
    at Object.0 (bootstrap:221)
    at __webpack_require__ (bootstrap:84)
    at checkDeferredModules (bootstrap:45)
    at Array.webpackJsonpCallback [as push] (bootstrap:32)
    at vendors~main.36831d27f332f6d18628.js:1

This is the relevant bit of pageconfig.js:

            if (el) {
                configData = JSON.parse(el.textContent || '');
                found = true;
            }

If I debug and inspect el.textContents, here's what I see (with a few fields nulled for privacy):

"{
    "quitButton": "True",
    "buildAvailable": "True",
    "buildCheck": "True",
    "devMode": "False",
    "token": null,
    "notebookVersion": "[7, 0, 0, \\\\\\\\\\\\\\\&#34;.dev0\\\\\\\\\\\\\\\&#34;]",
    "terminalsAvailable": "True",
    "ignorePlugins": "[]",
    "serverRoot": null
    "store_id": "2",
    "mathjaxConfig": "TeX-AMS-MML_HTMLorMML-full,Safe",
    "fullMathjaxUrl": "/static/components/MathJax/MathJax.js",
    "appName": "JupyterLab",
    "appNamespace": "jupyterlab",
    "appSettingsDir": "/usr/local/share/jupyter/lab/settings",
    "appUrl": "/lab",
    "appVersion": "1.0.4",
    "cacheFiles": "True",
    "schemasDir": "/usr/local/share/jupyter/lab/schemas",
    "settingsUrl": "/lab/api/settings",
    "staticDir": "/usr/local/share/jupyter/lab/static",
    "staticUrl": "/static/lab",
    "templatesDir": "/usr/local/share/jupyter/lab/static",
    "themesDir": "/usr/local/share/jupyter/lab/themes",
    "themesUrl": "/lab/api/themes",
    "treeUrl": "/lab/tree",
    "userSettingsDir": null,
    "workspacesApiUrl": "/lab/api/workspaces",
    "workspacesDir": null,
    "workspacesUrl": "/lab/workspaces",
    "fullAppUrl": "/lab",
    "fullSettingsUrl": "/lab/api/settings",
    "fullStaticUrl": "/static/lab",
    "fullThemesUrl": "/lab/api/themes",
    "fullTreeUrl": "/lab/tree",
    "fullWorkspacesApiUrl": "/lab/api/workspaces",
    "fullWorkspacesUrl": "/lab/workspaces",
    "disabledExtensions": "[]",
    "baseUrl": "/",
    "wsUrl": ""
  }"

Clearly the issue is the parsing/escaping of my version of the notebook package. The above string originates from the response to the http://localhost:8888/lab request that is made during Jlab startup.

I am using a dev install of notebook, and I can get Jlab to run if I replace it with the release version of notebook, which confirms that the odd escaping of notebook is the cause of my problem.

@telamonian
Copy link
Member Author

@telamonian telamonian commented Aug 15, 2019

Hmmm, this may actually be an issue with jupyterlab_server. I had to upgrade to the latest server release (1.0.2) in order to do a clean build (including pip install -e .) of the latest Jlab code.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 15, 2019

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 15, 2019

Does it work with #7016?

@telamonian
Copy link
Member Author

@telamonian telamonian commented Aug 16, 2019

Does it work with #7016?

Lemme check

@telamonian
Copy link
Member Author

@telamonian telamonian commented Aug 16, 2019

@jasongrout #7016 does fix the crash, but the escaping of notebook version is still kinda funky:

"notebookVersion": "[7, 0, 0, \\\\\\\\\\\\\\\".dev0\\\\\\\\\\\\\\\"]"

Working backwards, it looks like the version string is getting escaped 4 separate times. For each quote, on the first escape you'd get:

\"

then:

\\\"

and so forth.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 16, 2019

This looks rather suspicious. Perhaps it will work well now that we properly escape things?

# Export the version info tuple to a JSON array. This gets printed
# inside double quote marks, so we render it to a JSON string of the
# JSON data (so that we can call JSON.parse on the frontend on it).
# We also have to wrap it in `Markup` so that it isn't escaped
# by Jinja. Otherwise, if the version has string parts these will be
# escaped and then will have to be unescaped on the frontend.
page_config['notebookVersion'] = Markup(dumps(dumps(version_info))[1:-1])

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 16, 2019

I pushed a simple fix to #7016 (commit 9538853). Can you check to see if it fixes the escaping issue you are seeing?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 16, 2019

I just tested my fix in #7016, and (when I modify the notebook version manually), I get something like "notebookVersion": [5, 7, 8, "dev0"] in the page config script tag. So I think that fixes it.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 16, 2019

Actually, it looks like client-side code assumes that notebook version is a string, so I changed it back to a string for backwards compatibility, so it can be backported easily to 1.0.x.

I think we can put in a separate PR for 1.1 only that changes the page config value to a JSON array, rather than a string encoding of a JSON array. See the todo note in my amended commit f655977.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 16, 2019

So now the page config entry is "notebookVersion": "[5, 7, 8, \"dev0\"]"

@lock lock bot locked as resolved and limited conversation to collaborators Sep 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants