804064 - IonMonkey: Previously working pdf won't load

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[david.smitmanis]

Try the pdf in the URL. With the current version of pdf.js (in Nightly or with add-on), it won't load. However it does work with the 16th October build of Nightly, before the update to 0.6.39. 

Error message is Error: Invalid PDF structure @ blob:fc6d83dd-c437-40bb-a9d1-aa4ffb633439:664

Comment 1

[Yury Delendik (:yury)]

Works on 10-20-2012 build (ff4af83233dc), stopped working on 10-21-2012 (abe709bfc49a). That excludes bug 801280 (update to 0.6.39) as a cause of the regression.

Comment 2

[Yury Delendik (:yury)]

The first bad revision is:
changeset:   110833:e70b2e6a9207
user:        Nicolas B. Pierron <[e-mail]>
date:        Fri Oct 19 16:45:17 2012 -0700
Bug 799818 part 2 - Handle unknown double as input of a table switch. r=djvj,h4writer

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to Yury (:yury) from comment #2)
> The first bad revision is:
> changeset:   110833:e70b2e6a9207
> user:        Nicolas B. Pierron <[e-mail]>
> date:        Fri Oct 19 16:45:17 2012 -0700
> Bug 799818 part 2 - Handle unknown double as input of a table switch.
> r=djvj,h4writer

I've checked the spew of the functions which are kept ion IonMonkey, there does not seems to be any register allocation issue before the TableSwitchV LIR instruction.  In addition, I checked all values flowing in this instruction while rendering this PDF, and all of them are integers.

My guess is that we still have an underlying issue which just got used and which is causing this to happen.  I noticed that we are having a probably unexpected handle exception path which is taken, which might correspond to the message printed by PdfJS.

I haven't been able to reproduce the differencial behaviour of this issue with by replacing the PDF in the octane version of PdfJS, but I haven't deeply investigated octane's PdfJS to understand what was going there.

If you really need to view this pdf in the mean time, you can disable ion.content option in about:config .

Comment 4

[Yury Delendik (:yury)]

(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #3)

> I haven't been able to reproduce the differencial behaviour of this issue
> with by replacing the PDF in the octane version of PdfJS, but I haven't
> deeply investigated octane's PdfJS to understand what was going there.

Octane's pdf.js test uses simple PDF to be rendered during test, which uses small subset of the pdf.js functionality. So it's more about the document that is rendered rather than pdf.js version.

> 
> If you really need to view this pdf in the mean time, you can disable
> ion.content option in about:config .

Here is another suspect for this bug http://www.irs.gov/pub/irs-pdf/f8802.pdf (from https://github.com/mozilla/pdf.js/issues/2301).

Is there a way to turn ion for particular page?

Comment 5

[Yury Delendik (:yury)]

The PDFs that fail use PNG predictor algorithm. The JS code fails at https://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/web/viewer.html?force=1#29740 :

29739       default:
29740         error('Unsupported predictor: ' + predictor);
29741     }

with 'Unsupported predictor: 1' or 'Unsupported predictor: 2' message, which is impossible since the switch has "case 1:" and "case 2:" branches.

If the line 29669 above is changed from "var predictor = this.stream.getByte();" to ""var predictor = this.stream.getByte() + 0;", the switch is properly executed.

Another PDF (more simple one) with the same problem can be found in bug 805463: http://samplepdf.com/sample.pdf

Comment 7

[Yury Delendik (:yury)]

Attachment: Minimal test case to replicate 'unsupported predictor' issue

Comment 8

[Nicolas B. Pierron [:nbp]]

(In reply to Yury (:yury) from comment #5)
> The PDFs that fail use PNG predictor algorithm. The JS code fails at
> https://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/
> content/web/viewer.html?force=1#29740 :
> 
> 29739       default:
> 29740         error('Unsupported predictor: ' + predictor);
> 29741     }
> 
> with 'Unsupported predictor: 1' or 'Unsupported predictor: 2' message, which
> is impossible since the switch has "case 1:" and "case 2:" branches.
> 
> If the line 29669 above is changed from "var predictor =
> this.stream.getByte();" to ""var predictor = this.stream.getByte() + 0;",
> the switch is properly executed.
> 
> Another PDF (more simple one) with the same problem can be found in bug
> 805463: http://samplepdf.com/sample.pdf

Thanks Yoric for locating one of the issues, I now a minimal test case.  The octane benchmark was infer to have a Value type, but only Integer values were flowing through the switch, which seemed to work fine for the test case.

I should be able to come with a fix soon.

Comment 9

[Nicolas B. Pierron [:nbp]]

Attachment: TableSwitchV, unbox integers values before branching.

Add missing unboxing causing the tag to be interpreted as an Int making branches default to 0x1fff1 case (i-e most of the time the default case).  Strangely octane benchmark did not complain about a bad result, as expected and commented in Bug 799818.  Thus I add the corresponding test case with the patch.

Comment 10

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #8)
> (In reply to Yury (:yury) from comment #5)
> > The PDFs that fail use PNG predictor algorithm. The JS code fails at
> > https://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/
> > content/web/viewer.html?force=1#29740 :
> > 
> > 29739       default:
> > 29740         error('Unsupported predictor: ' + predictor);
> > 29741     }
> > 
> > with 'Unsupported predictor: 1' or 'Unsupported predictor: 2' message, which
> > is impossible since the switch has "case 1:" and "case 2:" branches.
> > 
> > If the line 29669 above is changed from "var predictor =
> > this.stream.getByte();" to ""var predictor = this.stream.getByte() + 0;",
> > the switch is properly executed.
> > 
> > Another PDF (more simple one) with the same problem can be found in bug
> > 805463: http://samplepdf.com/sample.pdf
> 
> Thanks Yoric …

Thanks Yury …, sorry for miss typing/reading.

Comment 11

[Hannes Verschore [:h4writer]]

Don't forget to add a minimal testcase!

Comment 12

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4770ab40cdb6

Comment 13

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/cdaa193babf2 - Android wasn't happy with it, https://tbpl.mozilla.org/php/getParsedLog.php?id=16576586&tree=Mozilla-Inbound

Comment 14

[Nicolas B. Pierron [:nbp]]

(Try) https://tbpl.mozilla.org/?tree=Try&rev=1e163c83248f
https://hg.mozilla.org/integration/mozilla-inbound/rev/e436ff984d56

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e436ff984d56

Comment 16

[Nicolas B. Pierron [:nbp]]

Alex Keybl, why setting tracking-firefox18 if the original patch which caused this bug only appeared in fx19 ?

Comment 17

[Alex Keybl [:akeybl]]

(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #16)
> Alex Keybl, why setting tracking-firefox18 if the original patch which
> caused this bug only appeared in fx19 ?

Looks like a mistake - thanks for catching.

Comment 18

[Paul Silaghi, QA [:pauly]]

I see this on FF 18 with Adobe Reader 9.0 http://imageshack.us/photo/my-images/266/ff18pdfissue.png/

Comment 19

[Paul Silaghi, QA [:pauly]]

Actually it's not related to pdf.js. Sorry

Comment 20

[Paul Silaghi, QA [:pauly]]

http://atratus.se/david/20120921_se_stockholm.pdf
The pdf is loaded, but there is still an error notification: "this pdf document might not be displayed correctly". What's wrong?