502933 - Installation takes way too long

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Alex Pakhotin (:alexp)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.11) Gecko/2009060215 Firefox/3.0.11 (.NET CLR 3.5.30729)
Build Identifier: fennec-1.0a2.en-US.wince-arm

It takes up to several minutes to install Fennec from a CAB on a WM device.


Reproducible: Always

Steps to Reproduce:
1. Copy fennec*.cab to a device.
2. Run it.
3. Notice the time it takes to install.

Comment 1

[Alex Pakhotin (:alexp)]

Attachment: Fennec WM CAB Installer speed test report

The CAB installer does not do anything special - it just copies all the files to the specified folders, so we are limited just by the speed of this operation, which is pretty long due to a large number of files, and their total size.

But as you can see from the time measurements, this standard system functionality is not implemented the most optimal way. Total Commander performs copying and extracting from an archive much faster (on the Asus it was 3 times faster).

So if we want to improve this installation speed we could use some kind of a custom/hand-made installer, which itself would extract the files from an archive.

Comment 2

[Alex Pakhotin (:alexp)]

Attachment: [Work in Progress] Zip Extractor implementation, which is supposed to replace CAB installer

This version extracts a zip file, which has the same name as the exe, just with
extension .zip, and is located in the same folder.
For example if you want to install fennec-1.0a3pre.en-US.wince-arm.zip, build
the program, rename fennec_installer.exe to fennec-1.0a3pre.en-US.wince-arm.exe,
copy both files to a Windows Mobile device and run the exe.

Note: Requires zlib.patch (see another attachment) to avoid conflict between mozz.lib and mozce_shunt.

Known issues:
- The Makefile is copied to the $(OBJDIR), but does not get invoked when the whole tree is built with "make -f client.mk build";
- nsZipArchive.* is a copy from modules/libjar - needs to be built from its original location, but with different compiler option;
- rc.exe cannot find "resdefce.h" when called from the make, though it works when the same command line is executed from cmd.exe.

Comment 3

[Alex Pakhotin (:alexp)]

Attachment: Patch to fix a conflict between mozz.lib and mozce_shunt, when both are linked statically

Comment 4

[Alex Pakhotin (:alexp)]

Attachment: [Work in Progress] Zip Extractor implementation, which is supposed to replace CAB installer

[Work in Progress] Zip Extractor implementation, which is supposed to replace CAB installer.

This version extracts a ZIP file appended to the EXE, i.e. works like a self-extracted ZIP.

For example if you want to install fennec-1.0a3pre.en-US.wince-arm.zip, build the program,
run the following command:

  cat fennec_installer.exe fennec-1.0a3pre.en-US.wince-arm.zip >fennec-1.0a3pre.en-US.wince-arm.exe

Then copy fennec-1.0a3pre.en-US.wince-arm.exe to a device and run it.

Note: Requires zlib.patch to avoid conflict between mozz.lib and mozce_shunt.

Comment 5

[Alex Pakhotin (:alexp)]

Attachment: [Work in Progress] Zip Extractor implementation, which is supposed to replace CAB installer

Updated version, which includes the uninstaller.

Known issue to be fixed:
- rc.exe cannot find "resdefce.h" when called from the make, though it works
when the same command line is executed from cmd.exe.

Comment 6

[Alex Pakhotin (:alexp)]

Attachment: Zip Extractor implementation to replace CAB installer

Some updates to the code.
Also includes the fix for the bug 503109 (do our first run work during cab installation).

Comment 7

[Alex Pakhotin (:alexp)]

Attachment: Zip Extractor implementation to replace CAB installer

Updated version - includes a couple zip headers, which still have standalone mode support removed with a fix to the bug 505784.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 393963 [details] [diff] [review]
Zip Extractor implementation to replace CAB installer

A couple quick things

1) This should be in m-c's toolkit/mozapps/installer/wince instead of m-b's installer/wince
2) delete things instead of commenting them out
3) Attach the .ico file for ui review
4) Build the shunt statically (as well as dynamically) and link to that
5) mozz.lib is in dist/lib, link to it there

Comments on the actual code to follow

Comment 9

[Alex Pakhotin (:alexp)]

Attachment: Zip Extractor implementation to replace CAB installer

Updated according to review comments.

Comment 10

[Alex Pakhotin (:alexp)]

