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

Correctly validate the response status for non-HTTP fetch requests (PR 8768 follow-up) #8866

Merged
merged 1 commit into from
Sep 5, 2017
Merged

Correctly validate the response status for non-HTTP fetch requests (PR 8768 follow-up) #8866

merged 1 commit into from
Sep 5, 2017

Conversation

Snuffleupagus
Copy link
Collaborator

@Snuffleupagus Snuffleupagus commented Sep 2, 2017

It seems that the status check, for non-HTTP loads, has been inverted which causes the default viewer to refuse to open local PDF files. Follow-up to PR #8768.

STR:

  1. Make sure that fetch support is enabled in the browser. In Firefox Nightly, set dom.streams.enabled = true and javascript.options.streams = true in about:config. Edit: Just to clarify, please note that the issue is reproducible in Google Chrome too.
  2. Open https://mozilla.github.io/pdf.js/web/viewer.html.
  3. Click on the "Open file" button, and open a new PDF file.

ER:
A new PDF file should open in the viewer.

AR:
The PDF file fails to open, with an error message of the following format:
Message: Unexpected server response (200) while retrieving PDF "blob:https://mozilla.github.io/a4fc455f-bc05-45b5-b6aa-2ecff3cb45ce".

Edit: Fixes #8870.

Copy link
Contributor

@yurydelendik yurydelendik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove entire if(!isHttp) statement. Not sure if fetch() for file: protocol is returning 0 for success.

@Snuffleupagus
Copy link
Collaborator Author

Snuffleupagus commented Sep 5, 2017

Let's remove entire if(!isHttp) statement.

Fixed now, thanks!

Not sure if fetch() for file: protocol is returning 0 for success.

According to https://fetch.spec.whatwg.org/#scheme-fetch, it would seem that file:// support isn't even properly defined in the specification.

@mozilla mozilla deleted a comment from pdfjsbot Sep 5, 2017
@mozilla mozilla deleted a comment from pdfjsbot Sep 5, 2017
@mozilla mozilla deleted a comment from pdfjsbot Sep 5, 2017
@mozilla mozilla deleted a comment from pdfjsbot Sep 5, 2017
@mozilla mozilla deleted a comment from pdfjsbot Sep 5, 2017
@mozilla mozilla deleted a comment from pdfjsbot Sep 5, 2017
…R 8768 follow-up)

It seems that the status check, for non-HTTP loads, causes the default viewer to *refuse* to open local PDF files.

***STR:***
 1. Make sure that fetch support is enabled in the browser. In Firefox Nightly, set `dom.streams.enabled = true` and `javascript.options.streams = true` in `about:config`.
 2. Open https://mozilla.github.io/pdf.js/web/viewer.html.
 3. Click on the "Open file" button, and open a new PDF file.

***ER:***
 A new PDF file should open in the viewer.

***AR:***
 The PDF file fails to open, with an error message of the following format:
`Message: Unexpected server response (200) while retrieving PDF "blob:https://mozilla.github.io/a4fc455f-bc05-45b5-b6aa-2ecff3cb45ce".`
@mozilla mozilla deleted a comment from pdfjsbot Sep 5, 2017
@mozilla mozilla deleted a comment from pdfjsbot Sep 5, 2017
@Snuffleupagus
Copy link
Collaborator Author

/botio-linux preview

@pdfjsbot
Copy link

pdfjsbot commented Sep 5, 2017

From: Bot.io (Linux m4)


Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.67.70.0:8877/491b20fe9aed89e/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Sep 5, 2017

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/491b20fe9aed89e/output.txt

Total script time: 2.32 mins

Published

@pdfjsbot
Copy link

pdfjsbot commented Sep 5, 2017

From: Bot.io (Linux m4)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.67.70.0:8877/faf8c29db700c60/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Sep 5, 2017

From: Bot.io (Windows)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.215.176.217:8877/4016277e2bbd6d7/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Sep 5, 2017

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/4016277e2bbd6d7/output.txt

Total script time: 29.66 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://54.215.176.217:8877/4016277e2bbd6d7/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

pdfjsbot commented Sep 5, 2017

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/faf8c29db700c60/output.txt

Total script time: 60.00 mins

@yurydelendik
Copy link
Contributor

/botio-linux test

@pdfjsbot
Copy link

pdfjsbot commented Sep 5, 2017

From: Bot.io (Linux m4)


Received

Command cmd_test from @yurydelendik received. Current queue size: 0

Live output at: http://54.67.70.0:8877/4b687477818032a/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Sep 5, 2017

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/4b687477818032a/output.txt

Total script time: 16.44 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@yurydelendik yurydelendik merged commit 9b14f8e into mozilla:master Sep 5, 2017
@yurydelendik
Copy link
Contributor

Thank you for the patch.

@mukulmishra18
Copy link
Contributor

According to https://fetch.spec.whatwg.org/#scheme-fetch, it would seem that file:// support isn't even properly defined in the specification.

file: protocol is not supported by the native fetch implementation and always throws TypeError, see JakeChampion/fetch#186 .

@Snuffleupagus Snuffleupagus deleted the fix-non-HTTP-validateResponseStatus branch September 5, 2017 18:08
movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018
…ateResponseStatus

Correctly validate the response status for non-HTTP fetch requests (PR 8768 follow-up)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants