[Mortar] mozbuild for PDFium and its Pepper layer from Chromium

RESOLVED WONTFIX

Status

()

enhancement
P2
major
RESOLVED WONTFIX
2 years ago
2 months ago

People

(Reporter: jj.evelyn, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [pdfium-release])

Attachments

(7 attachments, 8 obsolete attachments)

4.96 MB, application/zip
Details
16.55 KB, patch
Details | Diff | Splinter Review
23.14 KB, patch
Details | Diff | Splinter Review
54.90 KB, patch
Details | Diff | Splinter Review
128.04 KB, text/plain
Details
205 bytes, text/plain
Details
38.20 KB, text/plain
Details
Reporter

Description

2 years ago
No description provided.
Reporter

Comment 1

2 years ago
According to this: http://archive.mozilla.org/pub/firefox/releases/52.0/

I guess we need at least the following versions:

- linux-i686/ 		
- linux-x86_64/ 		
- mac
- win32/ 		
- win64/
Reporter

Updated

2 years ago
Duplicate of this bug: 1273059
Copy necessary source codes from chromium code base for PDFium and the PPAPI wrapper. (p.s. The file size is above the attachment limit, so I upload the compressed version.)
Copy auto-generated codes from the working directory of chromium for PDFium and the PPAPI wrapper.
Add necessary mozbuild files for compiling the pepperpdfium library.
Integrate mozbuild files of pepperpdfium into the whole build system.
Posted file mortar_copied_filelist.txt (obsolete) —
This is a file list which contains all copied files from chromium code base, and the corresponding result is attachment 8845828 [details].
Posted file mortar_generated_filelist.txt (obsolete) —
This is a file list which contains necessary auto-generated files from chromium working directory, and the corresponding result is attachment 8845829 [details] [diff] [review].
Posted file mortar_added_filelist.txt (obsolete) —
This is a file list which contains mozbuild related files (i.e. moz.build, *.mozbuild, and other files for working around) for building pepperpdfium by our own build system, and the corresponding result is attachment 8845831 [details] [diff] [review].
Posted patch pepperpdfium.diff (obsolete) — Splinter Review
This diff contains the necessary workaround to make chromium codes able to be built by our build system. And attachment 8845828 [details] is the result after applying this diff to those files listed in attachment 8845840 [details].
(In reply to Bruce Sun [:brsun] from comment #7)
> Created attachment 8845840 [details]
> mortar_copied_filelist.txt
> 
> This is a file list which contains all copied files from chromium code base,
> and the corresponding result is attachment 8845828 [details].

Sorry that the later part of the statement above is not correct. attachment 8845828 [details](0001_pepperpdfium_buildsystem_add_chromium_files.patch.tar.gz) is the result after applying attachment 8845849 [details] [diff] [review](pepperpdfium.diff) to the files listed in attachment 8845840 [details](mortar_copied_filelist.txt).
Reporter

Comment 12

2 years ago
Could you also list the built binaries size? Thanks!
These codes are based on this commit of chromium: 2193fbd54c6970038fbfb920b7521bbe0a5acc15
(In reply to Evelyn Hung [:evelyn] from comment #12)
> Could you also list the built binaries size? Thanks!

Sure. The size of built binaries on Windows and OSX are getting smaller after using mozbuild. But the binary size on Ubuntu seems very weird, since it getting a way larger than before. There might be some compiler flags or linker flags I should adopt correctly in order to reduce the file size. I will try to figure that out.

Windows:
 - rpc.dll: 0.766 MB
 - pepperpdfium.dll: 3.103 MB

Ubuntu:
 - librpc.so: 6.5 MB
 - libpepperpdfium.so: 69.5 MB

OSX:
 - librpc.dylib: 2.8 MB
 - libpepperpdfium.dylib: 5.7 MB

Comparing to the original binary size using GN:
 - pepperpdfium.dll: 6.326 MB (Windows)
 - libpepperpdfium.so: 7.053 MB (Ubuntu)
 - libpepperpdfium.dylib: 13.219 MB (OSX)
Comment on attachment 8845831 [details] [diff] [review]
0003_pepperpdfium_buildsystem_add_mozbuild_files.patch

Hi Mike,

Based on the discussion[1][2], we are going to add PDFium and PPAPI wrapper codes into our code base. All these newly added files will be located under 'pepperpdfium' sub-folder of 'mortar' browser extension. Currently, the binary will be built only if '--enable-mortar' config has been enabled locally.

I am still trying to figure out how to reduce the binary size on Ubuntu as stated on comment 14, but probably it wouldn't impact much on the structure of moz.build contained in this patch.

Would you mind having a glance on these newly added moz.build files and sharing your thoughts?

p.s. I am not sure whether it is worth to be mentioned, but I've experienced one extreme case. If I use the directory too deep from the root folder to contain Gecko source files, our 'pepperpdfium' would fail to be built. One simple workaround to fix this problem is to avoid using a very deep directory (ex. D:\Source\gecko\mozilla-central-working) to contain Gecko files locally; using a less deeper directory (ex. D:\Source\mozilla-central-working) instead. Probably we need to check the configuration of our CI servers (ex. try-server) to make it work as well. Another workaround I've made for this path-length issue within our moz.build is to avoid using 'FINAL_LIBRARY' to generate intermediate libraries because the actual file name of each intermediate library is the concatenation of all directory names from 'topsrcdir' down to the current folder of moz.build file. I explicitly group files into several static libraries first, and then I link them altogether into one single dynamic library later.

[1] https://groups.google.com/a/mozilla.com/forum/?utm_medium=email&utm_source=footer#!msg/mortar-core/cAt3x4RtQR0/4UkztMJPCQAJ
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1325032#c11
Attachment #8845831 - Flags: feedback?(mh+mozilla)
Comment on attachment 8845834 [details] [diff] [review]
0004_pepperpdfium_buildsystem_add_mozbuild_dependency.patch

And moz.build files in this patch as well.
Attachment #8845834 - Flags: feedback?(mh+mozilla)
Comment on attachment 8845828 [details]
0001_pepperpdfium_buildsystem_add_chromium_files.patch.tar.gz

Hi Peter,

Would you mind have a glance on these files? These files should be the same as the outcome stated on comment 11.
Attachment #8845828 - Flags: feedback?(peterv)
Comment on attachment 8845829 [details] [diff] [review]
0002_pepperpdfium_buildsystem_add_chromium_autogen_files.patch

And this one.
Attachment #8845829 - Flags: feedback?(peterv)
Comment on attachment 8845849 [details] [diff] [review]
pepperpdfium.diff

And this one.
Attachment #8845849 - Flags: feedback?(peterv)
Reporter

Updated

2 years ago
Blocks: 1347419
Reporter

Updated

2 years ago
Whiteboard: [pdfium-release]
After syncing compiler and liner options from BUILD.gn, now the file size of pepperpdfium binaries on three platforms becomes:
 - pepperpdfium.dll (Windows): 3.05 MB
 - libpepperpdfium.so (Ubuntu): 4.5 MB
 - libpepperpdfium.dylib (OSX): 4.5 MB
I've tried to build pepperpdfium on our try server, but there are some errors[1][2] needed to be fixed or worked-around further, such as:
 - |check_stdcxx| fails:
   = _ZNKSt8__detail20_Prime_rehash_policy14_M_need_rehashEjjj@GLIBCXX_3.4.18
   = _ZNKSt8__detail20_Prime_rehash_policy11_M_next_bktEj@GLIBCXX_3.4.18
 - Static analysis fails
 - undefined reference to `__libc_stack_end'
 - etc.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=526d4ccb85ea4429872d1c48e1f39723715164ab
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=6db4431f408c37636440bcc012671068a1cd037e
Comment on attachment 8845831 [details] [diff] [review]
0003_pepperpdfium_buildsystem_add_mozbuild_files.patch

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

From a 10000ft view:
- You're apparently trying to replicate the build flags used in the chromium build. Don't do that. Leave it to Firefox's build system to choose the optimization flags, and most other flags. If you get build failures because of warnings as errors, use ALLOW_COMPILER_WARNINGS = True.
- There is no explanation about why the forced includes are there, and adding them to the build flags of source files independently doesn't seem very appealing. I'd rather avoid them altogether if possible.
- It looks like this is going to include libjpeg, libpng, zlib, freetype, all of which we have a copy in the tree and that we're shipping in libxul (except for freetype, where it's only there on Android ; on Linux, we use the system library). Also, I don't know about the others, since the paths are not very verbose about it, but the zlib version is clearly old and has known security vulnerabilities.
- It looks like this includes lcms. IIRC, Firefox used to use that before qcms, and, IIRC again, one of the reasons to stop using lcms was the security vulnerabilities.
- Looking at some of the source file names, it seems this would be importing a *fourth* version of the chromium base code. (We have one for ipc, one for sandbox, and one for webrtc already). That doesn't really sound good.
Attachment #8845831 - Flags: feedback?(mh+mozilla) → feedback-
Comment on attachment 8845834 [details] [diff] [review]
0004_pepperpdfium_buildsystem_add_mozbuild_dependency.patch

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

Why isn't this part of the other patch?
Attachment #8845834 - Flags: feedback?(mh+mozilla)

Updated

2 years ago
Depends on: 1350261

Updated

2 years ago
Depends on: 1350262
Summarize what I found recently regarding to the status of the try:
 - Some workaround done within attachment 8845831 [details] [diff] [review] is caused by the dependency of STL headers on Windows platform. I further narrow down the use case and fire the corresponding bug 1350261. This issue can be workaround by including <xutility> at the beginning of each source file which is effected.
 - Some unordered_map operations cause check_stdcxx to complain about @GLIBCXX_3.4.18 symbols on the try server. I further narrow down the use case and fire the corresponding bug 1350262. This issue seems a little difficult to be workaround if we don't want to modify third_party codes very much.
 - There are some limitations if we build with AddressSanitizer configuration[1][2]. It seems that we cannot strip unused symbols when AddressSanitizer is enabled, and we have to add more source files from third_party codebase. There might be two options we can choose: one is to add more files from third_party codebase to make linker happy; another one is to totally disable mortar when we detect CONFIG['MOZ_ASAN'] within moz.build.
 - There are lots errors if we enable static analysis[1]. Probably there is some way to partially disable it under some specific folder, but I haven't found such configuration yet unfortunately. There might be two options we can choose: one is to fix the error one by one, which means lots of modifications on third_party codebase; another one is to totally disable mortar when we detect CONFIG['ENABLE_CLANG_PLUGIN'].
 - Regarding to '__libc_stack_end' I mentioned comment 21, I haven't narrow down the use case yet. Probably this is caused by configurations from BUILD.gn. I will try to remove those options as suggested to see if there are any clues.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dd197625c28a691ebbbb0d79472a6688be1f149
[2] https://github.com/google/sanitizers/issues/260
Thank you Mike. I forgot to say that our original idea is to modify third party codes as less as possible, so that we could keep these codes up-to-date more easily. So if we have any chance to workaround something without modifying the source, I tend to do so in that way.

(In reply to Mike Hommey [:glandium] from comment #22)
> Comment on attachment 8845831 [details] [diff] [review]
> 0003_pepperpdfium_buildsystem_add_mozbuild_files.patch
> 
> Review of attachment 8845831 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> From a 10000ft view:
> - You're apparently trying to replicate the build flags used in the chromium
> build. Don't do that. Leave it to Firefox's build system to choose the
> optimization flags, and most other flags. If you get build failures because
> of warnings as errors, use ALLOW_COMPILER_WARNINGS = True.

Got it. I will try to remove those optimization flags from BUILD.gn to see how it works. ALLOW_COMPILER_WARNINGS = True does help us pass through some errors, but there are more errors we have to deal with as mentioned on comment 24.

> - There is no explanation about why the forced includes are there, and
> adding them to the build flags of source files independently doesn't seem
> very appealing. I'd rather avoid them altogether if possible.

Most of these workaround have been narrowed down as bug 1350261. But I'm afraid that I don't have enough insight about how our STL wrapper works, any guidance or help would be appreciated.

> - It looks like this is going to include libjpeg, libpng, zlib, freetype,
> all of which we have a copy in the tree and that we're shipping in libxul
> (except for freetype, where it's only there on Android ; on Linux, we use
> the system library). Also, I don't know about the others, since the paths
> are not very verbose about it, but the zlib version is clearly old and has
> known security vulnerabilities.

I would give it a quick try to see how hard it would be to switch from these additional libraries to Gecko's ones.

> - It looks like this includes lcms. IIRC, Firefox used to use that before
> qcms, and, IIRC again, one of the reasons to stop using lcms was the
> security vulnerabilities.

I guess it would be a little bit hard to switch CMS used in PDFium comparing to switching other libraries. I would discuss this suggestion with our teammates.

> - Looking at some of the source file names, it seems this would be importing
> a *fourth* version of the chromium base code. (We have one for ipc, one for
> sandbox, and one for webrtc already). That doesn't really sound good.

If we would like to keep our 'pepperpdfium' library up-to-date with less pain, there might be a trade-off. Forking chromium's repo and building pepperpdfium library directly from it without checking in anything into m-c could be a quick solution, but this might not be a favored approach regarding to our convention. Merging other chromium base codes on m-c into one would be more elegant, but the potential risks to break existing functions could be very scary. Any suggestions?

(In reply to Mike Hommey [:glandium] from comment #23)
> Comment on attachment 8845834 [details] [diff] [review]
> 0004_pepperpdfium_buildsystem_add_mozbuild_dependency.patch
> 
> Review of attachment 8845834 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why isn't this part of the other patch?

Because 'rpc' and 'pepperpdfium' are two distinct dynamic libraries. The previous patch is to establish proper moz.build for 'pepperpdfium' only. I can merge these two patches if you prefer to see them altogether in one.
Flags: needinfo?(mh+mozilla)
> > - It looks like this includes lcms. IIRC, Firefox used to use that before
> > qcms, and, IIRC again, one of the reasons to stop using lcms was the
> > security vulnerabilities.
> 
> I guess it would be a little bit hard to switch CMS used in PDFium comparing
> to switching other libraries. I would discuss this suggestion with our
> teammates.
> 

Hi Evelyn,

Thanks for your sharing in the private channel. Would you mind sharing your investigation here again about the security issues that PDFium has been addressed for lcms, and the difference of ICC spec support between lcms and qcms?
Flags: needinfo?(ehung)
Reporter

Comment 27

2 years ago
(In reply to Mike Hommey [:glandium] from comment #22)
> - It looks like this includes lcms. IIRC, Firefox used to use that before
> qcms, and, IIRC again, one of the reasons to stop using lcms was the
> security vulnerabilities.

I saw there are a few patches in chromium codebase for addressing security and memory leak issues of lcms. 

https://cs.chromium.org/chromium/src/third_party/pdfium/third_party/lcms2-2.6/README.pdfium

I'm not sure if these ease our concerns. Jeff, could you take a look? Thank you so much.

I also had a hallway talk to Jeff, we are thinking it might be reasonable if we just keep lcms for now (if security vulnerabilities had been fixed), and ask PDFium team to understand if they have any plan to migrate to qcms, given the fact that there is a copy of qcms in chromium codebase.

A side note of PDFium color management test by opening this pdf[1] on Chrome: PDFium seems only supporting to ICC2, not up to ICC4.

[1] http://www.color.org/version4pdf.pdf
Flags: needinfo?(ehung) → needinfo?(jmuizelaar)
If Chrome is exposing lcms I'm ok with exposing it at least until we have a reason to believe that security is below what we need.
Flags: needinfo?(jmuizelaar)
Flags: needinfo?(mh+mozilla)
Update:

After adding more files, ASAN build of mortar on Linux can be passed on the try server[1]. However, the hazard build always fails for mortar, and I don't have any ideas about how to fix it yet.

I can build mortar locally with libjpeg, libpng, zlib, freetype inside Gecko code base. But it seems freetype cannot be built on Windows trivially, and libpng lacks some symbols on the try server[2]. Btw, thanks to :lochang's efforts, we found a nicer way to workaround the static analysis failures by modifying clang-plugin.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=b749f02dea50f435451f25978fd8d7171295689d
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=433dbf1855c680e0aa6714f38520e7070412a84a
> I can build mortar locally with libjpeg, libpng, zlib, freetype inside Gecko
> code base.
on my Ubuntu machine.
(In reply to Bruce Sun [:brsun] from comment #31)
> Hi Steve,
> 
> We've hit one hazard failure on the try server[1][2]: assert(callee.Kind ==
> "Drf");[3]
> Would you mind sharing what could be possible causes to this failure?

I'll look at the logs, but I can tell you right away that this is a bug in the analysis itself. It's seeing a control flow graph structured in a way that it doesn't know how to handle. (Which is a first -- there've been plenty of other sorts of unhandled things, but I've never hit this before.)

Hopefully I can just pull your rev down from try to reproduce, but if not I'll have to ask you to re-push to try with an extra flag (--upload-xdbs) to give me what I need to work on this.
Ok, I'm good. <https://treeherder.mozilla.org/#/jobs?repo=try&revision=f49776e925b9bfb64a05af18faef90b0cb4a4ed8>. Hopefully it'll be straightforward once I have those.
Depends on: 1356706
Thanks Steve,

Hazard build is green now[1]. Clear the ni flag for you. Please feel free to share your comment if any.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=91c791ed5b286c22558075df18e4ba29ca36e0af
Flags: needinfo?(sphink)
It was code using something similar to

  typedef void(*func_t)();
  ((func_t*)189)();

to force a crash. The analysis didn't expect a Call to contain an integer as its argument.
Attachment #8845828 - Attachment is obsolete: true
Attachment #8845829 - Attachment is obsolete: true
Attachment #8845831 - Attachment is obsolete: true
Attachment #8845834 - Attachment is obsolete: true
Attachment #8845840 - Attachment is obsolete: true
Attachment #8845844 - Attachment is obsolete: true
Attachment #8845847 - Attachment is obsolete: true
Attachment #8845849 - Attachment is obsolete: true
Attachment #8845828 - Flags: feedback?(peterv)
Attachment #8845829 - Flags: feedback?(peterv)
Attachment #8845849 - Flags: feedback?(peterv)
Attachment #8858992 - Flags: review?(peterv)
This file is just for reference. The contents contains a file list which we would copy from chromium/src.
This file is just for reference. The contents contains a file list which we would copy from chromium/out.
The build script contains in attachment 8858995 [details] [diff] [review] uses libjpeg, libpng, zlib, freetype2 within Gecko, so the corresponding files under |pdfium/third_party| are removed from attachment 8858992 [details] as well.

Bug 1350261 can be avoided by setting |DISABLE_STL_WRAPPING| as |True| in the build script. So currently the build results on the try server are all green[1] except for bug 1350262.

I've also quickly tried the built binaries on my Windows, Ubuntu, OSX machine locally, and I haven't found any obvious issues yet.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=abf608e93425b07ea857a7fe0954094d0d5e4aec
Comment on attachment 8858995 [details] [diff] [review]
0004_pepperpdfium_buildsystem_add_mozbuild_files.patch

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

Please split out the clang plugin changes, and file a separate bug for the freetype changes. Please also don't keep freetype building with its own build system on android if you're going to have it built through moz.build on (some) other platforms.

::: browser/extensions/mortar/host/moz.build
@@ +8,5 @@
> +    '..',
> +]
> +
> +SOURCES += [
> +    'rpc.cc',

Doesn't this conflict with bug 1347440?

@@ +15,5 @@
> +USE_LIBS += [
> +    'mozglue',
> +]
> +
> +SharedLibrary('rpc')

Seems like you want a GeckoSharedLibrary, and not a USE_LIBS with mozglue in it.

::: browser/extensions/mortar/pepperpdfium/base/moz.build
@@ +6,5 @@
> +
> +# libevent
> +if CONFIG['OS_TARGET'] != 'WINNT' and not CONFIG['MOZ_SYSTEM_LIBEVENT']:
> +    LOCAL_INCLUDES += [
> +        '../../../../../ipc/chromium/src/third_party/libevent',

Please use "absolute" paths. (absolute paths are relative to the top source directory)

::: browser/extensions/mortar/pepperpdfium/third_party/pdfium/third_party/moz.build
@@ +89,5 @@
> +    'media_libjpeg',
> +    # replacement for fx_lpng
> +    'mozpng',
> +    # replacement for fx_zlib
> +    'zlib',

While this avoids the duplication of code in the tree, this still duplicates the code in Firefox: one in libxul, one in libpepperpdfium. This is certainly not desirable on Android, but I don't know if this is supposed to build on Android. This would be rather unwanted on desktop, but someone up the management ladder could convince me it's okay.

You'll also need to update toolkit/content/license.html.

Also note that some of the symbols in some of those libraries are renamed in Gecko (I'm sure that's the case for zlib where they are prefixed with MOZ_Z_, but that's automatically done if you include the normal zlib headers). Please double check you're using the right symbols for those libraries.
Attachment #8858995 - Flags: review?(mh+mozilla)

Updated

2 years ago
Depends on: 1358023
Thanks Mike, I will modify patches according to your comments.

> Please split out the clang plugin changes, and file a separate bug for the
> freetype changes. Please also don't keep freetype building with its own
> build system on android if you're going to have it built through moz.build
> on (some) other platforms.
> 

Bug 1358023 is created to deal with moz.build for freetype.

> ::: browser/extensions/mortar/host/moz.build
> @@ +8,5 @@
> > +    '..',
> > +]
> > +
> > +SOURCES += [
> > +    'rpc.cc',
> 
> Doesn't this conflict with bug 1347440?
> 

It depends on the progress of that bug. But I will rebase my patches on top of bug 1347440 in the future anyway.
> :::
> browser/extensions/mortar/pepperpdfium/third_party/pdfium/third_party/moz.
> build
> @@ +89,5 @@
> > +    'media_libjpeg',
> > +    # replacement for fx_lpng
> > +    'mozpng',
> > +    # replacement for fx_zlib
> > +    'zlib',
> 
> While this avoids the duplication of code in the tree, this still duplicates
> the code in Firefox: one in libxul, one in libpepperpdfium. This is
> certainly not desirable on Android, but I don't know if this is supposed to
> build on Android. This would be rather unwanted on desktop, but someone up
> the management ladder could convince me it's okay.
> 

Hi Mike,

Mortar is for desktop only (at least for this moment). I will modify build scripts to further avoid building mortar on non-desktop platforms.

Regarding to duplications within different binaries, could we handle it as a follow-up optimization? Splitting existing libraries (i.e. libjpeg, libpng, zlib) from firefox/gecko into separated dynamic libraries might introduce unexpected regressions, even we disable mortar from the beginning. If it is possible, I would tend to break tough things down into smaller pieces. Does this baby-step approach make sense to you?
Flags: needinfo?(mh+mozilla)

Updated

2 years ago
Depends on: 1360494
(In reply to Bruce Sun [:brsun] from comment #36)
> Created attachment 8858992 [details]
> 0001_pepperpdfium_buildsystem_add_chromium_files.patch.zip

Bruce, please use reviewboard to upload this big patch, otherwise, it's hard to review and tracking.
Comment on attachment 8858993 [details] [diff] [review]
0002_pepperpdfium_buildsystem_add_chromium_autogen_files.patch

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

Please add the purpose of these autogen files, like these files are needed for build pass. BTW, it looks like there files contain some configuration. Do we use default setting with chromium or do we have our own configuration? If we have our own configuration, where is the configuration file?

PS. I just found the different setting of BUILDFLAG_INTERNAL_USE_EXPERIMENTAL_ALLOCATOR_SHIM() between chromium and your patch.

::: browser/extensions/mortar/pepperpdfium/base/generated_build_date.h
@@ +1,3 @@
> +// Generated by //build/write_build_date_header.py
> +#ifndef BUILD_DATE
> +#define BUILD_DATE "Nov 09 2004 00:00:00"

Is this the date we want to set?
Comment on attachment 8858994 [details] [diff] [review]
0003_pepperpdfium_buildsystem_modify_chromium_files.patch

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

I would suggest you just put MORTAR related changes in this patch. For other MORTAR_XXXX, please split into other patches.

::: browser/extensions/mortar/pepperpdfium/pdf/pdf.cc
@@ +39,5 @@
>    return true;
>  }
>  pp::Instance* PDFModule::CreateInstance(PP_Instance instance) {
>    if (!g_sdk_initialized_via_pepper) {
> +#if !defined(MORTAR_DISABLE_SCRIPT)

Please move DISABLE_SCRIPT to another patch and we could cherry-pick the standalone patch easier in the future.

::: browser/extensions/mortar/pepperpdfium/pdf/pdf_engine.h
@@ +19,5 @@
>  #include "ppapi/c/dev/pp_cursor_type_dev.h"
>  #include "ppapi/c/dev/ppp_printing_dev.h"
>  #include "ppapi/c/ppb_input_event.h"
>  #include "ppapi/cpp/completion_callback.h"
> +#include "ppapi/cpp/dev/buffer_dev.h"

Looks like this is not related to build. If yes, please move to another bug to save reviewing effort.

This patch also includes the following bug fixing:
 - Fix slicing issues when using PPP_Printing_Dev::PrintPages(...); further
   information can be found on bug 1311586 comment 7 and bug 1330182 comment 8

::: browser/extensions/mortar/pepperpdfium/pdf/pdfium/pdfium_engine.cc
@@ +245,5 @@
>      }
>    }
>    if (i == arraysize(kPdfFontSubstitutions)) {
> +#if defined(MORTAR_DISABLE_FONT_FACE_ENCODING_CONVERSION)
> +    description.set_face(face);

Why do we want to disable the font-face configuration? Will we generate differnt PDF(fonts) content with chromium?

@@ +1446,5 @@
>                          page_rect.width(),
>                          page_rect.height(),
>                          print_settings.orientation,
>                          FPDF_ANNOT | FPDF_PRINTING | FPDF_NO_CATCH);
> +#if !defined(MORTAR_DISABLE_PRINT_PDF_AS_IMAGE)

Please move this part as another standalone patch.
(In reply to Peter Chang[:pchang] from comment #49)
> Comment on attachment 8858993 [details] [diff] [review]
> 0002_pepperpdfium_buildsystem_add_chromium_autogen_files.patch
> 
> Review of attachment 8858993 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please add the purpose of these autogen files, like these files are needed
> for build pass. BTW, it looks like there files contain some configuration.
> Do we use default setting with chromium or do we have our own configuration?
> If we have our own configuration, where is the configuration file?
> 

The purpose of these autogen files are the same as other chromium files, they are mandatory for building pepperpdfium successfully. I separate these files from the previous patch for easing the tracking efforts. Files in 0001 part are copied from chromium directly. Files in 0002 part are copied from |out| folder (similar to |objdir-xxx| folder of Gecko after executing ./mach build). The scripts to generate these files are parts of GN build system, so I would rather directly copy the generated file to save our burden.

> PS. I just found the different setting of
> BUILDFLAG_INTERNAL_USE_EXPERIMENTAL_ALLOCATOR_SHIM() between chromium and
> your patch.
> 

Some settings contained in this patch are modified manually, which is different from the default setting of chromium. I don't use any configuration files from GN.

> ::: browser/extensions/mortar/pepperpdfium/base/generated_build_date.h
> @@ +1,3 @@
> > +// Generated by //build/write_build_date_header.py
> > +#ifndef BUILD_DATE
> > +#define BUILD_DATE "Nov 09 2004 00:00:00"
> 
> Is this the date we want to set?

The value is meaningless. The definition here is just for making the compiler happy before unused symbols has been stripped by the linker.
> ::: browser/extensions/mortar/pepperpdfium/pdf/pdfium/pdfium_engine.cc
> @@ +245,5 @@
> >      }
> >    }
> >    if (i == arraysize(kPdfFontSubstitutions)) {
> > +#if defined(MORTAR_DISABLE_FONT_FACE_ENCODING_CONVERSION)
> > +    description.set_face(face);
> 
> Why do we want to disable the font-face configuration? Will we generate
> differnt PDF(fonts) content with chromium?
> 

This trick was invented by Evelyn[1]. As far as I know, if we don't disable those codes, we have to copy many other files from chromium.

Evelyn, do you have more details about the intention and the corresponding impact to disable the font enumeration on Linux platform?

[1] bug 1273377 comment 9, or attachment 8837059 [details] [diff]
Flags: needinfo?(ehung)
Hi Elvin,

We are going to import many files from chromium project. I've go through all these new folders and try to add additional license notices into the license.html. Would you mind having a look on it first and share how you feel about that?
Attachment #8865765 - Flags: feedback?(ellee)
Reporter

Comment 54

2 years ago
(In reply to Bruce Sun [:brsun] from comment #52)
> > ::: browser/extensions/mortar/pepperpdfium/pdf/pdfium/pdfium_engine.cc
> > @@ +245,5 @@
> > >      }
> > >    }
> > >    if (i == arraysize(kPdfFontSubstitutions)) {
> > > +#if defined(MORTAR_DISABLE_FONT_FACE_ENCODING_CONVERSION)
> > > +    description.set_face(face);
> > 
> > Why do we want to disable the font-face configuration? Will we generate
> > differnt PDF(fonts) content with chromium?
> > 
> 
> This trick was invented by Evelyn[1]. As far as I know, if we don't disable
> those codes, we have to copy many other files from chromium.
> 
> Evelyn, do you have more details about the intention and the corresponding
> impact to disable the font enumeration on Linux platform?
> 
> [1] bug 1273377 comment 9, or attachment 8837059 [details] [diff]

The intention was originally to avoid introducing base/i18n/ into the codebase, but I then realize the whole code paragraph of the g_font_info definition was because "Font loading doesn't work in the renderer sandbox in Linux." [2] Therefore I commented out the whole g_font_info definition instead of just commenting out the place invoking base/i18n (which is what you did here). 

I guess it's better to verify if system font loading[3] works in our sandbox design and then decide what action we want to take here. It sounds to me that simply commenting out base/i18n usage doesn't make more sense.

[2] https://cs.chromium.org/chromium/src/pdf/pdfium/pdfium_engine.cc?q=pdfium_engin+package:%5Echromium$&l=632

[3] https://cs.chromium.org/chromium/src/third_party/pdfium/core/fxge/ge/fx_ge_linux.cpp?type=cs&l=148
Flags: needinfo?(ehung)
(In reply to Bruce Sun [:brsun] from comment #45)
> Regarding to duplications within different binaries, could we handle it as a
> follow-up optimization? Splitting existing libraries (i.e. libjpeg, libpng,
> zlib) from firefox/gecko into separated dynamic libraries might introduce
> unexpected regressions, even we disable mortar from the beginning. If it is
> possible, I would tend to break tough things down into smaller pieces. Does
> this baby-step approach make sense to you?

Since that code is going to ship in firefox anyways, and won't be updated through system addons, why bother putting everything in a separate library in the first place? Why not put everything in libxul and avoid the duplication this way?
Flags: needinfo?(mh+mozilla)
> Since that code is going to ship in firefox anyways, and won't be updated
> through system addons, why bother putting everything in a separate library
> in the first place? Why not put everything in libxul and avoid the
> duplication this way?

Hi Peter,

Do you have any suggestions for this topic?
Flags: needinfo?(peterv)
Update more context of comment58: split patches into smaller pieces per :pchang's comments.
I am going to double check the used symbols inside pepperpdfium with MOZ_Z_* prefixes[1] and MOZ_PNG_* prefixes[2].

[1] http://searchfox.org/mozilla-central/source/modules/zlib/src/mozzconf.h
[2] http://searchfox.org/mozilla-central/source/media/libpng/pnglibconf.h
The discussion about how PDFium should be integrated into m-c was brought up again recently. Most likely PDFium will be integrated into m-c first for converting EMF on Windows. How mortar project could cooperate with this change is not clear to me for the moment. Let's wait and see.
Comment on attachment 8858992 [details]
0001_pepperpdfium_buildsystem_add_chromium_files.patch.zip

Clear review request for the moment.
Attachment #8858992 - Flags: review?(peterv)

Updated

2 years ago
Attachment #8858993 - Flags: review?(peterv)

Updated

2 years ago
Attachment #8858994 - Flags: review?(peterv)
Comment on attachment 8865765 [details]
mortar_license.patch.wip

Clear feedback request for the moment.
Attachment #8865765 - Flags: feedback?(ellee)
Clear needinfo request for the moment.
Flags: needinfo?(peterv)
Bug 1368948 is created for integrating PDFium without Pepper Plugin APIs stuffs.
See Also: → 1368948
Reporter

Comment 66

2 years ago
to be clear: landing m-c the pepper layer into m-c seems to cause more long-term code management overhead, we are evaluating if we really want to do that. However, pdfium lib is relatively isolated and we want to use it to solve general(i.e for both web pages and pdf files) printing issue on Windows, therefore we decided to limit the code integration scope and file bug 1368948 for avoiding confusion.

We will revisit this bug when we are done the Pepper code evaluation.
Reporter

Updated

2 years ago
Severity: normal → major
Priority: -- → P2
unassign myself since I am not going to working on this bug
Assignee: brsun → nobody

I'm closing this bug as WONTFIX per:

"The Mortar experiment has concluded. Mozilla does not consider the PDF use case justifies the burden of implementing and maintaining PDFium and a Pepper API implementation in Gecko."

Source: https://wiki.mozilla.org/Mortar_Project

Status: NEW → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.