Attachment: Patch for mobile/installer/Makefile

Create EXE instead of CAB with "make package".

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 394427 [details] [diff] [review]
Zip Extractor implementation to replace CAB installer

Rob, since this is in toolkit now do you mind taking the review?

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 394427 [details] [diff] [review]
Zip Extractor implementation to replace CAB installer

Drive by comments:
* This patch is too fennec centric. I mean, we should be more like xulrunner_stub here. This includes:
** Do not include the fennec.ico icon in mozapps. Use _no_ icon and mobile can insert the right icon from the mobile build side (we do this for fennec.exe now)
** Name the installer "xulrunner_stub_installer" in mozapps. Yes I know it's long and ugly, but the point is mobile will rename it from the mobile build (we do this now for xulrunner_stub -> fennec)
** I'd prefer all occurrences of "Fennec" to be changed to "XULRunner" since this is really a method of installing xulrunner applications via a special installer.

Comment 13

[Axel Hecht [:Pike]]

Comment on attachment 394427 [details] [diff] [review]
Zip Extractor implementation to replace CAB installer

Glancing at the patch, I see several hard-coded strings that should be exposed to l10n. That should be done by either a .properties or .ini file, I guess. There shouldn't be any compilations to be run to repack en-US for l10n.

Reading the comments, we have:

- two files to download, the zip and the exe, which need to be next to each other at launching the exe?

- first-run code in the exe. How fennec-specific is that code?

Comment 14

[Alex Pakhotin (:alexp)]

Localization has been on my TODO list for quite a while now. Have to think of the best way to implement it so that everything will be still in just one file.

As for the questions:

- The latest version of the installer works like SFX-ZIP - there will be only one file to download - this is a must, I believe.

- First-run code is just executing "fennec.exe -silent". We are thinking about making it easily customizable - have some kind of a script file with a command (or several commands) to run.

Comment 15

[Alex Pakhotin (:alexp)]

Attachment: [Work in Progress] Zip Extractor implementation to replace CAB installer

- Fixed a couple of issues including a build error;
- Renamed fennec_installer to xulrunner-stub-installer.

Comment 16

[Alex Pakhotin (:alexp)]

Attachment: Patch for mobile/installer/Makefile

Fixed the file paths.

Comment 17

[Robert Strong (they/them - no direct email)]

I noticed that the latest 7-Zip which has better compression now has support for Windows CE.

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 394784 [details] [diff] [review]
[Work in Progress] Zip Extractor implementation to replace CAB installer

Please switch to use /xulrunner/app/xulrunner.ico or /toolkit/mozapps/installer/windows/nsis/setup.ico

We do not want a fennec.ico in mozilla-central

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

And we will need to fix the hard coded strings quickly too.

Comment 20

[Alex Pakhotin (:alexp)]

Attachment: Zip Extractor implementation to replace CAB installer

Implemented changes as per review comments:
- Separated text strings from the code into a separate file (setup.str), so it could be localized and customized for other versions;
- Replaced Fennec.ico with xulrunner.ico;
- Made Uninstall use the strings file too;
- Uninstall now copies itself to a temp directory and runs from there to delete the whole installation.

Comment 21

[Alex Pakhotin (:alexp)]

Attachment: Patch for mobile/installer/Makefile

Added strings file handling to the mobile installer Makefile.

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 395510 [details] [diff] [review]
Zip Extractor implementation to replace CAB installer

drive by review part 1

Meta comment, I think we can just go with wide strings rather than the ambiguous TCHARs

>-DIRS = downloads extensions update xpinstall plugins handling
>+DIRS = downloads extensions update xpinstall plugins handling installer
and
>diff --git a/toolkit/mozapps/installer/Makefile.in b/toolkit/mozapps/installer/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/installer/Makefile.in
we try to avoid no-op make files.  Just add installer/wince above


>+CStrings::CStrings()
Let's rename this to avoid confusion with MS's CString and to describe its function more clearly.  nsLocalizedStrings would seem to work.

>+CStrings::~CStrings()
>+{
>+  delete[] m_sBuf;
>+}
if (m_sBuf)
   delete[] m_sBuf;

>+    if (ReadFile(f, m_sBuf, m_nBufLen * sizeof(TCHAR), &dwRead, NULL))
>+      bResult = ParseStrings();

