Fix TypeError that occurs in colorspace.js on accidentally passing an 'Array' instead of 'TypedArray' #8666

apoorv-mishra · 2017-07-19T00:37:12Z

Hi,
(Assumption: The base color space is set to be DeviceRGB)
If an Array(not as TypedArray or String) is passed in IndexedCS constructor(https://github.com/mozilla/pdf.js/blob/master/src/core/colorspace.js#L509) as lookup, TypeError occurs on calling fillRgb(...)(on IndexedCS object), passing lookup as comps(https://github.com/mozilla/pdf.js/blob/master/src/core/colorspace.js#L116-L118). The error occurs because of src.subarray(...) in https://github.com/mozilla/pdf.js/blob/master/src/core/colorspace.js#L635 since src is expected to be a TypedArray. I made a small change to fix this.

Snuffleupagus · 2017-07-19T07:01:24Z

/botio-linux preview

Snuffleupagus · 2017-07-19T07:27:52Z

Thanks for the patch, but I've got a couple of questions/suggestions below.

Did you successfully run the reference tests locally, as explained in https://github.com/mozilla/pdf.js/wiki/Contributing, with this patch?
Looking at the IRC logs, in particular http://logs.glob.uno/?c=mozilla%23pdfjs&s=18+Jul+2017&e=18+Jul+2017#c64549, it mentions TAMReview.pdf as an example of Indexed ColorSpace usage. It appears that this patch unfortunately breaks at least that file, and thus quite possibly others as well.

From looking at the history of the code, see PR #3334, it appears that the only reason for supporting Array at all was for IE9 compatibility. Note that the lookup parameter, as coming from a PDF file, should never actually be a regular Array.
Wouldn't a better solution be to change colorspace.js#L528-L529 to e.g. the following instead?

} else if (lookup instanceof Uint8Array) {
  this.lookup = lookup;
} else if (lookup instanceof Array) {
  this.lookup = new Uint8Array(lookup);

Also, please update the commit message (and PR title) to something a little more descriptive, since it's really impossible to tell what the patch does based on TypeError fix in colorspace.js without also reading the PR description. (In this case, even looking at the code itself doesn't really explain the reason for the change.)

apoorv-mishra · 2017-07-19T12:16:28Z

Hi @Snuffleupagus , there is one more issue with making it as Uint8Array. Have a look at http://logs.glob.uno/?c=mozilla%23pdfjs&s=18+Jul+2017&e=18+Jul+2017#c64496.

yurydelendik

Looks like you need to change getRgbItem -> getRgbBuffer call in the IndexedCS_getRgbItem above.

yurydelendik · 2017-07-20T19:17:04Z

src/core/colorspace.js

      this.lookup = lookup;
