1064496 - Update pdf.js to version 1.0.801

Status: RESOLVED FIXED

(Firefox :: PDF Viewer, task, P2)

Description

[Jim Mathies [:jimm]]

+++ This bug was initially created as a clone of Bug #995431 +++

+++ This bug was initially created as a clone of Bug #990852 +++

We're going to bring the e10s changes down with a fresh merge. This sill also need to land with a patch from bug 942707.

Comment 1

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #0)
> +++ This bug was initially created as a clone of Bug #995431 +++
> 
> +++ This bug was initially created as a clone of Bug #990852 +++
> 
> We're going to bring the e10s changes down with a fresh merge. This sill
> also need to land with a patch from bug 942707.

s/this sill/this will

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Everything's landed upstream already?

Comment 3

[Jim Mathies [:jimm]]

Not yet apparently, yury is waiting on some reviews.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Changes since last update:
#5209 Set transformOrigin for text layer in css.
#5219 Avoid creating intermediate strings in sanitizeMetrics
#5232 Chrome extension: Isolate pageAction logic
#5240 In src/display/api.js, add documentation for the |progressCallback| parameter in |PDFJS.getDocument|
#5242 Fix for minutes tz calculation in document properties
#5237 Fix the placement of the findInput loading indicator in RTL locales
#5259 Handtool: Remove focus from previous node on click
#5251 Add basic support for ZapfDingbats
#5249 CCITTFaxStream parser: resolve xref if needed
#5260 Add more cases to |mapSpecialUnicodeValues| to fix the rendering of various Symbol encoded brackets
#5267 Version 1.0.712
#5226 Fix Zoom box resizing when it's hidden when the viewer loads (issue 5224)
#5248 Add getStats unit test
#5276 Fix handling of RGBA buffers in CalRGB colorspace (issue 5270)
#5235 Remove a duplicate PDF file from the test suite
#5221 Don't scale single-char text divs.
#5281 Fetches params in makeFilter
#5229 Auto zoom landscape documents (like slide presentations) to fit their height
#5271 Opera fixes
#5247 Fix links to wiki subsections and remove TextDecoder feature tests
#5284 Fetch decodeParams if it's a |Ref| in JBIG2Decode streams
#5289 Fixed typo in core/jpx.js #5227
#5287 Bug 1031612 - In PDF Viewer, the buggy XMP title "Untitled" overrides th...
#5286 Fix loading of inline JPEG images
#5254 Avoid showing 'blob..' as a title when using pdfjs from B2G
#5233 Workaround for TrueType fonts with exotic cmap tables (bug 1057544)
#5302 Removes examples from jsbin.com
#5275 Fix the exception propagation when rejecting workerReadyCapability
#5245 Further amend GlyphMapForStandardFonts (issue 5244)
#5306 Update zh-TW translation
#5305 Miscellaneous e10s fixes

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Attachment: Update pdf.js to version 1.0.794

https://tbpl.mozilla.org/?tree=Try&rev=4b319a652724

Comment 6

[Jim Mathies [:jimm]]

Thanks for working on this! I think you'll get try failures though, we're going to need this patch too - 

https://bugzilla.mozilla.org/attachment.cgi?id=8458044&action=diff

Comment 7

[Ryan VanderMeulen [:RyanVM]]

(In reply to Jim Mathies [:jimm] from comment #6)
> Thanks for working on this! I think you'll get try failures though, we're
> going to need this patch too - 
> 
> https://bugzilla.mozilla.org/attachment.cgi?id=8458044&action=diff

With that applied also:
https://tbpl.mozilla.org/?tree=Try&rev=343fcf7da46f

Comment 8

[Jim Mathies [:jimm]]

I guess e10s tests are still disabled, so both pushes will probably succeed. I'll need to land the browser.ini part of the tests patch and the e10s browser patch in bug 942707 in a follow up.

So assuming the first try push comes up green (probably will) you can land that, and I'll get the rest landed after the work here is on mc.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8491639 [details] [diff] [review]
Update pdf.js to version 1.0.794

This has changes that need Mossop's blessing as well.

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 8491639 [details] [diff] [review]
Update pdf.js to version 1.0.794

Review of attachment 8491639 [details] [diff] [review]:
-----------------------------------------------------------------

The amount of synchronous messaging here concerns me and I'd like to know that there re bugs on file to deal with that.

::: browser/extensions/pdfjs/content/PdfJs.jsm
@@ +42,5 @@
>  XPCOMUtils.defineLazyServiceGetter(Svc, 'pluginHost',
>                                     '@mozilla.org/plugin/host;1',
>                                     'nsIPluginHost');
> +XPCOMUtils.defineLazyModuleGetter(this, "BrowserUtils",
> +                                  "resource://gre/modules/BrowserUtils.jsm");

Seems to be unused

::: browser/extensions/pdfjs/content/PdfStreamConverter.jsm
@@ +51,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, 'PdfjsContentUtils',
> +  'resource://pdf.js/PdfjsContentUtils.jsm');
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "BrowserUtils",
> +  'resource://gre/modules/BrowserUtils.jsm');

Also unused

::: browser/extensions/pdfjs/content/PdfjsChromeUtils.jsm
@@ +31,5 @@
> +Cu.import('resource://gre/modules/XPCOMUtils.jsm');
> +Cu.import('resource://gre/modules/Services.jsm');
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "BrowserUtils",
> +                                  "resource://gre/modules/BrowserUtils.jsm");

Again, not used

@@ +138,5 @@
> +      // with the content process. _ppmm is currently the global process
> +      // manager, which means this is going to fire to every child process
> +      // we have open. Unfortunately I can't find a way to get at that
> +      // process specific mm from js.
> +      this._ppmm.broadcastAsyncMessage("PDFJS:Child:refreshSettings", {});

Unless you're going to track which of the child processes have pdfjs instances in them I don't think this is really a problem.

@@ +261,5 @@
> +   * Display a notification warning when the renderer isn't sure
> +   * a pdf displayed correctly.
> +   */
> +  _displayWarning: function (aMsg) {
> +    let json = aMsg.json;

json is deprecated, use aMsg.data, and throughout the rest of this patch

::: browser/extensions/pdfjs/content/PdfjsContentUtils.jsm
@@ +25,5 @@
> +Cu.import('resource://gre/modules/XPCOMUtils.jsm');
> +Cu.import('resource://gre/modules/Services.jsm');
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "BrowserUtils",
> +                                  "resource://gre/modules/BrowserUtils.jsm");

And again

@@ +115,5 @@
> +                       .getInterface(Ci.nsIDocShell)
> +                       .sameTypeRootTreeItem
> +                       .QueryInterface(Ci.nsIDocShell)
> +                       .QueryInterface(Ci.nsIInterfaceRequestor)
> +                       .getInterface(Ci.nsIContentFrameMessageManager);

I think this can be simplified to:

  let winmm = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
                     .getInterface(Ci.nsIDocShell)
                     .QueryInterface(Ci.nsIInterfaceRequestor)
                     .getInterface(Ci.nsIContentFrameMessageManager);

@@ +192,5 @@
> +                     "failed for unknown reasons.");
> +      return null;
> +    }
> +    return suitcase._findbar;
> +  }

What is the plan for removing all this CPOW usage?

Comment 11

[Jim Mathies [:jimm]]

(In reply to Dave Townsend [:mossop] from comment #10)
> > +XPCOMUtils.defineLazyModuleGetter(this, "BrowserUtils",
> > +                                  "resource://gre/modules/BrowserUtils.jsm");
> 
> Seems to be unused

My fault on this, I use dumpLn a lot for debugging but forgot to remove BrowserUtils from my patches.

> > +                     "failed for unknown reasons.");
> > +      return null;
> > +    }
> > +    return suitcase._findbar;
> > +  }
> 
> What is the plan for removing all this CPOW usage?

Why remove this? This seems like a perfect example of how cpows are incredibly useful. We don't have to add any apis outside of the extension code to get things working.

Comment 12

[Dave Townsend [:mossop]]

(In reply to Jim Mathies [:jimm] from comment #11)
> (In reply to Dave Townsend [:mossop] from comment #10)
> > > +XPCOMUtils.defineLazyModuleGetter(this, "BrowserUtils",
> > > +                                  "resource://gre/modules/BrowserUtils.jsm");
> > 
> > Seems to be unused
> 
> My fault on this, I use dumpLn a lot for debugging but forgot to remove
> BrowserUtils from my patches.
> 
> > > +                     "failed for unknown reasons.");
> > > +      return null;
> > > +    }
> > > +    return suitcase._findbar;
> > > +  }
> > 
> > What is the plan for removing all this CPOW usage?
> 
> Why remove this? This seems like a perfect example of how cpows are
> incredibly useful. We don't have to add any apis outside of the extension
> code to get things working.

Aren't CPOWs going away at some point? It's causing chrome to block content and vice-versa.

Comment 13

[Jim Mathies [:jimm]]

Comment on attachment 8491639 [details] [diff] [review]
Update pdf.js to version 1.0.794

Review of attachment 8491639 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/extensions/pdfjs/content/PdfjsChromeUtils.jsm
@@ +38,5 @@
> +XPCOMUtils.defineLazyServiceGetter(Svc, 'mime',
> +                                   '@mozilla.org/mime;1',
> +                                   'nsIMIMEService');
> +
> +/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

nit - somehow we got double headers in this file as well.

Comment 14

[Jim Mathies [:jimm]]

> > > What is the plan for removing all this CPOW usage?
> > 
> > Why remove this? This seems like a perfect example of how cpows are
> > incredibly useful. We don't have to add any apis outside of the extension
> > code to get things working.
> 
> Aren't CPOWs going away at some point? It's causing chrome to block content
> and vice-versa.

Spoke with Mossop, we'll file a follow up on trying to remove these and make the pdf.js code work with whatever new api we come up with for it.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Changes since last update:
#5315 Adds preprocessor directive to the X-Content-Security-Policy stuff
#5317 Bug 1064496 review changes
#5312 For |make firefox/mozcentral| builds, add cleanupJSSource to remove duplicate file headers in extensions/firefox/content/PdfjsChromeUtils.jsm

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Attachment: Update pdf.js to version 1.0.800

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8492375 [details] [diff] [review]
Update pdf.js to version 1.0.800

Carrying over Mossop's previous r+

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Attachment: Update pdf.js to version 1.0.800

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Yury noticed that a new file wasn't being packaged and fixed the upstream build script to include it.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Attachment: Update pdf.js to version 1.0.801

Comment 21

[Yury Delendik (:yury)]

Comment on attachment 8492392 [details] [diff] [review]
Update pdf.js to version 1.0.801

Thanks

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/8edf69df721b

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/8edf69df721b

Comment 24

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #14)
> > > > What is the plan for removing all this CPOW usage?
> > > 
> > > Why remove this? This seems like a perfect example of how cpows are
> > > incredibly useful. We don't have to add any apis outside of the extension
> > > code to get things working.
> > 
> > Aren't CPOWs going away at some point? It's causing chrome to block content
> > and vice-versa.
> 
> Spoke with Mossop, we'll file a follow up on trying to remove these and make
> the pdf.js code work with whatever new api we come up with for it.

filed bug 1071082

Comment 25

[Bobby Holley (:bholley)]

Hi Yury,

It looks like this patch added new non-trivial usage of __exposedProps__, which is verboten per [1]. Did that all happen in this bug, or is there another more-specific bug where this was discussed?

The normal policy on these is a backout, but given that this is on the critical e10s path, we'll probably want a less heavy-handed solution here (i.e. a prompt followup bug).

[1] https://groups.google.com/forum/#!msg/mozilla.dev.platform/yNKS1Z6UYQo/qloD9G0AdkcJ

Comment 26

[Jim Mathies [:jimm]]

(In reply to Bobby Holley (:bholley) from comment #25)
> Hi Yury,
> 
> It looks like this patch added new non-trivial usage of __exposedProps__,
> which is verboten per [1]. Did that all happen in this bug, or is there
> another more-specific bug where this was discussed?
> 
> The normal policy on these is a backout, but given that this is on the
> critical e10s path, we'll probably want a less heavy-handed solution here
> (i.e. a prompt followup bug).
> 
> [1]
> https://groups.google.com/forum/#!msg/mozilla.dev.platform/yNKS1Z6UYQo/
> qloD9G0AdkcJ

CPOWs rely on exposedprops for security. You should speak with billm on this I think.

Comment 27

[Bobby Holley (:bholley)]

(In reply to Jim Mathies [:jimm] from comment #26)
> CPOWs rely on exposedprops for security.

No, they rely on routing access from the child process to the parent process through an unprivileged scope in the parent. The only supported operation on any such object is CALL, which does not interact with __exposedProps__.

> You should speak with billm on this I think.

I have, at length.

Comment 28

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Sorry, my fault. I forgot that we're not supposed to use __exposedProps__. It looks like we can avoid it here without too much trouble. I filed bug 1074426.

Comment 29

[Yury Delendik (:yury)]

(In reply to Bobby Holley (busy) from comment #25)

> It looks like this patch added new non-trivial usage of __exposedProps__,
> which is verboten per [1]. Did that all happen in this bug, or is there
> another more-specific bug where this was discussed?
> 
> The normal policy on these is a backout, but given that this is on the
> critical e10s path, we'll probably want a less heavy-handed solution here
> (i.e. a prompt followup bug).

Addressed by bug 1074426.