you should probably fill msBuf in a loop 

Also, m_cMaxStringID isn't very obvious.  mNumbStringIDs maybe?



>+  // SHInitExtraControls should be called once during your application's initialization to initialize any
>+  // of the device specific controls such as CAPEDIT and SIPPREF.
Sounds like these comments were taken from some example code or boiler plate.

Comment 23

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 395510 [details] [diff] [review]
Zip Extractor implementation to replace CAB installer

part 2

rename CZipExtractorDlg to nsZipExtractorDlg

>diff --git a/toolkit/mozapps/installer/wince/nsZipArchive.cpp b/toolkit/mozapps/installer/wince/nsZipArchive.cpp
>diff --git a/toolkit/mozapps/installer/wince/nsZipArchive.h b/toolkit/mozapps/installer/wince/nsZipArchive.h
>diff --git a/toolkit/mozapps/installer/wince/nsZipExtractor.cpp b/toolkit/mozapps/installer/wince/nsZipExtractor.cpp
>diff --git a/toolkit/mozapps/installer/wince/nsZipExtractor.h b/toolkit/mozapps/installer/wince/nsZipExtractor.h
>diff --git a/toolkit/mozapps/installer/wince/zipfile.h b/toolkit/mozapps/installer/wince/zipfile.h
>diff --git a/toolkit/mozapps/installer/wince/zipstub.h b/toolkit/mozapps/installer/wince/zipstub.h

why are you copying these files?


>diff --git a/toolkit/mozapps/installer/wince/stdafx.h b/toolkit/mozapps/installer/wince/stdafx.h
we don't usually use these sort of headers

Comment 24

[Axel Hecht [:Pike]]

Comment on attachment 395510 [details] [diff] [review]
Zip Extractor implementation to replace CAB installer

r- on the strings parts. Not sure what '.str' is supposed to be (can't tell from the diff), but you should use one of the existing formats, either .ini or .properties, or .inc. Look at what the crashreporter and the updater code do, there's already standalone code to copy for reading .ini files.

The localizable content will need to be in locales/en-US/, like in a subdir installer, that's where other apps have that.

Comment 25

[Robert Strong (they/them - no direct email)]

Comment on attachment 395510 [details] [diff] [review]
Zip Extractor implementation to replace CAB installer

I had a discussion with bsmedberg and taras today and they don't want to un-nuke standalone libjar (I didn't realize it had been nuked before the conversation). They both agreed that a solution using 7-Zip (we already have the 7-Zip source under other-licenses though it would need to be updated to the latest to support Windows CE) would be accepted.

Comment 26

[Alex Pakhotin (:alexp)]

Attachment: [Work in Progress] Compiled binary for testing

This archive contains compiled zip-extractor binary and a script, which can be used to create an EXE installer package from existing fennec directory.
Can be used to test the current version of the EXE installer.

Comment 27

[Alex Pakhotin (:alexp)]

Attachment: Archive Extractor implementation to replace CAB installer

- Fixed code review issues
- Localizable text is now stored in locales/en-US/installer/setup.ini
- Modified updater/readstrings.* parser to support other applications
- Refactored archive extractor to make a switch to another archiver easier

Comment 28

[Alex Pakhotin (:alexp)]

Attachment: Patch for mobile/installer/Makefile

- Added locales/en-US/installer/setup.ini
- Modified mobile/installer/Makefile to handle ZIP archive and setup.ini.

Comment 29

[Alex Pakhotin (:alexp)]

Attachment: Patch for mobile/installer/Makefile

[corrected patch]

- Added locales/en-US/installer/setup.ini
- Modified mobile/installer/Makefile to handle ZIP archive and setup.ini.

Comment 30

[Alex Pakhotin (:alexp)]

Test version of EXE installer using 7-zip compression is now available at
http://people.mozilla.com/~alexp/

Comment 31

[Alex Pakhotin (:alexp)]

Attachment: lib7z

Library to extract 7z archives.
Includes 7-zip LZMA SDK version 9.07 beta.

Comment 32

[Alex Pakhotin (:alexp)]

Attachment: 7-zip binaries included in LZMA SDK

Comment 33

[Alex Pakhotin (:alexp)]

Attachment: 7z SFX Archive implementation to replace CAB installer