+    } else if (lookup instanceof Array) {
+      this.lookup = new Uint8Array(lookup);


Can you revert the changes above? For IE9, it will not matter.

yurydelendik

Looks good (with just getRgbBuffer change)

yurydelendik · 2017-07-20T19:41:20Z

/botio test

… 'Array' instead of 'TypedArray' Fix TypeError that occurs in colorspace.js on accidentally passing an 'Array' instead of 'TypedArray' Changed getRgbItem(...) to getRgbBuffer(...) since this.lookup has values in range[0, 255] whereas getRgbItem(...) expects those to be in range [0, 1] Revert changes for IE9 compatibility

timvandermeij · 2017-07-24T22:00:37Z

/botio-linux preview

pdfjsbot · 2017-07-24T22:00:37Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.67.70.0:8877/8b8ec0b4975c7dc/output.txt

pdfjsbot · 2017-07-24T22:03:01Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/8b8ec0b4975c7dc/output.txt

Total script time: 2.38 mins

Published

timvandermeij · 2017-07-24T22:05:27Z

/botio test

pdfjsbot · 2017-07-24T22:05:28Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/cef1bb0b5ee49e7/output.txt

pdfjsbot · 2017-07-24T22:05:28Z

From: Bot.io (Linux m4)

Received

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

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

pdfjsbot · 2017-07-24T22:22:03Z

From: Bot.io (Linux m4)

Success

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

Total script time: 16.58 mins

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

pdfjsbot · 2017-07-24T23:17:22Z

From: Bot.io (Windows)

Success

Full output at http://54.215.176.217:8877/cef1bb0b5ee49e7/output.txt

Total script time: 29.34 mins

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

timvandermeij · 2017-07-25T20:13:17Z

Merging with r=yurydelendik. Thank you for the patch!

Added unit-tests for DeviceGray, DeviceRGB and DeviceCMYK Added unit-tests for CalGray Added unit-tests for CalRGB Removed redundant code Added unit-tests for LabCS Added unit-tests for IndexedCS Update comment Change lookup to Uint8Array as mentioned in pdf specs(these tests will pass after PR mozilla#8666 is merged). Added unit-tests for AlternateCS

Added unit-tests for DeviceGray, DeviceRGB and DeviceCMYK Added unit-tests for CalGray Added unit-tests for CalRGB Removed redundant code Added unit-tests for LabCS Added unit-tests for IndexedCS Update comment Change lookup to Uint8Array as mentioned in pdf specs(these tests will pass after PR mozilla#8666 is merged). Added unit-tests for AlternateCS Resolved code-style issues

Added unit-tests for DeviceGray, DeviceRGB and DeviceCMYK Added unit-tests for CalGray Added unit-tests for CalRGB Removed redundant code Added unit-tests for LabCS Added unit-tests for IndexedCS Update comment Change lookup to Uint8Array as mentioned in pdf specs(these tests will pass after PR mozilla#8666 is merged). Added unit-tests for AlternateCS Resolved code-style issues Fixed code-style issues

Added unit-tests for DeviceGray, DeviceRGB and DeviceCMYK Added unit-tests for CalGray Added unit-tests for CalRGB Removed redundant code Added unit-tests for LabCS Added unit-tests for IndexedCS Update comment Change lookup to Uint8Array as mentioned in pdf specs(these tests will pass after PR mozilla#8666 is merged). Added unit-tests for AlternateCS Resolved code-style issues Fixed code-style issues Addressed issues pointed out in mozilla#8611 (review)

Fix TypeError that occurs in colorspace.js on accidentally passing an 'Array' instead of 'TypedArray'

Added unit-tests for DeviceGray, DeviceRGB and DeviceCMYK Added unit-tests for CalGray Added unit-tests for CalRGB Removed redundant code Added unit-tests for LabCS Added unit-tests for IndexedCS Update comment Change lookup to Uint8Array as mentioned in pdf specs(these tests will pass after PR mozilla#8666 is merged). Added unit-tests for AlternateCS Resolved code-style issues Fixed code-style issues Addressed issues pointed out in mozilla#8611 (review)

Snuffleupagus added core 4-work-in-progress labels Jul 19, 2017

apoorv-mishra changed the title ~~TypeError fix in colorspace.js~~ Fix TypeError that occurs on calling base.getRgbBuffer(...) with 'lookup' as an 'Array' Jul 19, 2017

apoorv-mishra changed the title ~~Fix TypeError that occurs on calling base.getRgbBuffer(...) with 'lookup' as an 'Array'~~ Fix TypeError in colorspace.js that occurs on accidentally passing an 'Array' instead of 'TypedArray' Jul 19, 2017

yurydelendik requested changes Jul 19, 2017

View reviewed changes

apoorv-mishra changed the title ~~Fix TypeError in colorspace.js that occurs on accidentally passing an 'Array' instead of 'TypedArray'~~ Fix TypeError that occurs in colorspace.js on accidentally passing an 'Array' instead of 'TypedArray' Jul 19, 2017

apoorv-mishra force-pushed the fix-colorspace branch from 94895e7 to d8e5577 Compare July 20, 2017 00:48

yurydelendik reviewed Jul 20, 2017

View reviewed changes

yurydelendik approved these changes Jul 20, 2017

View reviewed changes

apoorv-mishra force-pushed the fix-colorspace branch from 4a5b49d to 7068c9b Compare July 20, 2017 19:39

apoorv-mishra force-pushed the fix-colorspace branch from 7068c9b to d14956d Compare July 20, 2017 19:49

timvandermeij removed the 4-work-in-progress label Jul 20, 2017

mozilla deleted a comment from pdfjsbot Jul 24, 2017

timvandermeij merged commit 44a5cec into mozilla:master Jul 25, 2017

apoorv-mishra deleted the fix-colorspace branch August 1, 2017 15:46

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018

Merge pull request mozilla#8666 from apoorv-mishra/fix-colorspace

7eb2e21

Fix TypeError that occurs in colorspace.js on accidentally passing an 'Array' instead of 'TypedArray'

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix TypeError that occurs in colorspace.js on accidentally passing an 'Array' instead of 'TypedArray' #8666

Fix TypeError that occurs in colorspace.js on accidentally passing an 'Array' instead of 'TypedArray' #8666

apoorv-mishra commented Jul 19, 2017

Snuffleupagus commented Jul 19, 2017

Snuffleupagus commented Jul 19, 2017 •

edited

Loading

apoorv-mishra commented Jul 19, 2017

yurydelendik left a comment

yurydelendik Jul 20, 2017

apoorv-mishra Jul 20, 2017

yurydelendik left a comment

yurydelendik commented Jul 20, 2017

timvandermeij commented Jul 24, 2017

pdfjsbot commented Jul 24, 2017

pdfjsbot commented Jul 24, 2017

timvandermeij commented Jul 24, 2017

pdfjsbot commented Jul 24, 2017

pdfjsbot commented Jul 24, 2017

pdfjsbot commented Jul 24, 2017

pdfjsbot commented Jul 24, 2017

timvandermeij commented Jul 25, 2017

Fix TypeError that occurs in colorspace.js on accidentally passing an 'Array' instead of 'TypedArray' #8666

Fix TypeError that occurs in colorspace.js on accidentally passing an 'Array' instead of 'TypedArray' #8666

Conversation

apoorv-mishra commented Jul 19, 2017

Snuffleupagus commented Jul 19, 2017

Snuffleupagus commented Jul 19, 2017 • edited Loading

apoorv-mishra commented Jul 19, 2017

yurydelendik left a comment

Choose a reason for hiding this comment

yurydelendik Jul 20, 2017

Choose a reason for hiding this comment

apoorv-mishra Jul 20, 2017

Choose a reason for hiding this comment

yurydelendik left a comment

Choose a reason for hiding this comment

yurydelendik commented Jul 20, 2017

timvandermeij commented Jul 24, 2017

pdfjsbot commented Jul 24, 2017

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jul 24, 2017

From: Bot.io (Linux m4)

Success

Published

timvandermeij commented Jul 24, 2017

pdfjsbot commented Jul 24, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented Jul 24, 2017

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jul 24, 2017

From: Bot.io (Linux m4)

Success

pdfjsbot commented Jul 24, 2017

From: Bot.io (Windows)

Success

timvandermeij commented Jul 25, 2017

Snuffleupagus commented Jul 19, 2017 •

edited

Loading