Implement guards for stringToBytes and bytesToString #5600

timvandermeij · 2014-12-30T13:20:53Z

This makes it more clear where the problem could be for broken PDFs (as was the case for #5599).

Snuffleupagus · 2014-12-30T13:29:40Z

src/shared/util.js

@@ -454,7 +454,7 @@ function bytesToString(bytes) {
 }

 function stringToBytes(str) {
-  var length = str.length;
+  var length = (str ? str.length : 0);


Since str === undefined probably indicates that there is something wrong with the PDF file, should we print a warning in this case?
Otherwise we might make debugging some future issues harder.

Good point. Missing any indication of where the problem occurred made finding the issue harder. I'll add that.

SamyCookie · 2014-12-30T13:45:16Z

src/shared/util.js

@@ -454,7 +454,10 @@ function bytesToString(bytes) {
 }

 function stringToBytes(str) {
-  var length = str.length;
+  if (!str) {


An empty string will check this condition. Is this behaviour desired ?

I would say yes. If we check the usage of stringToBytes using, for example, https://github.com/mozilla/pdf.js/search?utf8=%E2%9C%93&q=stringToBytes, none of those usages should be dealing with the empty string. Even if the empty string is passed, the function will still work, it will just output a warning to the console as an indication that something might be wrong. A regular user won't see that and it allows us to look into it further.

is it called often? maybe we then should not do a double checking here and the ternary below but return here a new Uint8Array(0)

@CodingFabian You are right. I thought about that before, but I didn't check how often it was called so I went for this solution. For http://www.etsi.org/deliver/etsi_ts/151000_151099/15101001/10.03.00_60/ts_15101001v100300p.pdf, a large PDF document, it is called almost 100 times per page. I will change that, thanks!

@CodingFabian +1, seems a little more efficient.
@timvandermeij thanks for the answer !

Snuffleupagus · 2014-12-31T11:22:17Z

(Sorry about the wall of text!)

I've been thinking more about this, and I'm no longer sure if warn is such a great idea here. Perhaps we should use error, or simply assert(str, ...), instead.

My thinking is that if a PDF file is sufficiently broken, or if our code contains an error, we should ideally have failed before attempting to call stringToBytes(undefined).
From a user perspective a warning could be problematic, since there is every chance that a part of the file won't render properly without the user noticing. (Since I'm assuming that most users aren't looking at the console messages).
By throwing an error here we would fail early, and in a very simple way. If we just issue a warning and then try to continue parsing/rendering the file, we're basically moving the error to another part of the code. This being done under the (implicit) assumption that some other part of the code will catch any subsequent errors.

timvandermeij · 2014-12-31T12:19:27Z

@Snuffleupagus True, but triggering error would cause the fallback bar to appear, right? Is that always what we want here? If stringToBytes is called with undefined or null, it might not be a breaking error as we have seen for #5599. If we catch undefined there, the document renders perfectly, so I'm a bit worried that we might be bothering the user with error messages even if the document renders perfectly fine. I thought warn is good for us because we do not bother the user with any messages and if there is a problem and it is reported to us, then when we can see from the console that stringToBytes is at fault.

Snuffleupagus · 2014-12-31T12:34:42Z

but triggering error would cause the fallback bar to appear, right?

Yes, that was kind of my point.

it might not be a breaking error as we have seen for #5599.

That was true for one particular issue, but in general we have no way of knowing that!
As I said, I'm not so sure that it's a great idea to just "move" an error to another part of the code base.

so I'm a bit worried that we might be bothering the user with error messages even if the document renders perfectly fine.

Calling stringToBytes(undefined) should be sufficiently uncommon that this isn't really an issue, in my opinion.

timvandermeij · 2014-12-31T15:18:34Z

@Snuffleupagus I think you're right about that, so I have changed the commit to use error instead of warn. Is it good this way?

Snuffleupagus · 2014-12-31T15:26:07Z

src/shared/util.js

@@ -454,6 +454,10 @@ function bytesToString(bytes) {
 }

 function stringToBytes(str) {
+  if (!str) {
+    error('Invalid or empty string passed to stringToBytes');
+    return new Uint8Array(0);


When using error, this line is no longer necessary since it won't be reached.

Also, I think this patch could be a one-liner if you instead just did:
assert(str, 'Invalid or empty string passed to stringToBytes');

True, assets wraps that for us. Really nice, it's a one-liner now :)

Snuffleupagus · 2014-12-31T16:53:08Z

src/shared/util.js

@@ -454,6 +454,7 @@ function bytesToString(bytes) {
 }

 function stringToBytes(str) {
+  assert(str, 'Invalid or empty string passed to stringToBytes');


This was a stupid suggestion on my part, since an empty string should obviously be considered valid as well (this would explain the test failures, since str === '' at one point). Sorry about that!

Using typeof str === 'string' as condition would probably work, but given how often stringToBytes seems to be used, that might slow things down. I'm not sure what the best solution is, unfortunately :-(

How about this?

if (str === null || str === undefined) { error('Invalid argument passed to stringToBytes'); }

which should be equal to:

assert(str === null || str === undefined, 'Invalid argument passed to stringToBytes');

That way we only check for undefined and null and leave the rest (including the empty string) through.

typeof str is nice, since it actually checks that it's a string. But if the result of micro benchmarks is to be trusted, the performance is unfortunately a bit varied in different browsers: http://jsperf.com/typeof-vs-undefined-null. Just do what you want here :-)

yurydelendik · 2015-01-02T19:11:50Z

src/shared/util.js

@@ -454,6 +454,8 @@ function bytesToString(bytes) {
 }

 function stringToBytes(str) {
+  assert(str !== null && str !== undefined,


Make it: assert(typeof str === 'string', ...);

Also bytesToString looks non-symmetrical now :) can we add asset(bytes !== null && typeof bytes === 'object' && bytes.length !== undefined, ...);

Done in the most recent commit.

timvandermeij · 2015-01-02T22:38:42Z

/botio-linux preview

pdfjsbot · 2015-01-02T22:38:43Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/626cf9f48d3b201/output.txt

pdfjsbot · 2015-01-02T22:39:30Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/626cf9f48d3b201/output.txt

Total script time: 0.78 mins

Published

Viewer: http://107.21.233.14:8877/626cf9f48d3b201/web/viewer.html
B2G Viewer: http://107.21.233.14:8877/626cf9f48d3b201/extensions/b2g/content/web/viewer.html
Extension: http://107.21.233.14:8877/626cf9f48d3b201/extensions/firefox/pdf.js.xpi
Extension (AMO): http://107.21.233.14:8877/626cf9f48d3b201/extensions/firefox/pdf.js.amo.xpi

timvandermeij · 2015-01-02T22:39:56Z

/botio test

pdfjsbot · 2015-01-02T22:39:57Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/9c132cad5578bc5/output.txt

pdfjsbot · 2015-01-02T22:39:57Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/7c2d07b52074ddb/output.txt

pdfjsbot · 2015-01-02T22:57:24Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/9c132cad5578bc5/output.txt

Total script time: 17.44 mins

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

pdfjsbot · 2015-01-02T23:02:57Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/7c2d07b52074ddb/output.txt

Total script time: 23.00 mins

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

Implement guards for stringToBytes and bytesToString

yurydelendik · 2015-01-06T22:27:20Z

Thanks

Implement guards for stringToBytes and bytesToString

timvandermeij added the core label Dec 30, 2014

Snuffleupagus reviewed Dec 30, 2014
View reviewed changes

timvandermeij force-pushed the str-undefined branch from 06fca7c to d0a0d7f Compare December 30, 2014 13:38

SamyCookie reviewed Dec 30, 2014
View reviewed changes

timvandermeij force-pushed the str-undefined branch from d0a0d7f to b437334 Compare December 30, 2014 14:18

timvandermeij force-pushed the str-undefined branch from b437334 to 3b57439 Compare December 31, 2014 15:17

Snuffleupagus reviewed Dec 31, 2014
View reviewed changes

timvandermeij force-pushed the str-undefined branch from 3b57439 to d7f62b6 Compare December 31, 2014 15:30

Snuffleupagus reviewed Dec 31, 2014
View reviewed changes

timvandermeij force-pushed the str-undefined branch 2 times, most recently from 0aed2b3 to f27cd69 Compare January 1, 2015 18:05

yurydelendik reviewed Jan 2, 2015
View reviewed changes

Implement guards for stringToBytes and bytesToString

6e99c29

timvandermeij force-pushed the str-undefined branch from f27cd69 to 6e99c29 Compare January 2, 2015 22:37

timvandermeij changed the title ~~Do not call str.length if str is undefined~~ Implement guards for stringToBytes and bytesToString Jan 2, 2015

yurydelendik added a commit that referenced this pull request Jan 6, 2015

Merge pull request #5600 from timvandermeij/str-undefined

34bed63

Implement guards for stringToBytes and bytesToString

yurydelendik merged commit 34bed63 into mozilla:master Jan 6, 2015

timvandermeij deleted the str-undefined branch January 6, 2015 22:29

speedplane pushed a commit to speedplane/pdf.js that referenced this pull request Feb 24, 2015

Merge pull request mozilla#5600 from timvandermeij/str-undefined

7137285

Implement guards for stringToBytes and bytesToString

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement guards for stringToBytes and bytesToString #5600

Implement guards for stringToBytes and bytesToString #5600

timvandermeij commented Dec 30, 2014

Snuffleupagus Dec 30, 2014

timvandermeij Dec 30, 2014

SamyCookie Dec 30, 2014

timvandermeij Dec 30, 2014

CodingFabian Dec 30, 2014

timvandermeij Dec 30, 2014

SamyCookie Dec 30, 2014

Snuffleupagus commented Dec 31, 2014

timvandermeij commented Dec 31, 2014

Snuffleupagus commented Dec 31, 2014

timvandermeij commented Dec 31, 2014

Snuffleupagus Dec 31, 2014

timvandermeij Dec 31, 2014

Snuffleupagus Dec 31, 2014

timvandermeij Dec 31, 2014

Snuffleupagus Jan 1, 2015

yurydelendik Jan 2, 2015

timvandermeij Jan 2, 2015

timvandermeij commented Jan 2, 2015

pdfjsbot commented Jan 2, 2015

pdfjsbot commented Jan 2, 2015

timvandermeij commented Jan 2, 2015

pdfjsbot commented Jan 2, 2015

pdfjsbot commented Jan 2, 2015

pdfjsbot commented Jan 2, 2015

pdfjsbot commented Jan 2, 2015

yurydelendik commented Jan 6, 2015

Implement guards for stringToBytes and bytesToString #5600

Implement guards for stringToBytes and bytesToString #5600

Conversation

timvandermeij commented Dec 30, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus commented Dec 31, 2014

timvandermeij commented Dec 31, 2014

Snuffleupagus commented Dec 31, 2014

timvandermeij commented Dec 31, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij commented Jan 2, 2015

pdfjsbot commented Jan 2, 2015

From: Bot.io (Linux)

Received

pdfjsbot commented Jan 2, 2015

From: Bot.io (Linux)

Success

Published

timvandermeij commented Jan 2, 2015

pdfjsbot commented Jan 2, 2015

From: Bot.io (Windows)

Received

pdfjsbot commented Jan 2, 2015

From: Bot.io (Linux)

Received

pdfjsbot commented Jan 2, 2015

From: Bot.io (Windows)

Success

pdfjsbot commented Jan 2, 2015

From: Bot.io (Linux)

Success

yurydelendik commented Jan 6, 2015