1115197 - Make JS callers of ios.newChannel call ios.newChannel2 in browser/extensions/pdfjs

Status: RESOLVED FIXED

(Firefox :: PDF Viewer, defect, P2)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_3_c_browser_pdfjs.patch

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Whoever ends up reviewing those patches, please find useful documentation here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1087726#c2

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8540999 [details] [diff] [review]
js_3_c_browser_pdfjs.patch

I don't think it makes sense to use "frontWindow" here (that is an arbitrary window). In fact I'm not sure why that is used at all, instead of this.domWindow.

Comment 4

[Yury Delendik (:yury)]

We are trying to support the PDF.js extension (Firefox PDF Viewer is built on it) for testing and development purpose. We are trying to patch code at https://github.com/mozilla/pdf.js/tree/master/extensions/firefox/content and then uplift it to the m-c.

Notice that we are trying to support the extension for older Firefox versions. Do newChannel2 and asyncFetch2 present and usable starting from FF36? What will be best way to feature test it?

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Yury Delendik (:yury) from comment #4)
> We are trying to support the PDF.js extension (Firefox PDF Viewer is built
> on it) for testing and development purpose. We are trying to patch code at
> https://github.com/mozilla/pdf.js/tree/master/extensions/firefox/content and
> then uplift it to the m-c.
> 
> Notice that we are trying to support the extension for older Firefox
> versions. Do newChannel2 and asyncFetch2 present and usable starting from
> FF36? What will be best way to feature test it?

Yuri, thanks for checking back. At the moment the arguments (loadingPrincipal, loadingNode, etc.) will only hang off a channel and will *not* really be used for some time. Once we have a loadInfo on all channels, we are going to move security checks into asyncOpen(). Once that infrastructure is in place, we are going to consume the arguments you provide when creating a channel. I don't think you can feature test at the moment.

Worth mentioning is, whenever some addon still calls ioservice::NewChannel() instead of ioservice::NewChannel2() - we forward that call (see [1]) and then try to create a channel using either protocolhandler::NewChannel2() or protocolhandler::NewChannel(), see [2]. In easier words, we provide a wrapper (shim) that allows addons to still work even if not calling ioservice::NewChannel2().

The rule of thumb should be: All of the code we (like we as Mozilla) write, should call ::NewChannel2() instead of ::NewChannel().

To answer your question, newChannel2 and also asyncFetch2 landed on mc recently (first being tested with Bug 1087739, which landed on 12/29 and is therefore a milestone for FF37.


 



[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsIOService.cpp#804
[2] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsIOService.cpp#647

Comment 6

[Brendan Dahl [:bdahl]]

Hi Christoph, can you create a pull request against the pdf.js repo https://github.com/mozilla/pdf.js as well?

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Brendan Dahl [:bdahl] from comment #6)
> Hi Christoph, can you create a pull request against the pdf.js repo
> https://github.com/mozilla/pdf.js as well?

Here we go: https://github.com/mozilla/pdf.js/pull/5620

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

As discussed over email I am assigning this bug to you. Thanks for fixing!

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8540999 [details] [diff] [review]
js_3_c_browser_pdfjs.patch

Clearing review request since the patch is slighlty outdated.

Comment 10

[Yury Delendik (:yury)]

https://github.com/mozilla/pdf.js/pull/5819

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Yury, thanks for fixing and landing on github! When is this code actually merged into mc?

We are about to land a channel wrapper (bug 1120487) which wraps all channels that do not call newChannel2 early in the next cycle (early April) which is very likely to cause problems for pdf.js.

Comment 12

[Jonas Jenwald [:Snuffleupagus]]

Christoph, this will land on mozilla-central as part of bug 1148192.