-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
Feature-test moz-chunked-arraybuffer before use #5531
Conversation
@yurydelendik Could you merge this? Since Chrome 44.0.2360.0, the console prints the following warning:
|
@@ -75,6 +75,21 @@ var NetworkManager = (function NetworkManagerClosure() { | |||
return array.buffer; | |||
} | |||
|
|||
//#if (GENERIC || FIREFOX || B2G || MOZCENTRAL) |
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'm just wondering: Since we know that moz-chunked-arraybuffer
is supported for MOZCENTRAL
(and FIREFOX
too, for all versions that we realistically care about), couldn't we avoid doing any additional checks for those cases? E.g. something like this:
//#if (FIREFOX || MOZCENTRAL)
// var supportsMozChunked = true;
//#else
var supportsMozChunked = (function supportsMozChunkedClosure() {
var x = new XMLHttpRequest();
.
.
.
//#endif
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.
for all versions that we realistically care about
If that's the case, then yes, we can. Do others feel the same?
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.
Some additional information:
It seems that moz-chunked-arraybuffer
was added in https://bugzilla.mozilla.org/show_bug.cgi?id=687087, which landed in Firefox 9 (released on 2011-12-20). I would be very surprised if the addon even works in such an old version of Firefox anyway, so it should be fine.
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 have added another commit to the PR that completely eliminates this extra variable in Firefox. Could you take a look?
07dbb74
to
d9410f3
Compare
//#if (FIREFOX || MOZCENTRAL) | ||
//var supportsMozChunked = true; | ||
//#endif | ||
//#if GENERIC |
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.
Nit: Since there is a SINGLE_FILE
build target as well, would it perhaps make more sense to just use //#else
instead of
//#endif
//#if GENERIC
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 wanted to exclude the feature detection logic from the CHROMIUM
builds. And AFAIK there is no #elseif
in our preprocessor, hence I've used #endif
+ #if GENERIC
.
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.
That's a good idea. Let's use that logic for FIREFOX as well, e.g. use supportMozChunked only when !(FIREFOX || MOZCENTRAL || CHROMIUM)
#elseif
shall be expressed in terms of #if CASE1
, #if CASE2
, #if !(CASE1 || CASE2)
I'd still like @yurydelendik to sign off on the patch, but let's check that the tests pass. /botio test |
From: Bot.io (Linux)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/795d9c3547006e2/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.22.172.223:8877/bd277a69974edbe/output.txt |
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/bd277a69974edbe/output.txt Total script time: 17.74 mins
|
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/795d9c3547006e2/output.txt Total script time: 18.30 mins
|
@yurydelendik Gentle ping to get this PR merged. I want to get rid of console spam (#5531 (comment)) (the log spam already appears in the current version of Chrome, 43.0.2357.81). |
} else { | ||
xhr.responseType = 'arraybuffer'; | ||
} | ||
//#else |
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.
make it #if CHROMIUM
and move #else branch up
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.
Why?
Do you want something like this:
//#if CHROMIUM
xhr.responseType = 'arraybuffer';
//#endif
//#if (GENERIC || FIREFOX || MOZCENTRAL)
...
//#endif
or
//#if CHROMIUM
xhr.responseType = 'arraybuffer';
//#else
...
//#endif
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.
We don't want to miss any other future targets (in the past it was B2G), so I'm looking into something like:
//#if CHROMIUM
// xhr.responseType = 'arraybuffer';
//#endif
//#if (FIREFOX || MOZCENTRAL)
// ....
//#endif
//#if !(CHROMIUM || FIREFOX || MOZCENTRAL)
...
//#endif
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.
Since supportsMozChunked
is always set for non-Chromium builds, we can do either of the following (the FIREFOX|MOZCENTRAL build does not really need the boolean, but in my previous PR a reviewer suggested to not abuse the preprocessor to eliminate the boolean, so I've introduced the boolean). WDYT?
//#if CHROMIUM
// xhr.responseType = 'arraybuffer';
//#else
if (args.onProgressiveData && supportsMozChunked) {
xhr.responseType = 'moz-chunked-arraybuffer';
pendingRequest.onProgressiveData = args.onProgressiveData;
pendingRequest.mozChunked = true;
} else {
xhr.responseType = 'arraybuffer';
}
//#endif
or
xhr.responseType = 'arraybuffer';
//#if !CHROMIUM
if (args.onProgressiveData && supportsMozChunked) {
xhr.responseType = 'moz-chunked-arraybuffer';
pendingRequest.onProgressiveData = args.onProgressiveData;
pendingRequest.mozChunked = true;
}
//#endif
or something like #5531 (diff)
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.
but in my previous PR a reviewer suggested to not abuse the preprocessor to eliminate the boolean
Missed that bit -- I'm sorry for confusion. (Now I have to convince @Snuffleupagus to agree with me :) But since we are doing special case for CHROMIUM, I would like to remove a compatibility code for MOZCENTRAL as well, and also to be consistent with stuff we did for fonts in the past.
I'm thinking creating XMLHttpRequest object for feature test during scripts loading is not cheap process, so my goal to not include this code into production FF code base.
(Side note about //#if CASE1
if (check) {
//#endif
-- I don't think we shall use incomplete if-/while-/try-statements in the preprecessor)
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.
The supportsMozChunked
is always true for MOZCENTRAL and FIREFOX, but it can also be true or false for non-FF/CHROMIUM. Because of this, and because we don't want preprocessor magic to eliminate the boolean, the supportsMozChunked
check always happens in non-Chromium builds.
The appearance of the preprocessor special case for Chromium can be removed by adding
//#if CHROMIUM
//var supportsMozChunked = false;
//#endif
but that doesn't gain us much, does it?
Do you have a preference for any of the presented alternatives (this comment and my previous comment)?
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.
and because we don't want preprocessor magic to eliminate the boolean,
Au contraire, I want to eliminate the boolean for FF/CHROMIUM, means that in non-FF/CHROMIUM code (which will be uncommented by default) the supportsMozChunked feature test and check will be present. For FF/CHROMIUM cases, we use preprocessor to provide optimized versions of the same code (which will be commented) with assumption of supportsMozChunked=true/false. (Don't worry about code duplication)
Does it make more sense?
In the future, I hope we can re-write this code and define better classes/interfaces than XHR that will be more suitable for our operations.
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.
That certainly makes sense, and I've also understood it in that way.
By "preprocessor magic", I meant #5531 (diff). Assuming that setting .responseType
multiple times is inexpensive (which probably is), the complete code (with minimal use of the boolean) could be:
xhr.responseType = 'arraybuffer';
//#if (MOZCENTRAL || FIREFOX)
// if (args.onProgressiveData) {
//#endif
//#if !(MOZCENTRAL || FIREFOX || CHROMIUM)
if (args.onProgressiveData && supportsMozChunked) {
//#endif
//#if !CHROMIUM
xhr.responseType = 'moz-chunked-arraybuffer';
pendingRequest.onProgressiveData = args.onProgressiveData;
pendingRequest.mozChunked = true;
}
//#endif
But in a previous comment, you've expressed that you don't want this if-in-ifdef construct.
(The alternative (without boolean AND without if-in-ifdef) is duplication of code (copy & paste of the full if
block, and include supportsMozChunked
in non-FF/CR builds, and omit supportsMozChunked
in FF builds). This is even uglier and undesirable.)
In the future, I hope we can re-write this code and define better classes/interfaces than XHR that will be more suitable for our operations.
Definitely. The current code is too tightly coupled with XHR, and trying to replace it with the fetch
API will be a challenge (in Chrome 43+, fetch
supports readable streams, which enables PDF.js to display PDFs even if the server does not support range requests - #5319 & #6126).
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.
duplication of code might be fine in this case. okay, may I suggest a compromise that will remove the test for MOZCENTRAL/CHROMIUM, however 'moz-chunked-arraybuffer' branch will still be present (it will be mostly likely removed by JIT)?
var useMozChunkedLoading = false;
//#if (FIREFOX || MOZCENTRAL)
// useMozChunkedLoading = !!args.onProgressiveData;
//#endif
//#if !(CHROMIUM || FIREFOX || MOZCENTRAL)
useMozChunkedLoading = supportsMozChunked && !!args.onProgressiveData;
//#endif
if (useMozChunkedLoading) {
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've submitted a variant of this, please take a look. To emphasize that the options are mutually exclusive, I've moved var useMozChunkedLoading = false;
inside a #ifdef CHROMIUM
and added var
in front of the other options.
d9410f3
to
8a87bff
Compare
Looks good. Good to land after testing. /botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @yurydelendik received. Current queue size: 1 Live output at: http://107.22.172.223:8877/500677f2e55c729/output.txt |
From: Bot.io (Linux)ReceivedCommand cmd_test from @yurydelendik received. Current queue size: 1 Live output at: http://107.21.233.14:8877/5c8ca61cc6ee9ad/output.txt |
/botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @yurydelendik received. Current queue size: 2 Live output at: http://107.21.233.14:8877/7ee6b6f9ef78a3b/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/5c8ca61cc6ee9ad/output.txt Total script time: 18.07 mins
|
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/500677f2e55c729/output.txt Total script time: 18.24 mins
|
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/7ee6b6f9ef78a3b/output.txt Total script time: 0.39 mins Published |
|
|
8a87bff
to
5578d6a
Compare
/botio-windows test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @yurydelendik received. Current queue size: 0 Live output at: http://107.22.172.223:8877/75ab13080d8abb8/output.txt |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @yurydelendik received. Current queue size: 0 Live output at: http://107.21.233.14:8877/91fc9230a6f1995/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/91fc9230a6f1995/output.txt Total script time: 0.72 mins Published |
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/75ab13080d8abb8/output.txt Total script time: 18.27 mins
|
var x = new XMLHttpRequest(); | ||
// Firefox requires that .open() is called before setting responseType. | ||
// https://bugzilla.mozilla.org/show_bug.cgi?id=707484 | ||
x.open('GET', 'http://example.com'); |
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.
Is it possible to move this line into the try
block instead.
The reason that I'm asking is that when testing the preview (i.e. the GENERIC
build), Firefox threw an error on this line and the viewer thus failed to load the PDF file. I tracked this down to the fact that I'm using the NoScript addon, which blocked this request (since I hadn't white-listed that particular domain).
Considering that NoScript is a fairly common addon, it's conceivable that more people could run into this. In this case, it thus seems better to me that streaming gets disabled, than the entire viewer breaking.
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.
Done.
diff --git a/src/core/network.js b/src/core/network.js
index 0c714f8..e3a9b63 100644
--- a/src/core/network.js
+++ b/src/core/network.js
@@ -78,9 +78,16 @@ var NetworkManager = (function NetworkManagerClosure() {
//#if !(CHROME || FIREFOX || MOZCENTRAL)
var supportsMozChunked = (function supportsMozChunkedClosure() {
var x = new XMLHttpRequest();
- // Firefox requires that .open() is called before setting responseType.
- // https://bugzilla.mozilla.org/show_bug.cgi?id=707484
- x.open('GET', 'http://example.com');
+ try {
+ // Firefox 37- required .open() to be called before setting responseType.
+ // https://bugzilla.mozilla.org/show_bug.cgi?id=707484
+ x.open('GET', 'https://example.com');
+ } catch (e) {
+ // Even though the URL is not visited, .open() could fail if the URL is
+ // blocked, e.g. via the connect-src CSP directive or the NoScript addon.
+ // When this error occurs, this feature detection method will mistakenly
+ // report that moz-chunked-arraybuffer is not supported in Firefox 37-.
+ }
try {
x.responseType = 'moz-chunked-arraybuffer';
return x.responseType === 'moz-chunked-arraybuffer';
5578d6a
to
2e63dcd
Compare
/botio test |
From: Bot.io (Linux)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/559be50aa5ebc0d/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.22.172.223:8877/ed880e31d361ac3/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/559be50aa5ebc0d/output.txt Total script time: 18.52 mins
|
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/ed880e31d361ac3/output.txt Total script time: 18.52 mins
|
Feature-test moz-chunked-arraybuffer before use
Thank you for the patch |
Resubmission of #5457, rebased to master. The previous one was closed with the following comment:
This PR indeed has a bit more code, but that is caused by the fact that this PR uses feature detection instead of try-catch.
The other diff is just some re-indention after moving the Mozilla-specific APIs between preprocessor conditionals.