Closed Bug 966865 Opened 6 years ago Closed 6 years ago

Update gaia pdf.js to version 0.8.997

Categories

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

All
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:1.3+, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

RESOLVED FIXED
1.4 S1 (14feb)
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: njn, Assigned: bdahl)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [c=memory p= s= u=1.3] [MemShrink:P1])

Attachments

(2 files)

Updating pdf.js to 0.8.990 would give us some big memory wins:

- bug 878397
- bug 959925
- bug 962933
- bug 965130

Mozilla-central was just updated to 0.8.990 in bug 965861.
Keywords: perf
Summary: Update gaia pdf.js to version 0.8.990 → Update gaia pdf.js to version 0.8.997
pdf.js is broken in v1.4 but this work with v1.3 and should be uplifted.
Attachment #8369684 - Flags: review?(21)
(In reply to Brendan Dahl [:bdahl] from comment #1)
> Created attachment 8369684 [details] [review]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15928
> 
> pdf.js is broken in v1.4 but this work with v1.3 and should be uplifted.

Really? pdf.js was broken in bug 966132, but it was already fixed on 1/31. Is there some other error you are seeing?
Comment on attachment 8369684 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15928

I will buy some extra memory.
Attachment #8369684 - Flags: review?(21) → review+
(In reply to Jason Smith [:jsmith] from comment #2)
> Really? pdf.js was broken in bug 966132, but it was already fixed on 1/31.
> Is there some other error you are seeing?

I/Gecko   ( 2441): Attempting load of libEGL.so
E/Sandbox ( 2441): seccomp sandbox violation: pid 2441, syscall 281, args 2 1 6 1136621052 1186918144 1136596515.  Killing process.
I/Gecko   ( 1989): 
I/Gecko   ( 1989): ###!!! [Parent][MessageChannel] Error: Channel error: cannot send/recv
I/Gecko   ( 1989): 
E/Sensors ( 1989): sensor activate  handle 0 enable 1 drv 0
E/GeckoConsole( 1989): [JavaScript Error: "this.getContextForMM(...) is undefined" {file: "resource://gre/modules/DOMIdentity.jsm" line: 345}]
(In reply to Brendan Dahl [:bdahl] from comment #4)
> (In reply to Jason Smith [:jsmith] from comment #2)
> > Really? pdf.js was broken in bug 966132, but it was already fixed on 1/31.
> > Is there some other error you are seeing?
> 
> I/Gecko   ( 2441): Attempting load of libEGL.so
> E/Sandbox ( 2441): seccomp sandbox violation: pid 2441, syscall 281, args 2
> 1 6 1136621052 1186918144 1136596515.  Killing process.
> I/Gecko   ( 1989): 
> I/Gecko   ( 1989): ###!!! [Parent][MessageChannel] Error: Channel error:
> cannot send/recv
> I/Gecko   ( 1989): 
> E/Sensors ( 1989): sensor activate  handle 0 enable 1 drv 0
> E/GeckoConsole( 1989): [JavaScript Error: "this.getContextForMM(...) is
> undefined" {file: "resource://gre/modules/DOMIdentity.jsm" line: 345}]

Okay - can you get a bug on file for this? This appears to be different than bug 966132.
Whiteboard: [MemShrink] → [MemShrink:P1]
(In reply to Brendan Dahl [:bdahl] from comment #4)
> E/Sandbox ( 2441): seccomp sandbox violation: pid 2441, syscall 281, args 2 1 6 1136621052 1186918144 1136596515.  Killing process.

That's socket(AF_INET, SOCK_STREAM, IPPROTO_TCP), which a content process shouldn't be doing directly.  If this is reproducible using content-exposed interfaces, please file a bug.
(In reply to Jed Davis [:jld] from comment #6)
> That's socket(AF_INET, SOCK_STREAM, IPPROTO_TCP), which a content process
> shouldn't be doing directly.  If this is reproducible using content-exposed
> interfaces, please file a bug.

Do you have a device with v1.4 that you can try to open a pdf with?
I just tried on a Buri with v1.4 and I don't see that seccomp violation. The PDF loads -- the white progress bar makes its way across the top of the screen -- and then it reliably crashes just as it tries to actually paint, seemingly due to gralloc memory being exhausted.
What do you think about uplifting this under approval for 1.3 and 1.3t?  I'm not sure it should block, but would be nice to have.
Flags: needinfo?(fabrice)
Priority: -- → P1
Whiteboard: [MemShrink:P1] → [MemShrink:P1][c=memory u= p= s=]
I kicked a few of the travis jobs to see if they recover from the intermittent failures they got.
> What do you think about uplifting this under approval for 1.3 and 1.3t?

I certainly had that in mind when I filed the bug... though I failed to mark the appropriate flags.
(In reply to Ben Kelly [:bkelly] from comment #9)
> What do you think about uplifting this under approval for 1.3 and 1.3t?  I'm
> not sure it should block, but would be nice to have.

I'm fine with taking that on 1.3, yes.
blocking-b2g: --- → 1.3+
Flags: needinfo?(fabrice)
Whiteboard: [MemShrink:P1][c=memory u= p= s=] → [c=memory p= s= u=1.3] [MemShrink:P1]
Target Milestone: --- → 1.4 S1 (14feb)
I got travis green.  Any objections to landing?  The seccomp questions all answered?
(In reply to Ben Kelly [:bkelly] from comment #13)
> I got travis green.  Any objections to landing?  The seccomp questions all
> answered?

Merged to master: https://github.com/mozilla-b2g/gaia/commit/d15809c5db9f57869d2119aebf7c5bfb1908cf49
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Quick note, the PNG assets added in this bug had not been re-compressed prior to merging. Re-compressing them brings significant space savings. You can re-compress the images with the following command from the gaia root directory:

./tools/png_recompress.sh -v \
    apps/pdfjs/content/web/images/div_line_left.png \
    apps/pdfjs/content/web/images/div_line_left@1.5x.png \
    apps/pdfjs/content/web/images/div_line_left@2x.png \
    apps/pdfjs/content/web/images/div_line_right.png \
    apps/pdfjs/content/web/images/div_line_right@1.5x.png \
    apps/pdfjs/content/web/images/div_line_right@2x.png \
    apps/pdfjs/content/web/images/document_bg.png \
    apps/pdfjs/content/web/images/icon_next_page.png \
    apps/pdfjs/content/web/images/icon_next_page@1.5x.png \
    apps/pdfjs/content/web/images/icon_previous_page.png \
    apps/pdfjs/content/web/images/icon_previous_page@1.5x.png \
    apps/pdfjs/content/web/images/icon_zoom_in.png \
    apps/pdfjs/content/web/images/icon_zoom_in@1.5x.png \
    apps/pdfjs/content/web/images/icon_zoom_out.png \
    apps/pdfjs/content/web/images/icon_zoom_out@1.5x.png \
    apps/pdfjs/content/web/images/spinner.png \
    apps/pdfjs/content/web/images/toolbar_background.png
Assignee: nobody → bdahl
I was not able to uplift this bug to v1.3.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1.3, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1.3
  git cherry-pick -x -m1 d15809c5db9f57869d2119aebf7c5bfb1908cf49
  <RESOLVE MERGE CONFLICTS>
  git commit
Flags: needinfo?(bdahl)
All of the conflicts are images from https://github.com/mozilla-b2g/gaia/commit/99744b521b2eb1b7db64151259546437dd837ce5 which shouldn't have been merged without upstreaming the changes to the pdf.js repo.

Gabrieile, can you open a PR against the pdf.js repo with these changes?
Flags: needinfo?(bdahl) → needinfo?(gsvelto)
1.3 blockers no longer have auto-approval to land. Please request gaia-v1.3 approval on the patch for uplift.
(In reply to Brendan Dahl [:bdahl] from comment #18)
> All of the conflicts are images from
> https://github.com/mozilla-b2g/gaia/commit/
> 99744b521b2eb1b7db64151259546437dd837ce5 which shouldn't have been merged
> without upstreaming the changes to the pdf.js repo.

Sorry for having caused this issue, I had blanket-compressed all PNG files in gaia without realizing that the pdf.js ones had been imported from a separate repository.

> Gabrieile, can you open a PR against the pdf.js repo with these changes?

I've opened bug 972435 for this, I will post a PR there shortly.
Flags: needinfo?(gsvelto)
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #19)
> 1.3 blockers no longer have auto-approval to land. Please request gaia-v1.3
> approval on the patch for uplift.
Flags: needinfo?(bdahl)
NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):n/a
[User impact] if declined: outdated version of pdf.js
[Testing completed]: tested locally
[Risk to taking this patch] (and alternatives if risky): low risk, has been in master for awhile
[String changes made]: some, but pdf.js manages it's own string changes
Attachment #8380770 - Flags: approval-gaia-v1.3?
Flags: needinfo?(bdahl)
This is needed for 1.3T too!
(In reply to Nicholas Nethercote [:njn] from comment #23)
> This is needed for 1.3T too!

That is merge automatically from 1.3
Attachment #8380770 - Flags: approval-gaia-v1.3? → approval-gaia-v1.3+
v1.3: 6429297ba0694c05b312584a7e88d74773ccba0f
Duplicate of this bug: 822636
You need to log in before you can comment on or make changes to this bug.