New version, which uses 7-zip instead of Zip.
Requires lib7z.

Comment 34

[Alex Pakhotin (:alexp)]

Attachment: Additional patch for the mobile tree

Includes localizable strings and a change to Makefile to create a package.

Comment 35

[Robert Strong (they/them - no direct email)]

Alex, I have the flu and am rather out of it so please have someone else review these patches especially since I'm not a peer of this part of the tree

Comment 36

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 402983 [details] [diff] [review]
Additional patch for the mobile tree


>-	cd $(DIST) && $(PYTHON) $(topsrcdir)/build/package/wince/make_wince_cab.py $(CABARGS) "$(VSINSTALLDIR)/SmartDevices/SDK/SDKTools/cabwiz.exe" "$(MOZ_PKG_DIR)" "$(MOZ_APP_DISPLAYNAME)" "$(PKG_PATH)$(PKG_BASENAME).cab" && $(ZIP) -r9D $(PACKAGE) $(MOZ_PKG_DIR) && echo "Installer $(PKG_PATH)$(PKG_BASENAME).cab created!"
>+	cp $(srcdir)/../locales/$(AB_CD)/installer/setup.ini $(DIST)/setup.ini
>+	cd $(DIST) && mv $(MOZ_PKG_DIR)/xulrunner/uninstall.exe $(MOZ_PKG_DIR)/ && 7z a $(PKG_PATH)$(PKG_BASENAME).7z $(MOZ_PKG_DIR) setup.ini -x!$(MOZ_PKG_DIR)/xulrunner/xulrunner-stub-installer.sfx && cat $(MOZ_PKG_DIR)/xulrunner/xulrunner-stub-installer.sfx $(PKG_PATH)$(PKG_BASENAME).7z>$(PKG_PATH)$(PKG_BASENAME).exe && rm $(PKG_PATH)$(PKG_BASENAME).7z && rm setup.ini
>+	@echo Installer $(PKG_PATH)$(PKG_BASENAME).exe created!

two nits, use nsinstall instead of cp and we don't need this echo

Comment 37

[Alex Pakhotin (:alexp)]

Attachment: Additional patch for the mobile tree

Fixed the "nits".

Comment 38

[Benjamin Smedberg]

This patch contains windows-style line endings in many places. Please make sure that only unix-style endings are checked in.

I presume that the files in LZMASDK are copied without modification from the 7zip SDK? Was that taken from version control or a release number? I think you should at least have a README.mozilla file which explains where the sources came from.

> diff --git a/modules/lib7z/7zLib.cpp b/modules/lib7z/7zLib.cpp

> +/* -*- Mode: C++; c-basic-offset: 2; tab-width: 4; indent-tabs-mode: nil; -*- */^M

Here and in 7zLib.h: tab-width should be 8 (so it's easy to notice
tabs that get inserted by accident). There are windows-style newlines
in this file. Please remove them.

