If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

IonMonkey: Previously working pdf won't load

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: david.smitmanis, Assigned: nbp)

Tracking

({regression})

19 Branch
x86_64
Windows 7
regression
Points:
---

Firefox Tracking Flags

(firefox18-, firefox19+ fixed)

Details

(Whiteboard: [ion:p1:fx19], URL)

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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
(Reporter)

Updated

5 years ago
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.
(Reporter)

Updated

5 years ago
Keywords: regressionwindow-wanted
The first bad revision is:
changeset:   110833:e70b2e6a9207
user:        Nicolas B. Pierron <nicolas.b.pierron@mozilla.com>
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
Assignee: nobody → general
Component: PDF Viewer → JavaScript Engine
Product: Firefox → Core
tracking-firefox19: --- → ?
Keywords: regressionwindow-wanted
(Assignee)

Updated

5 years ago
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
(Assignee)

Comment 3

5 years ago
(In reply to Yury (:yury) from comment #2)
> The first bad revision is:
> changeset:   110833:e70b2e6a9207
> user:        Nicolas B. Pierron <nicolas.b.pierron@mozilla.com>
> 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 .
Depends on: 799818
Blocks: 799818
No longer depends on: 799818
(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?

Updated

5 years ago
tracking-firefox19: ? → +

Updated

5 years ago
tracking-firefox18: --- → +
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

Updated

5 years ago
Duplicate of this bug: 805463
Created attachment 675382 [details]
Minimal test case to replicate 'unsupported predictor' issue
Summary: Previously working pdf won't load → IonMonkey: Previously working pdf won't load
Whiteboard: [ion:p1:fx19]
(Assignee)

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
Created attachment 676414 [details] [diff] [review]
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.
Attachment #676414 - Flags: review?(hv1989)
(Assignee)

Comment 10

5 years ago
(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.
Attachment #676414 - Flags: review?(hv1989) → review+
Don't forget to add a minimal testcase!
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4770ab40cdb6
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
(Assignee)

Comment 14

5 years ago
(Try) https://tbpl.mozilla.org/?tree=Try&rev=1e163c83248f
https://hg.mozilla.org/integration/mozilla-inbound/rev/e436ff984d56
https://hg.mozilla.org/mozilla-central/rev/e436ff984d56
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox19: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
(Assignee)

Comment 16

5 years ago
Alex Keybl, why setting tracking-firefox18 if the original patch which caused this bug only appeared in fx19 ?
status-firefox19: fixed → ---
tracking-firefox18: + → ?
Target Milestone: mozilla19 → ---
(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.
tracking-firefox18: ? → -
I see this on FF 18 with Adobe Reader 9.0 http://imageshack.us/photo/my-images/266/ff18pdfissue.png/
Actually it's not related to pdf.js. Sorry

Updated

5 years ago
status-firefox19: --- → fixed
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?
You need to log in before you can comment on or make changes to this bug.