-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
windows: explicitly register correct MIME types #3128
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary: The standard library’s `mimetypes.guess_types` can give incorrect results on Windows, because arbitrary programs can influence its behavior by writing to the registry. This can be worked around by reinstating the correct MIME types with `mimetypes.add_type`, as in this patch. Fixes #3120. Test Plan: To check that this patch works at all on Windows, edited the registry to add a bad content type: ``` reg add HKCR\.js /v "Content Type" /d application/x-testing-123 ``` …then verified that before this change, `mimetypes.guess_type("foo.js")` gives the wrong value and TensorBoard does not render properly (strict MIME type checking error in Chrome, as expected), whereas after this change `mimetypes.guess_type("foo.js")` gives the correct value and TensorBoard returns the correct content type in its JavaScript responses. TensorBoard still does not render on Windows due to unrelated errors in the debugger plugin. To check that this set of patches is sufficient, `git grep guess_type` shows call sites only in `core_plugin` and `projector_plugin`. The former takes file names from `webfiles.zip`, which has only `html`, `js`, and `woff2` files (plus a `LICENSE`). The latter has a fixed set of inputs with extensions `html` and `js`. wchargin-branch: windows-explicit-mime
Can confirm that after patching in #3129, TensorBoard actually works on |
wchargin-branch: windows-explicit-mime wchargin-source: e069737c538dba27f74f4283f8e3ded6c2ebc887
psybuzz
approved these changes
Jan 10, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't verified it on my Window machine at home, but lgtm to merge.
Happening on AWS Sagemaker as well.. |
This was referenced Jun 7, 2022
bmd3k
added a commit
that referenced
this pull request
Jun 8, 2022
…5746) Recently the test core_plugin_test.py::test_js_cache began to fail in some environments with the following error: ``` ... core_plugin_test.py", line 196, in test_js_cache self.assertEqual( AssertionError: - private, max-age=86400 + no-cache, must-revalidate ``` The root cause is that these systems have now been configured to return 'text/javascript' as the mimetype for js files, where they were previously returning 'application/javascript'. This means the 'Cache-Control' header was not set by this line: https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/plugins/core/core_plugin.py;l=137;drc=c061b659f50cde9dd9da5aa906ecb682ecdc68d4 ``` expires = ( JS_CACHE_EXPIRATION_IN_SECS if request.args.get("_file_hash") and mimetype == "application/javascript" else 0 ) ``` After further investigation (thanks @nfelt!) we determined that the change in mimetype is likely intentional after the recent publishing of RFC 9239 (https://www.rfc-editor.org/rfc/rfc9239), where 'text/javascript' becomes the standard mimetype for js files. We decided, therefore, to change the Cache-Control-setting logic to also recognize 'text/javascript'. Note, in practice, the change in mimetype had no impact on running tensorboards thanks to the logic added in #3128, which always ensures ".js" is mapped to mimetype "application/javascript" but does it at a higher level (ie outside either core plugin or the http utils).
bmd3k
added a commit
to bmd3k/tensorboard
that referenced
this pull request
Jun 8, 2022
…ensorflow#5746) Recently the test core_plugin_test.py::test_js_cache began to fail in some environments with the following error: ``` ... core_plugin_test.py", line 196, in test_js_cache self.assertEqual( AssertionError: - private, max-age=86400 + no-cache, must-revalidate ``` The root cause is that these systems have now been configured to return 'text/javascript' as the mimetype for js files, where they were previously returning 'application/javascript'. This means the 'Cache-Control' header was not set by this line: https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/plugins/core/core_plugin.py;l=137;drc=c061b659f50cde9dd9da5aa906ecb682ecdc68d4 ``` expires = ( JS_CACHE_EXPIRATION_IN_SECS if request.args.get("_file_hash") and mimetype == "application/javascript" else 0 ) ``` After further investigation (thanks @nfelt!) we determined that the change in mimetype is likely intentional after the recent publishing of RFC 9239 (https://www.rfc-editor.org/rfc/rfc9239), where 'text/javascript' becomes the standard mimetype for js files. We decided, therefore, to change the Cache-Control-setting logic to also recognize 'text/javascript'. Note, in practice, the change in mimetype had no impact on running tensorboards thanks to the logic added in tensorflow#3128, which always ensures ".js" is mapped to mimetype "application/javascript" but does it at a higher level (ie outside either core plugin or the http utils).
bmd3k
added a commit
that referenced
this pull request
Jun 8, 2022
…5746) Recently the test core_plugin_test.py::test_js_cache began to fail in some environments with the following error: ``` ... core_plugin_test.py", line 196, in test_js_cache self.assertEqual( AssertionError: - private, max-age=86400 + no-cache, must-revalidate ``` The root cause is that these systems have now been configured to return 'text/javascript' as the mimetype for js files, where they were previously returning 'application/javascript'. This means the 'Cache-Control' header was not set by this line: https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/plugins/core/core_plugin.py;l=137;drc=c061b659f50cde9dd9da5aa906ecb682ecdc68d4 ``` expires = ( JS_CACHE_EXPIRATION_IN_SECS if request.args.get("_file_hash") and mimetype == "application/javascript" else 0 ) ``` After further investigation (thanks @nfelt!) we determined that the change in mimetype is likely intentional after the recent publishing of RFC 9239 (https://www.rfc-editor.org/rfc/rfc9239), where 'text/javascript' becomes the standard mimetype for js files. We decided, therefore, to change the Cache-Control-setting logic to also recognize 'text/javascript'. Note, in practice, the change in mimetype had no impact on running tensorboards thanks to the logic added in #3128, which always ensures ".js" is mapped to mimetype "application/javascript" but does it at a higher level (ie outside either core plugin or the http utils).
bmd3k
added a commit
that referenced
this pull request
Jun 8, 2022
While investigating the test failure that lead to #5746, we realized that RFC 9239 (https://www.rfc-editor.org/rfc/rfc9239) has changed the preferred js mimetype to 'text/javascript' from 'application/javascript'. To be a good citizen of the internet, this change modifies the logic introduced in #3128 to force the mapping of js files to mimetype 'text/javascript'. We must also modify http_util.py to ensure that the proper charset is included in the Content-Type header. And, finally, we modify the example plugin to use 'text/javascript' as mimetype. To Test: * I ran local tensorboard and used the chrome developer tools to verify that the Content-Type for js files fetched by the browser was 'text/javascript; charset=utf-8'. I did a basic sanity test ensuring both Angular and Polymer portions of the application are running. * I ran a tensorboard with the example plugin installed and verified that index.js is returned with Content-Type 'text/javascript'.
yatbear
pushed a commit
to yatbear/tensorboard
that referenced
this pull request
Mar 27, 2023
…ensorflow#5746) Recently the test core_plugin_test.py::test_js_cache began to fail in some environments with the following error: ``` ... core_plugin_test.py", line 196, in test_js_cache self.assertEqual( AssertionError: - private, max-age=86400 + no-cache, must-revalidate ``` The root cause is that these systems have now been configured to return 'text/javascript' as the mimetype for js files, where they were previously returning 'application/javascript'. This means the 'Cache-Control' header was not set by this line: https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/plugins/core/core_plugin.py;l=137;drc=c061b659f50cde9dd9da5aa906ecb682ecdc68d4 ``` expires = ( JS_CACHE_EXPIRATION_IN_SECS if request.args.get("_file_hash") and mimetype == "application/javascript" else 0 ) ``` After further investigation (thanks @nfelt!) we determined that the change in mimetype is likely intentional after the recent publishing of RFC 9239 (https://www.rfc-editor.org/rfc/rfc9239), where 'text/javascript' becomes the standard mimetype for js files. We decided, therefore, to change the Cache-Control-setting logic to also recognize 'text/javascript'. Note, in practice, the change in mimetype had no impact on running tensorboards thanks to the logic added in tensorflow#3128, which always ensures ".js" is mapped to mimetype "application/javascript" but does it at a higher level (ie outside either core plugin or the http utils).
yatbear
pushed a commit
to yatbear/tensorboard
that referenced
this pull request
Mar 27, 2023
…5747) While investigating the test failure that lead to tensorflow#5746, we realized that RFC 9239 (https://www.rfc-editor.org/rfc/rfc9239) has changed the preferred js mimetype to 'text/javascript' from 'application/javascript'. To be a good citizen of the internet, this change modifies the logic introduced in tensorflow#3128 to force the mapping of js files to mimetype 'text/javascript'. We must also modify http_util.py to ensure that the proper charset is included in the Content-Type header. And, finally, we modify the example plugin to use 'text/javascript' as mimetype. To Test: * I ran local tensorboard and used the chrome developer tools to verify that the Content-Type for js files fetched by the browser was 'text/javascript; charset=utf-8'. I did a basic sanity test ensuring both Angular and Polymer portions of the application are running. * I ran a tensorboard with the example plugin installed and verified that index.js is returned with Content-Type 'text/javascript'.
dna2github
pushed a commit
to dna2fork/tensorboard
that referenced
this pull request
May 1, 2023
…ensorflow#5746) Recently the test core_plugin_test.py::test_js_cache began to fail in some environments with the following error: ``` ... core_plugin_test.py", line 196, in test_js_cache self.assertEqual( AssertionError: - private, max-age=86400 + no-cache, must-revalidate ``` The root cause is that these systems have now been configured to return 'text/javascript' as the mimetype for js files, where they were previously returning 'application/javascript'. This means the 'Cache-Control' header was not set by this line: https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/plugins/core/core_plugin.py;l=137;drc=c061b659f50cde9dd9da5aa906ecb682ecdc68d4 ``` expires = ( JS_CACHE_EXPIRATION_IN_SECS if request.args.get("_file_hash") and mimetype == "application/javascript" else 0 ) ``` After further investigation (thanks @nfelt!) we determined that the change in mimetype is likely intentional after the recent publishing of RFC 9239 (https://www.rfc-editor.org/rfc/rfc9239), where 'text/javascript' becomes the standard mimetype for js files. We decided, therefore, to change the Cache-Control-setting logic to also recognize 'text/javascript'. Note, in practice, the change in mimetype had no impact on running tensorboards thanks to the logic added in tensorflow#3128, which always ensures ".js" is mapped to mimetype "application/javascript" but does it at a higher level (ie outside either core plugin or the http utils).
dna2github
pushed a commit
to dna2fork/tensorboard
that referenced
this pull request
May 1, 2023
…5747) While investigating the test failure that lead to tensorflow#5746, we realized that RFC 9239 (https://www.rfc-editor.org/rfc/rfc9239) has changed the preferred js mimetype to 'text/javascript' from 'application/javascript'. To be a good citizen of the internet, this change modifies the logic introduced in tensorflow#3128 to force the mapping of js files to mimetype 'text/javascript'. We must also modify http_util.py to ensure that the proper charset is included in the Content-Type header. And, finally, we modify the example plugin to use 'text/javascript' as mimetype. To Test: * I ran local tensorboard and used the chrome developer tools to verify that the Content-Type for js files fetched by the browser was 'text/javascript; charset=utf-8'. I did a basic sanity test ensuring both Angular and Polymer portions of the application are running. * I ran a tensorboard with the example plugin installed and verified that index.js is returned with Content-Type 'text/javascript'.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
The standard library’s
mimetypes.guess_type
can give incorrect resultson Windows, because arbitrary programs can influence its behavior by
writing to the registry. This can be worked around by reinstating the
correct MIME types with
mimetypes.add_type
, as in this patch. Fixes#3120.
Test Plan:
To check that this patch works at all on Windows, edited the registry to
add a bad content type:
…then verified that before this change,
mimetypes.guess_type("foo.js")
gives the wrong value and TensorBoard does not render properly (strict
MIME type checking error in Chrome, as expected), whereas after this
change
mimetypes.guess_type("foo.js")
gives the correct value andTensorBoard returns the correct content type in its JavaScript
responses. TensorBoard still does not render on Windows due to unrelated
errors in the debugger plugin.
To check that this set of patches is sufficient,
git grep guess_type
shows call sites only in
core_plugin
andprojector_plugin
. Theformer takes file names from
webfiles.zip
, which has onlyhtml
,js
, andwoff2
files (plus aLICENSE
). The latter has a fixed setof inputs with extensions
html
andjs
.wchargin-branch: windows-explicit-mime