> +/* ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1

> + *

> + * The contents of this file are subject to the Mozilla Public License Version

> + * 1.1 (the "License"); you may not use this file except in compliance with

> + * the License. You may obtain a copy of the License at

> + * http://www.mozilla.org/MPL/

> + *

> + * Software distributed under the License is distributed on an "AS IS" basis,

> + * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License

> + * for the specific language governing rights and limitations under the

> + * License.

> + *

> + * The Original Code is 7z Client and 7z Standalone Extracting Plugin.

> + *

> + * The Initial Developer of the Original Code is

> + * Igor Pavlov

> + *

> + * Portions created by the Initial Developer are Copyright (C) 2009

> + * the Initial Developer. All Rights Reserved.


This is incorrect. Please have Gerv create a correct license header for this
file. Although Igor Pavlov is the author of the original code, he explicitly
disclaimed all copyright by placing it into the public domain. Your
modifications of his code can be tri-licensed, but the header needs to say
something different than this.

> diff --git a/modules/lib7z/7zobjs.mk b/modules/lib7z/7zobjs.mk

> +7ZIPSRCS = \

> +	$(7ZIPSRCDIR)/CPP/7zip/Archive/7z/7zDecode.cpp \


In order for the build system to calculate dependencies correctly, use
need to use bare filenames in CPPSRCS/CSRCS instead of directories. So
what you want here is:

7ZIPCPPSRCS = \
  7ZDecode.cpp \
  7zExtract.cpp \
  etc...

7ZIPCSRCS = \
  7zCrc.c \
  etc..

vpath %.cpp $(7ZIPSRCDIR)/CPP/7zip/Archive/7z
vpath %.cpp $(7ZIPSRCDIR)/CPP/Archive
vpath %.cpp $(7ZIPSRCDIR)/CPP/7zip/Archive/Common
etc...
vpath %.c $(7ZIPSRCDIR)/C

Also please use 7ZIPCPPSRCS instead of 7ZIPSRCS. It took me a minute to figure out what the difference between 7ZIPSRCS and 7ZIPCSRCS was.

More windows-style line endings

> diff --git a/modules/lib7z/Makefile.in b/modules/lib7z/Makefile.in

> +# The Initial Developer of the Original Code is
> +# Alex Pakhotin <[e-mail]>

The correct initial developer for employees/contractors is "The Mozilla Foundation <http://www.mozilla.org>"

Can we move the WINCE ifdef out of this makefile. There's no point even recursing into this directory if we're not on WINCE; it just takes extra build time. Put the ifdef in toolkit-tiers.mk

> +OS_LIBS += oleaut32.lib ole32.lib

Why do you have this line? You're creating a static library, so
OS_LIBS doesn't apply, I don't think.

Comment 39

[Gervase Markham [:gerv]]

Because the 7zip code is in the public domain, it is entirely *legal* for us to slap tri-license headers on our copies, and say that these particular copies (even without modifications) are tri-licensed.

It is legal, but it's probably not the right thing to do. The nice thing to do is to put our changes in the public domain also, so that 7zip can pick them up if they want. So I suggest that you do not add any license headers, to Igor's files, but instead have a README which says something like:

This code is from the LZMA SDK. It is in the public domain (see <url>).
Any copyright in these files held by contributors to the Mozilla Project is also dedicated to the Public Domain.
http://creativecommons.org/licenses/publicdomain/

Gerv

Comment 40

[Alex Pakhotin (:alexp)]

The library consists of two parts.

LZMASDK sub-directory contains unchanged code from LZMA SDK. It is planned to be intact, I'm only adding a README.mozilla file as recommended by Benjamin, to state the version and origin.
Theoretically the files in that directory could be replaced with the newer version of the SDK later.

4 files outside of LZMASDK including a C++ source, Makefile, and a couple of include files are not from the SDK. Makefile is Mozilla-build specific and created from scratch. 7zLib.cpp does contain a lot of code from the SDK, because it is basically a heavily modified for our purposes version of LZMASDK/CPP/7zip/UI/Client7z/Client7z.cpp with some specific additions to make it work in a static library. It could be placed in the public domain, though I doubt our changes could be picked up "as is".

Taking this into account, do you still want me to just add that README and not have any header in 7zLib.cpp?

And if we place this code in the public domain, should we move it to other-licenses again? Please note this library is integrated into the build system, I'm not sure if it will the right thing to use it this way from other-licenses.

Comment 41

[Gervase Markham [:gerv]]

Please add a header like the one I suggest to the heavily-modified file 7zLib.cpp. There is no need to move anything to other-licenses; "other" means "things not under the tri-license or a trvially compatible one (such as BSD or public domain)". Add a tri-license header to the Makefile. That will be all :-)

Gerv

Comment 42

[Alex Pakhotin (:alexp)]

Attachment: lib7z

Library to extract 7z archives.
Includes 7-zip LZMA SDK version 9.07 beta.

Implemented changes as per the review.

Comment 43

[Benjamin Smedberg]

+# Having this rule here to compile 7zIn.cpp, but not 7zIn.c
+%.$(OBJ_SUFFIX): %.cpp $(GLOBAL_DEPS)

I wanted this to be a static pattern rule:

7zIn.$(OBJ_SUFFIX): %.$(OBJ_SUFFIX): %.cpp $(GLOBAL_DEPS)

And any custom rules should be *after* rules.mk, not before, to avoid messing up other ordering. r=me with that change

Comment 44

[Robert Strong (they/them - no direct email)]

Comment on attachment 402981 [details] [diff] [review]
7z SFX Archive implementation to replace CAB installer

>diff --git a/toolkit/mozapps/update/src/updater/readstrings.cpp b/toolkit/mozapps/update/src/updater/readstrings.cpp
>--- a/toolkit/mozapps/update/src/updater/readstrings.cpp
>+++ b/toolkit/mozapps/update/src/updater/readstrings.cpp
>...
>@@ -106,19 +111,52 @@ NS_strtok(const char *delims, char **str
>...
>+/**
>+ * Very basic parser for updater.ini taken mostly from nsINIParser.cpp
>+ * Modified to be used for the installer or similar standalone app as well.
nit: Change the comment to s/Very/A very/
Change the second line to:
that can be used by standalone apps.

r=me for the updater changes... do you need me to review the remainder as well?

Comment 45

[Alex Pakhotin (:alexp)]

Attachment: lib7z

Library to extract 7z archives.
Includes 7-zip LZMA SDK version 9.07 beta.
---
Fixed Makefile.in as suggested.

Comment 46

[Alex Pakhotin (:alexp)]

Attachment: 7z SFX Archive implementation to replace CAB installer

The latest version of the patch with some minor fixes.

Comment 47

[Robert Strong (they/them - no direct email)]

Alex, have you had a chance to send the patches to the try server as I asked yesterday and if so, what are the results especially in regards to Firefox on WinCE?

Comment 48

[Alex Pakhotin (:alexp)]

The patch has to be fixed a bit (one makefile), after that it's OK on all the platforms, ...except WinMo!
Now I'm trying to figure out why it fails. It builds fine on my machine with exactly the same mozconfig.

Comment 49

[Alex Pakhotin (:alexp)]

Attachment: 7z SFX Archive implementation to replace CAB installer

Fixed the Makefile for Firefox WinCE build.

Everything builds on the Try server now.

Comment 50

[Brian Crowder]

Comment on attachment 406736 [details] [diff] [review]
7z SFX Archive implementation to replace CAB installer

Robert asked me to take a glance at this, so here are my thoughts:

Non-NIT:  Prefer MessageBoxW(...) to MessageBox(...) wherever possible, and use explicit calls instead of overloads for all code like this.

+class ns7zipExtractor : public nsArchiveExtractor
+{
+public:
NIT: If it's all public, how about a struct {} ?  I don't know what our standards say about this, though.

+nsArchiveExtractor::nsArchiveExtractor(const WCHAR *sArchiveName, DWORD dwSfxStubSize, nsExtractorProgress *pProgress)
+{
+  m_sArchiveName = NULL;
+  if (sArchiveName)
+    m_sArchiveName = wcsdup(sArchiveName);
+  m_pProgress = pProgress;
+  m_dwSfxStubSize = dwSfxStubSize;
+}
NIT: How about using an initializer list for all the copyable stuff here (and in similar classes)?  Again, not sure what our standards say about this.

+  if (m_sArchiveName)
+    free(m_sArchiveName);
NIT: free() null-checks.

+  if (!InitInstance(hInstance, nCmdShow)) 
+  {
+    return nResult;
+  }
NIT: Don't need the extra  { } scope here (other similar spots throughout).

+  hImage = CreateFile(g_sExeFullFileName,
+    GENERIC_READ,
+    FILE_SHARE_READ,
+    NULL,
+    OPEN_EXISTING,
+    FILE_ATTRIBUTE_NORMAL,
+    NULL);
NIT: Robert can speak more authoritatively here, but I prefer indentation that aligns with the the column after the opening ( of the function call.

+    ErrorMsg(L"Could not open the EXE"); //GetLastError()
How does this get localized?

+  ReadBytes(hImage,
+    &image_dos_header,
+    sizeof(IMAGE_DOS_HEADER));
I recommend that you prefer sizeof(image_dos_header) over sizeof(IMAGE_DOS_HEADER) here (that is, use the variable itself rather than its explicit type), so that if the type of the variable changes at a later date, this code does not become a potentially serious overrun.  Check everywhere you're using sizeof() or a constant like IMAGE_SIZEOF_FILE_HEADER.

+  if (!ReadFile(hFile,
+    buffer,
+    size,
+    &bytes,
+    NULL))
NIT: Are multiple lines here even necessary?  You have other places in your patch where you have all parameters on one line, so I think it would be okay here.

+  _snwprintf(sShortcutPath, MAX_PATH, L"%s\\%s.lnk",
+    sProgramsPath, Strings.GetString(StrID_AppShortName));
Possible for sProgramsPath to be used uninitialized here?

+  BOOL bResult = CreateProcess(sCmdLine, L"-silent -nosplash",
+    NULL, NULL, FALSE, 0, NULL, NULL, NULL, &pi);
Do we actually want this to run and enable the faststart "daemon"?

Comment 51

[Alex Pakhotin (:alexp)]

(In reply to comment #50)
> +    ErrorMsg(L"Could not open the EXE"); //GetLastError()
> How does this get localized?

This part is the "loader", which gets the localized strings. These English-only messages are not supposed to be shown in normal conditions. They may show up only in some very bad and very unrealistic case when we cannot do anything with that.

> +  BOOL bResult = CreateProcess(sCmdLine, L"-silent -nosplash",
> +    NULL, NULL, FALSE, 0, NULL, NULL, NULL, &pi);
> Do we actually want this to run and enable the faststart "daemon"?

This is different from the faststart "daemon". It's just the first run, which creates a user profile. It is supposed to speed up the first actual start by the user - the profile will already exist at that moment.

Comment 52

[Alex Pakhotin (:alexp)]

Attachment: 7z SFX Archive implementation to replace CAB installer

Code review changes.

Comment 53

[Brian Crowder]

(In reply to comment #51)
> (In reply to comment #50)
> > Do we actually want this to run and enable the faststart "daemon"?
> 
> This is different from the faststart "daemon". It's just the first run, which
> creates a user profile. It is supposed to speed up the first actual start by
> the user - the profile will already exist at that moment.

Understood.  I'm just saying, we could actually make that first start -really- fast by leaving the daemon running, too.  Probably good fodder for a follow-up.

Comment 54

[Alex Pakhotin (:alexp)]

(In reply to comment #53)
> I'm just saying, we could actually make that first start -really-
> fast by leaving the daemon running, too.  Probably good fodder for a follow-up.

Well, this makes sense, especially because the user is very likely going to run the freshly installed application. But the user must have the ability to disable it.
We have to think about the fast-start daemon anyway - the EXE installer does not support that at all at the moment. And the daemon definitely has to be optional, so we'll probably need a pref or something.

Comment 55

[Brian Crowder]

Comment on attachment 410690 [details] [diff] [review]
7z SFX Archive implementation to replace CAB installer

Looks good to me.  Thanks!

Comment 56

[Alex Pakhotin (:alexp)]

Three patches have to be applied:
"lib7z" - it's 7-zip library, has to be applied first;
"7z SFX Archive implementation" - actual installer, it depends on lib7z;
"Additional patch for the mobile tree" - contains changes for mobile makefiles and localization.

"7-zip binaries" - is optional, but better be applied as well to have complete LZMA (7-zip) SDK in our tree.

Comment 57

[Brian Crowder]

http://hg.mozilla.org/mozilla-central/rev/0b3c29bf0fb8 (7-zip import)
http://hg.mozilla.org/mozilla-central/rev/57e57190f520 (cab replacement)

Comment 58

[Brian Crowder]

http://hg.mozilla.org/mobile-browser/rev/9e58ca6f80ef

Comment 59

[Brian Crowder]

I'd rather not put binaries into the tree; is there a reason beyond completeness that we need the 7-zip binaries patch?

Comment 60

[Brian Crowder]

I think this is all in, and this bug has already represented more than its share of patches; we can follow-up in another bug regarding the 7-zip binaries, I think?

Comment 61

[Alex Pakhotin (:alexp)]

Binaries are not required for the installer itself - they are just a part of LZMA SDK. I guess we may skip them for now.

Comment 62

[Brian Crowder]

Can't they be built from source?

Comment 63

[Brian Crowder]

Comment on attachment 404907 [details] [diff] [review]
lib7z

This patch is necessary to enable the .exe installer for Windows Mobile Fennec.

Comment 64

[Brian Crowder]

Comment on attachment 410690 [details] [diff] [review]
7z SFX Archive implementation to replace CAB installer

This patch is necessary to enable the .exe installer for Windows Mobile Fennec.

Comment 65

[Alex Pakhotin (:alexp)]

(In reply to comment #62)
> Can't they be built from source?

Hey! That's right!

Just run nmake in these directories:
...\LZMASDK\CPP\7zip\Bundles\Alone7z\
...\LZMASDK\CPP\7zip\Compress\LZMA_Alone\

So we don't really need the compiled binaries there.

Comment 66

[Aakash Desai [:aakashd]]

Is there an estimated amount of drop off time from before this patch was landed to afterwards? Installing the .cab file is faster, but I'd like to get a definite time to verify against.

Comment 67

[Alex Pakhotin (:alexp)]

Here are my timings using HTC TouchPro and the latest nightly build:
CAB Installation: 1 minute 23 seconds
EXE Installation: 55 seconds, including the first silent run to create a user profile (just installation itself without running Fennec took about 31 seconds).

File sizes:
CAB Installer: 10736398 bytes
EXE Installer:  7456321 bytes

Comment 68

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 404907 [details] [diff] [review]
lib7z

a192=beltzner, good gracious, this is a lot of code.

Comment 69

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 410690 [details] [diff] [review]
7z SFX Archive implementation to replace CAB installer

a192=beltzner, do we need tests here?

Comment 70

[Brian Crowder]

We definitely need this installation process to be tested via litmus, if it is not already.

Comment 71

[Brian Crowder]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d9e61ef653ee
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ac81bcba3ec7
http://hg.mozilla.org/mobile-browser/rev/0639fc4d1047

Comment 72

[Brian Crowder]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c5acfaebd602 (for the .ico on 192, whoops)

Comment 73

[Aki Sasaki (not active)]

Attachment: upload exe rather than cab/zip

Comment 74

[Axel Hecht [:Pike]]

FWIW, I'm not sure if dropping the zip file creation was a good choice here. Didn't realize that that was about to happen from following the bug either.

Comment 75

[Aki Sasaki (not active)]

Comment on attachment 412930 [details] [diff] [review]
upload exe rather than cab/zip

http://hg.mozilla.org/build/buildbotcustom/rev/f644a5aa549a

Comment 76

[Brian Crowder]

afaik, zip-file creation should still be possible...  is there some reason releng should continue generating them?

Comment 77

[Axel Hecht [:Pike]]

Not sure if this is a biggie for mobile devices, but zip's are usually easy to just put somewhere for QA purposes.

The other piece is that we need to repack the en-US builds, and it's not clear on #mobile that 7z x foo.exe works for that. If we can't unpack the exe, using the zip and creating an exe out of that similar to the way it's done in en-US would be the alternative route.

Comment 78

[Alex Pakhotin (:alexp)]

.zip file creation is addressed in the bug 529420.

Comment 79

[Axel Hecht [:Pike]]

The setup.ini file has multiple instances of "Fennec" hardcoded in it.

I guess that's bad when we rebrand it to Firefox eventually?

Comment 80

[Alex Pakhotin (:alexp)]

(In reply to comment #79)

If needed we can handle it the same way as done for updater.ini - have a place-holder and a script, which puts the actual product name there.

Comment 81

[Al Billings [:abillings - ex-MoCo]]

in-litmus+

https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=9769

Comment 82

[Nick Thomas [:nthomas] (UTC+12)]

Could you clarify what the correct make command is with this new installer ? Currently the tinderboxes are doing 'make installer' in objdir/mobile and hitting "*** No rule to make target `installer'.  Stop."

Comment 83

[Alex Pakhotin (:alexp)]

(In reply to comment #82)
> Could you clarify what the correct make command is with this new installer ?

Weird.
'make installer' in objdir/mobile works for me, as well as 'make package'.
Nothing has changed in this respect - the same command is used as it was for the CAB installer.

Comment 84

[Nick Thomas [:nthomas] (UTC+12)]

It'll be fallout from bug 515748, I'll reopen that.

Comment 85

[Joel Maher ( :jmaher ) (UTC -8)]

how can I turn this to verified?  Are we doing .cab files anymore?

Comment 86

[Alex Pakhotin (:alexp)]

This original bug was in fact converted to a new one, about the EXE-installer.
It probably makes sense just to change the description to something like "Replace slow CAB installer with self-extracting EXE", because this is what actually it is.

Comment 87

[Brad Lassey [:blassey] (use needinfo?)]

I think you can verify it with this new, more appropriate summary. The solution to the cab installation being slow was to not use it and develop our own self extracting installer.