Closed Bug 1152841 Opened 5 years ago Closed 5 years ago
.js in Firefox OS is crashing but then have graphic glitch
65.62 KB, image/png
28.28 KB, image/png
299.47 KB, text/x-log
46 bytes, text/x-github-pull-request
|Details | Review|
6.78 MB, video/mp4
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.
[Blocking Requested - why for this release]: Regression which forces the user to reboot the phone to recover the homescreen.
blocking-b2g: --- → 2.2?
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?
Johan: does this patch fix the bug for you?
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?
Attachment #8592523 - Flags: review?(aus)
QA Whiteboard: [QAnalyst-Triage?]
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
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?
Flags: needinfo?(jlorenzo) → needinfo?(dflanagan)
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.
I've filed 1154838 for the window management part of this bug.
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. :)
Attachment #8592523 - Flags: review?(aus) → review+
There aren't any existing PDF viewer tests that I can see, so just landing this as-is.
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.
Okay, Autolander, I'll land it by hand... https://github.com/mozilla-b2g/gaia/commit/6390c2289e9cb4c3d3152057cc4663ce812c2990
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 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.
Attachment #8592523 - Flags: approval-gaia-v2.2?(bbajaj)
Attachment #8592523 - Flags: approval-gaia-v2.2?(bbajaj) → approval-gaia-v2.2+
v2.2: https://github.com/mozilla-b2g/gaia/commit/8e24d8b7f5e7c74c3004b22710dda0dac3e04ead Does something need to get upstreamed here?
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.
Though I suppose I should have just set needinfo for him here...
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
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][MGSEI-Triage+]
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
You need to log in before you can comment on or make changes to this bug.