1152841 - PDF.js in Firefox OS is crashing but then have graphic glitch

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::PDF Viewer, defect)

Description

[Clément Lefèvre]

Attachment: Screen in PDF.js and visible screen even if you click for homescreen

I just downloaded some PDFs I then got on the phone memory to read them later.

Then, by opening them through a file browser, it just happened to crash PDJ.js and create some graphic glitch making it impossible to access to the homescreen anymore.

It looks like it could be caused because it's opening the PDF inside the application.

Tested on

Build ID               20150408010203
Build Type             user
Gaia Revision          84cbd4391fb7175d5380fa72c04d68873ce77e6d
Gaia Date              2015-04-07 17:33:14
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/078128c2600a
Gecko Version          40.0a1
Build Name             KVT49L dev-keys
Device ID              flame
Firmware(Release)      4.4.2
Firmware(Incremental)  39
Firmware Date          Thu Oct 16 18:19:14 CST 2014
Bootloader             L1TC00011880

See attachments for screen views.
It was, for example, tested withis this PDF (http://www.assemblee-nationale.fr/14/pdf/ta-commission/r2697-a0.pdf), but I tried others and problem was the same.

Comment 1

[Clément Lefèvre]

Attachment: Visible screen in task manager after trying to open the PDF.

Comment 2

[Johan Lorenzo [:jlorenzo]]

Attachment: logcat with gaia debug enabled

Comment 3

[Johan Lorenzo [:jlorenzo]]

Reproduced on 2.2[1] with either the app "File manager" or "File" present on the Marketplace. 2.1[2] is not affected, you can close the PDF viewer without any glitch. However, the pdf file mentioned in comment 0 can't be visualized, the UI of the PDF viewer activity is not responsive. 

With both app, I see this particular line in the logcat. You have an example in attachment 8590798 [details]. 
W/PDF Viewer( 1324): [JavaScript Error: "Error: Invalid parameter array, need either .data or .url" {file: "app://pdfjs.gaiamobile.org/content/build/pdf.js" line: 254}]

In 2.1, I didn't see this line.

[1] Build ID               20150409162502
Gaia Revision          df0e04acad7c8c993f6ffe07b0ccb0ec20ee50bb
Gaia Date              2015-04-09 22:35:05
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/091b1cc1240b
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150409.200542
Firmware Date          Thu Apr  9 20:05:54 EDT 2015
Bootloader             L1TC000118D0


[2] Build ID               20150409161204
Gaia Revision          bbe983b4e8bebfec26b3726b79568a22d667223c
Gaia Date              2015-04-09 13:52:48
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/81a3aad1e6d7
Gecko Version          34.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20141120.194707
Firmware Date          Thu Nov 20 19:47:17 EST 2014
Bootloader             L1TC00011880

Comment 4

[Johan Lorenzo [:jlorenzo]]

[Blocking Requested - why for this release]: Regression which forces the user to reboot the phone to recover the homescreen.

Comment 5

[David Flanagan [:djf]]

When I follow the STR, the home button works fine for me, both in master and 2.2.

I can't actually view my PDF file, and I do see the error in the logcat, but the home button still works.  No reboot is required.

It would be nice to fix this, but since the home button does not actually seem to be broken anymore, maybe this does not need to be a blocker.

QA: could you test this again? Also, is it actually a regression? Do pdfs display correctly in 2.1 when opened via a file manager app?

Comment 6

[GitHub Autolander Bot]

Attachment: [gaia] davidflanagan:bug1152841 > mozilla-b2g:master

Comment 7

[David Flanagan [:djf]]

Johan: does this patch fix the bug for you?

Comment 8

[David Flanagan [:djf]]

Comment on attachment 8592523 [details] [review]
[gaia] davidflanagan:bug1152841 > mozilla-b2g:master

Aus,

Can you review this one-line patch for a 2.2+ blocker? Git blame says that you looked at this code less than a year ago.

The issue is that when 3rd party file manager apps use an open activity to view a PDF file, they send a blob and a filename, but not a URL. In this case fileURL is undefined in the activity handler but PDFViewerApplication.open() expects an object with a truthy originalURL property.  I opted for the simplest fix here of always passing a truthy value from the activity handler rather than trying to modify the open() method.

The README file for pdfjs says to modify the upstream repo, but I don't actually know if anyone is doing that.  It looks like the viewer.js upstream has the activity handler commented out, so maybe we just check this fix in directly?

Comment 9

[Brogan Zumwalt [Inactive]]

I am seeing the same results as David in comment 5 in Flame 3.0. I am seeing the error in logcat and the PDF indicated in comment 0 does not display, but the homescreen button works fine and there is no crash.

In Flame 2.1 the PDF does not open properly either from File Manager app (no crash though), but like 3.0 there is no issue with getting the home button to work. However, I do not see the same javascript error message in the log. There are a few other error messages that seem similar though:

04-14 17:00:17.444: E/GeckoConsole(1428): [JavaScript Error: "errorWrapper is null" {file: "app://pdfjs.gaiamobile.org/content/web/viewer.js" line: 4680}]
04-14 17:00:17.444: E/GeckoConsole(1428): [JavaScript Error: "NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "errorWrapper is null" {file: "app://pdfjs.gaiamobile.org/content/web/viewer.js" line: 4680}]'[JavaScript Error: "errorWrapper is null" {file: "app://pdfjs.gaiamobile.org/content/web/viewer.js" line: 4680}]' when calling method: [nsIDOMSystemMessageCallback::handleMessage]" {file: "jar:file:///system/b2g/omni.ja!/components/SystemMessageManager.js" line: 93}]


Device: Flame 3.0 Master
Build ID: 20150414072436
Gaia: c8cb0c0ebb8dd1f5c0c9037e38f8e4b237beb77b
Gecko: 388f5861dc7d
Version: 40.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

Device: Flame 2.1
Build ID: 20150414001210
Gaia: bbe983b4e8bebfec26b3726b79568a22d667223c
Gecko: 3e3cbe35bce3
Version: 34.0 (2.1)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Comment 10

[Johan Lorenzo [:jlorenzo]]

Thanks David for this patch, now I can see the PDF correctly displayed!

There is still the graphical glitch, though. It happens when the app gets closed like when you kill it with the task switcher. Here's a video showing the steps and the effects of it: http://mzl.la/1OAsj0c. Side note: I shot it before applying the patch. I tried the same steps with the patch applied, and I get into the same state. 

David, do you want to fix this separate issue in another bug?

Comment 11

[David Flanagan [:djf]]

Johan,

Yesterday I could not reproduce that other part of the bug, but today I can reproduce it, at least some of the time. That is much more than a "graphical glitch"!

It is a separate, more serious bug, I think. I'll file a new bug and will carry the 2.2+ forward on that new bug.

Comment 12

[David Flanagan [:djf]]

I've filed 1154838 for the window management part of this bug.

Comment 13

[Aus Lacroix [:aus]]

Comment on attachment 8592523 [details] [review]
[gaia] davidflanagan:bug1152841 > mozilla-b2g:master

the change looks good to me, does pdf.js have tests? If so, we should ideally have a test for this change before final commit. r+ assumes we'll do the right thing with the test. :)

Comment 14

[David Flanagan [:djf]]

There aren't any existing PDF viewer tests that I can see, so just landing this as-is.

Comment 15

[GitHub Autolander Bot]

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Comment 16

[David Flanagan [:djf]]

Okay, Autolander, I'll land it by hand...

https://github.com/mozilla-b2g/gaia/commit/6390c2289e9cb4c3d3152057cc4663ce812c2990

Comment 17

[David Flanagan [:djf]]

Note that this fix is only for the part of the bug where the PDF viewer does not display anything.

The window management issue has been spun off into bug 1154838.

Comment 18

[David Flanagan [:djf]]

Comment on attachment 8592523 [details] [review]
[gaia] davidflanagan:bug1152841 > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): not a regression; I don't think this has ever worked right

[User impact] if declined: users won't be able to view PDF files opened from 3rd party file manager apps.

[Testing completed]: locally

[Risk to taking this patch] (and alternatives if risky): not risky. This is a 2-line patch that just adds a fallback alternative in the case where the current code does not work. Given that the current code does not work, there is no way that this patch could make anything worse.

[String changes made]: none. When we have an unknown filename my patch purposely displays nothing instead of trying to add some kind of localized "unknown" string.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/8e24d8b7f5e7c74c3004b22710dda0dac3e04ead

Does something need to get upstreamed here?

Comment 20

[David Flanagan [:djf]]

As far as I can tell we've diverged from upstream and are no longer keeping the FirefoxOS pdf viewer app in sync with the offical pdf.js repo.  But I've emailed Brendan Dahl to ask if he's interested in this patch.

Comment 21

[David Flanagan [:djf]]

Though I suppose I should have just set needinfo for him here...

Comment 22

[Shally Li（leave from Mozilla）]

Per Comment 17 ("Note that this fix is only for the part of the bug where the PDF viewer does not display anything"), this bug has been successfully verified on latest Nightly Flame v2.2&3.0.
See attachment: verified_v2.2&3.0.mp4
Reproduce rate: 0/5

STR:
1.Download File manager app from Marketplace.
2.Copy PDF files to device's storage.
3.Open File manager,and select a PDF file to open (Or download a pdf file from browser,then open the file form notification bar).
**The PDF file shows its content normally.

-------------------------------------------------------
Device: Flame 2.2 build(Pass)
Build ID               20150416162504
Gaia Revision          d50b8a3919a7b4d8d289f150d3b9bed704ebafa9
Gaia Date              2015-04-16 21:46:57
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/5ebf32030512
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150416.195720
Firmware Date          Thu Apr 16 19:57:29 EDT 2015
Bootloader             L1TC000118D0

Device: Flame 3.0 build(Pass)
Build ID               20150416160206
Gaia Revision          3cd0a9facce26c2acc7be3755a17131a6358e33f
Gaia Date              2015-04-16 16:33:22
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/51e3cb11a258
Gecko Version          40.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150416.191700
Firmware Date          Thu Apr 16 19:17:10 EDT 2015
Bootloader             L1TC000118D0

Comment 23

[Shally Li（leave from Mozilla）]

Attachment: verified_v2.2&3.0.mp4

Comment 24

[Brendan Dahl [:bdahl]]

Just to reiterate what was mentioned in IRC. We're working on making pdf.js live on it's own in gaia and have it's own viewer and just provide a viewer API. We'll still try to sync up changes for the time being.

Filed: https://github.com/mozilla/pdf.js/issues/5982