Fix for web worker test in Safari #1849

Merged
merged 9 commits into from Jun 28, 2012

Projects

None yet

5 participants

@Reggino
Contributor
Reggino commented Jun 26, 2012

Safari can not read or write typed arrays to 'web workers'. This will cause Safari on the iPhone and iPad to crash. The desktop version of Safari just throws a weird error.

This behaviour is tested by pdf.js while a pdf is being downloaded from a URL. However, sometimes the pdf has finished downloading BEFORE the web worker-test has finished! In that case the rendering of the final pdf would cause errors, because the web workers have not yet been disabled.

This patch fixes this and the issues #1314, #1627, #1643, #1846 and #1805 .

@yurydelendik yurydelendik commented on an outdated diff Jun 27, 2012
@@ -49,23 +50,26 @@ PDFJS.getDocument = function getDocument(source) {
url: url,
progress: function getPDFProgress(evt) {
if (evt.lengthComputable)
- promise.progress({
+ workerReadyPromise.progress({
yurydelendik
yurydelendik Jun 27, 2012 Contributor

Indentation is wrong

@yurydelendik yurydelendik commented on an outdated diff Jun 27, 2012
loaded: evt.loaded,
total: evt.total
});
},
error: function getPDFError(e) {
- promise.reject('Unexpected server response of ' +
+ workerReadyPromise.reject('Unexpected server response of ' +
yurydelendik
yurydelendik Jun 27, 2012 Contributor

Indentation is wrong

@yurydelendik yurydelendik commented on an outdated diff Jun 27, 2012
e.target.status + '.');
},
headers: headers
},
function getPDFLoad(data) {
- transport.sendData(data, parameters);
+ //we have to wait for the WorkerTransport to finalize worker-support detection! This may take a while...
yurydelendik
yurydelendik Jun 27, 2012 Contributor

Not complete/useful comment. Please add your issue description text here: "Sometimes the pdf has finished downloading before the web worker-test has finished. In that case the rendering of the final pdf would cause errors." (without capitalization and exclamation points); then "We have to wait for the WorkerTransport to finalize worker-support detection."

Contributor

/botio-linux lint

Collaborator

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/14c05dc01cdbab4/output.txt

Collaborator

From: Bot.io (Linux)


Failed

Full output at http://107.21.233.14:8877/14c05dc01cdbab4/output.txt

Total script time: 1.17 mins

  • Lint: FAILED
Contributor

Thank you for investigating this. Please address the long lines, illegal tabs and other lint issues.

Also, this patch might conflict with #1840.

/botio-windows preview

Collaborator

From: Bot.io (Windows)


Received

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

Live output at: http://107.22.172.223:8877/2dd123771760a8a/output.txt

Contributor
Reggino commented Jun 27, 2012

Hmmm i just found out that iPhone / iPad support needs some more love... will look further into that...

Contributor
Reggino commented Jun 27, 2012

pdf.js now is actually working on iOS again. I found out that Uint8array on Safari 5 is lacking the subarray-method and has another issue with Object.defineProperty and DOM objects. This is now fixed and the code is made lint-compliant.

@yurydelendik yurydelendik and 1 other commented on an outdated diff Jun 27, 2012
web/compatibility.js
@@ -69,8 +79,10 @@
// Object.defineProperty() ?
(function checkObjectDefinePropertyCompatibility() {
- if (typeof Object.defineProperty !== 'undefined')
- return;
+ // safari 5 cannot use this on DOM objects and thus is unusable,
+ // see http://kangax.github.com/es5-compat-table/
+ if ((typeof Object.defineProperty !== 'undefined') &&
+ /Safari\/5/.test(navigator.userAgent)) return;
yurydelendik
yurydelendik Jun 27, 2012 Contributor

Looks like it shall be !/Safari\/5/.test(navigator.userAgent)

Reggino
Reggino Jun 27, 2012 Contributor

Wow, great catch!

The reason this slipped through is because my iPad seems to be running Safari 6 and that browser is still having the same issue. Fixed it, thanks @yurydelendik

Contributor

/botio-windows lint

Collaborator

From: Bot.io (Windows)


Received

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

Live output at: http://107.22.172.223:8877/257ea6e3d31d34b/output.txt

Contributor

/botio-linux preview

Collaborator

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/fc664b957bebc0c/output.txt

Collaborator

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/257ea6e3d31d34b/output.txt

Total script time: 1.19 mins

  • Lint: Passed
@brendandahl brendandahl and 1 other commented on an outdated diff Jun 27, 2012
web/compatibility.js
@@ -69,8 +79,9 @@
// Object.defineProperty() ?
(function checkObjectDefinePropertyCompatibility() {
- if (typeof Object.defineProperty !== 'undefined')
- return;
+ // safari 5 and 6 cannot use this on DOM objects and thus it's unusable,
+ if ((typeof Object.defineProperty !== 'undefined') &&
+ !/Safari/.test(navigator.userAgent)) return;
brendandahl
brendandahl Jun 27, 2012 Contributor

We should not be looking at the user agent. Use feature detection instead. See https://github.com/kriskowal/es5-shim/blob/master/es5-shim.js#L599 for inspiration.

Reggino
Reggino Jun 27, 2012 Contributor

I agree. I've updated the test accordingly... Thanks!

Contributor

/botio-windows lint
/botio-linux preview

Collaborator

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/87d064919c336a3/output.txt

Collaborator

From: Bot.io (Windows)


Received

Command cmd_lint from @brendandahl received. Current queue size: 0

Live output at: http://107.22.172.223:8877/820df6c19fffd51/output.txt

Collaborator

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/820df6c19fffd51/output.txt

Total script time: 1.31 mins

  • Lint: Passed
Tim de Koning added some commits Jun 27, 2012
Contributor
Reggino commented Jun 28, 2012

/botio-windows lint
/botio-linux preview

Contributor

/botio-windows lint
/botio-linux preview

Collaborator

From: Bot.io (Windows)


Received

Command cmd_lint from @jviereck received. Current queue size: 0

Live output at: http://107.22.172.223:8877/04043f768493b8a/output.txt

Collaborator

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/9ce7abdaeeccada/output.txt

Collaborator

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/04043f768493b8a/output.txt

Total script time: 1.23 mins

  • Lint: Passed
@yurydelendik yurydelendik merged commit 665ff0d into mozilla:master Jun 28, 2012
Contributor

Thank you for the patch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment