1345328 - [jsplugins] way to build and pack JSPlugin to an XPI file

Status: RESOLVED FIXED

(Firefox :: PDF Viewer, enhancement)

Description

[Evelyn Hung]

Our target release format for JSplugin part is a system addon and thus it needs to be packed into an XPI file.

Comment 1

[Evelyn Hung]

@Ya-chieh, could you help out here? Thanks!

Comment 2

[Ya-Chieh Wu (inactive)]

(In reply to Evelyn Hung [:evelyn] from comment #1)
> @Ya-chieh, could you help out here? Thanks!
put in my bug list.


not sure if we need to pack into a .xpi as a system add-on. 
We have to ask people who work on add-on manager? 
also we probably need to do add-on signing.

Comment 3

[Evelyn Hung]

I'm sure we need to pack it into an xpi since it's listed on release document [1].

I will file another bug for signing xpi, which will be taken by operation of AMO team. Thanks for reminding me.

[1] http://gecko.readthedocs.io/en/latest/toolkit/mozapps/extensions/addon-manager/SystemAddons.html

Comment 4

[Ya-Chieh Wu (inactive)]

It seems that we don't need to pack into a .xpi as the other system add-ons in Firefox now. 
Or you want to place .xpi in certain place for some reasons? 

If the propose of this bug is to enable installing our system add-on by default, I think what we need is adding the setting into 'moz.build' as other cases in Firefox.

Comment 5

[Evelyn Hung]

(In reply to Ya-Chieh Wu from comment #4)
> It seems that we don't need to pack into a .xpi as the other system add-ons
> in Firefox now. 

hmm, how do you know that?

> Or you want to place .xpi in certain place for some reasons? 

No, I'm saying we need *a way* to build and pack into xpi, so release team can do that. Guess writing some build scripts?

> 
> If the propose of this bug is to enable installing our system add-on by
> default, I think what we need is adding the setting into 'moz.build' as
> other cases in Firefox.

Yeah, I guess a moz.build and maybe the rest of dependent files e.g jar.mn are what I was referring to. I think these are not for "enable installing our system add-on by default", are they? Guess they are just for build. 

The result I'm expecting is that release team could generate an xpi like other system add-ons in this release folder:
http://archive.mozilla.org/pub/system-addons/

Comment 6

[Ya-Chieh Wu (inactive)]

Attachment: wip.patch

This patch could install our add-on by default as a build-in add-on. But when pdfium launches, the paths of mRPCLibrary and mPluginLibrary are wrong which I still need to fix them.

Comment 7

[Ya-Chieh Wu (inactive)]

(In reply to Ya-Chieh Wu from comment #6)
> Created attachment 8847922 [details] [diff] [review]
> wip.patch
> 

By applying this patch, we don't need to put our mortar's path in the profile like we used did. 
This patch can build and install our mortar by default so you can ./mach run and find out that mortar is installed. 

About the paths issues, you can temporarily  copy "ppapi/out/*" and "plugin/" into obj-xxxdist/NightlyDebug.app/Contents/Resources/browser/features. I will find out what is the proper fix for the path issues.

btw above descriptions are all running on debug-build. I will test release-build and update on this bug.

Comment 8

[Ya-Chieh Wu (inactive)]

quickly update

I set my moz config to:  
mk_add_options MOZ_MAKE_FLAGS="-j4"
ac_add_options --enable-optimize --disable-debug

All the system add-ons are built into several folders but .xpi under the path of : 

obj-xxx/dist/Nightly.app/Contents/Resources/browser/features/

And this path is similar to official release Firefox's add-on's relative path.

Comment 9

[Ya-Chieh Wu (inactive)]

I found that our rpc.dylib can be indexed from obj-xxx/dist/Nightly.app/Contents/Resources/browser/ so we can change our setting in bootstrap.js to index to them.

Comment 10

[Ya-Chieh Wu (inactive)]

Attachment: modify-moz.build-to-build-pdfium-as-a-build-in-addon.patch

Hey Mike,

I saw you help to review lots of moz.build so I think you might be able to me. thanks in advance. 

In this patch, I want to enable our add-on as a build-in addon when "ac_add_options --enable-mortar" is set. 

Other than this, I have a question for you. In our add-on we need to build out a dynamic library[1]. However, I found that we build this library into a symbolic link under obj-xxx/dist/Nightly.app/Contents/Resources/browser/ which points to the real place under obj-xxx/browser/extensions/mortar/. 

Now, I am able to link my path in the addon to the symbolic link but should I build this library directly into our addon[2] or at least a hard link under under obj-xxx/dist/Nightly.app/Contents/Resources/browser/? How am I able to do that?

thank you Mike for your time.



[1] https://dxr.mozilla.org/mozilla-central/source/browser/extensions/mortar/moz.build#13
[2] obj-xxx/dist/Nightly.app/Contents/Resources/browser/features/[e-mail]

Comment 11

[Mike Hommey [:glandium]]

Now that you mention it, you can't really have a dynamic library in an addon. Well, you can, but then it needs to be extracted in the user profile and loaded from there. I don't think this is a desirable behavior for system addons, but maybe Dave has something to say here.

Comment 12

[Ya-Chieh Wu (inactive)]

Hi Dave,

Here are some background stories for you and you don't need to read from the first Comment.

We are trying to bring pdfium into Firefox and ship it as a system addon. As previous discussions[1], we need to build pdfium and pepperRpc libraries from our m-c which will be done in Bug 1345330. 

However, these two libraries are now built as two soft links under obj-xxx/dist/Nightly.app/Contents/Resources/browser/ which link to the hard links under obj-xxx/browser/extensions/mortar/. And we are able to link to the soft link from our addon and use them but we are now running on a debug build so it probably won't work for releasing. That's why we are seeking for suggestions about how to place these two libraries.

Could you share some advice of the feasible ways to place these two build-out libraries? Is it possible we built them directly into our addon? Or as Comment 11, we can place our libraries into the user profile but how to do that?

Thank you for your help.



[1] https://groups.google.com/a/mozilla.com/forum/?utm_medium=email&utm_source=footer#!msg/mortar-core/cAt3x4RtQR0/4UkztMJPCQAJ

Comment 13

[Dave Townsend [:mossop]]

You could mark the add-on to be installed unpacked, but we frown upon that for performance reasons and plus we're considering embedding system add-ons into the omni.ja so you're going to have to cope with the fact that individual files in the add-on don't exist on disk regardless.

If you want to be able to update the libraries with the add-on then I think your only choice is to extract them from the add-on to somewhere on disk before use. The profile directory seems like a sensible choice, just remember to remove them in the uninstall method of the add-on. You'll just need to use NetUtil or something to copy from the resource uri inside the add-on to a stream writing to a file on disk.

Comment 14

[Mike Hommey [:glandium]]

BTW, this is going to be sandboxed, right? because a dynamically loaded binary put in a user-owned directory is a call for malwares to replace the file...

Comment 15

[Dave Townsend [:mossop]]

That's a good point. Can we ensure the safety of this with a signature on the file or something?

Comment 16

[Ya-Chieh Wu (inactive)]

Hi Dave and Mike, thanks for the reply. 


I put my understanding into summaries here and if I am wrong, please correct me. That will help a lot.  

I have three ways on where to place the two libraries[1]:

(1) Place the libraries into .xpi
    pros: We can update the libraries with the add-on.
    cons: We need to extract them from .xpi and make one copy[2] into the profile directory to use them.

(2) Mark the add-on to be installed unpacked and place the libraries in it.
    pros: We can directly use the libraries.
    cons: Bad for performance reasons and will have omni.ja changes as Comment 13 mentioned.

(3) Built them out to objxx/dist/bin/pdfium
    This way wasn't mentioned above but I think it might work. 
    pros:  We don't need to extract the libraries from .xpi for using them. 
    cons:  We can't update the libraries with the add-on.


I will discuss with our team next week to see what we can do for placing the libraries.

thanks for Mike's and Dave's inputs again.

[1] pdfium and pepperRpc
[2] have security issues so we need to ensure the safety of this with a signature on the file

Comment 17

[Mike Hommey [:glandium]]

Note you also have the security issues with (2).

There might be option (4) put everything in libxul.

Comment 18

[Ya-Chieh Wu (inactive)]

Hey Evelyn,

Could you share your thoughts on Comment 16?

thx

Comment 19

[Ya-Chieh Wu (inactive)]

After today's weekly meeting, we think that shipping these two libraries within Firefox(option 3 or 4) is a better choice because we assume that these two libraries won't be updated frequently and by doing so, we can avoid to have security issues and performance issues if we ship them within add-on.  However, we will wait for Evelyn's feedback.

Comment 20

[Evelyn Hung]

(In reply to Ya-Chieh Wu from comment #19)
> After today's weekly meeting, we think that shipping these two libraries
> within Firefox(option 3 or 4) is a better choice because we assume that
> these two libraries won't be updated frequently 

I think pepperRpc and pepperPdfium are different cases on this argument. For pepperRpc it's true that we probably won't update it often since it will only need to be update when a certain set of Pepper APIs we used is changed. However, given that Pepper APIs are quite stable now (Google is not going to add more features as far as I know), so we do just need periodically updates from version to version.

However, pepperPdfium is different when you consider the reaction we need to take on 0-day. Here are things that should be done as soon as possible when we have 0-day: (1) pull in security fixed from upstream; (2) merge these changes into m-c and resolve conflicts if any; (3) roll out the update either by doing a dot-release of Firefox or by doing out-of-cycle system add-on update.

I feel (3) might be a minor issue here, because the most difficult part is (2) - how do we manage our source code to speed it up. We unfortunately don't have automation to do the merge, nor a better way of cutting down impacted files. We can only deal this manually case by case, which takes time. 1~2 days or even longer, I guess.

To sum up, I'm fine to put these binaries in libxul since it sounds easier now. This means we lose the out-of-cycle-update flexibility. We can review this after some releases, and put more effort on addressing security concerns and extract-to-disk issues if we have nothing to improve but speed up its roll-out-update time.

Comment 21

[Evelyn Hung]

(In reply to Evelyn Hung [:evelyn] from comment #20)
 > To sum up, I'm fine to put these binaries in libxul 

I meant *part of Firefox package*, not exactly packing in libxul as we want an isolated dynamic library - at least for pepperpdfium. pepperRpc can be in libxul I guess? It is really small.

Comment 22

[Ya-Chieh Wu (inactive)]

Attachment: 0001-Bug-1345328-modify-moz.build-to-build-pdfium-as-a-bu.patch

Hey mconley,

I saw you reviewing system add-on's codes so I think you might be able to help with reviewing this one.  

thank you.


result of try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eaf4c43830227fcc4c32e69776f3939f75f48f9c

Comment 23

[Evelyn Hung]

(In reply to Ya-Chieh Wu from comment #22)
> Created attachment 8858740 [details] [diff] [review]
> 0001-Bug-1345328-modify-moz.build-to-build-pdfium-as-a-bu.patch
> 
> Hey mconley,
> 
> I saw you reviewing system add-on's codes so I think you might be able to
> help with reviewing this one.  
> 
> thank you.
> 

I think :rhelmer will be able to review here since he is in charge of system add-ons.

Comment 24

[Robert Helmer [:rhelmer]]

Comment on attachment 8858740 [details] [diff] [review]
0001-Bug-1345328-modify-moz.build-to-build-pdfium-as-a-bu.patch

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

Overall this looks great! I've requested one very small change to the install manifest.

::: browser/extensions/mortar/host/pdf/install.rdf.in
@@ +11,3 @@
>    <Description about="urn:mozilla:install-manifest">
>      <em:id>[e-mail]</em:id>
> +    <em:version>1.0</em:version>

Please provide a name and short description - the name shows up in `about:support`

Comment 25

[Ya-Chieh Wu (inactive)]

Attachment: 0001-Bug-1345328-modify-moz.build-to-build-pdfium-as-a-bu.patch

Hey rhelmer,

thanks a lot for your help. Here is the updated version.

Comment 26

[Ya-Chieh Wu (inactive)]

Attachment: 0001-Bug-1345328-modify-moz.build-to-build-pdfium-as-a-bu.patch

result of try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=351ade7bb85ba2168c8abae46d5b180e27402492

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/085e1f764ab0
modify moz.build to build pdfium as a build-in addon. r=rhelmer

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/085e1f764ab0