Closed Bug 292021 Opened 19 years ago Closed 19 years ago

Software Update self-contained executable

Categories

(Toolkit Graveyard :: Security, defect)

x86
All
defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.8beta3

People

(Reporter: benjamin, Assigned: darin.moz)

References

()

Details

Attachments

(2 files, 6 obsolete files)

This bug is for the self-contained executable for the Firefox 1.1 update plan.
It needs to be small, support binary patching, etc.

http://wiki.mozilla.org/Software_Update
http://wiki.mozilla.org/Firefox:1.1_Software_Update_Upgrades

I am attaching what I've got here. It builds and runs on mac. The binary
patching code is untested, because I (or someone) needs to update "bsdiff" to
add the checksum and remove bzip2, and make everything 32bit. But it's a start,
and it's small enough to make me happy.
Attached patch Self-container updater, draft (obsolete) — Splinter Review
The updater/ bits of this patch build on mac, and appear to basically work.
The other-licenses/bsdiff parts do not build.

The first priority here is to get bsdiff working with the same header/32bit
types as bspatch. Since I'm going to be having a new baby tomorrow, and this is
fairly urgent, I welcome assistance with that task. If you do start on
something, please comment to let me know, so that we don't duplicate activity.
Blocks: 292028
FYI, I'm working on an revised version of this patch while bsmedberg is on
vacation.  For the most part, I'm very happy with the current patch.

I'm adding the following pieces:

 * Recover better from errors from Execute steps:
   1)  Add Finish step to each Action after all Execute steps have run.
   2)  Make backup copies of all files before removing any files.
   3)  Restore backups in Finish step if any Execute step failed.

 * Implement UI.  For this task, my plan is as follows:
   1)  Move realmain onto a background thread.
   2)  Record state of realmain in some global progress variables.
   3)  Protect global progress variables in a mutex.
   4)  Implement native progress UI that polls progress variables.

I also had to remove the submanifest action because I don't have an
implementation of fork, though I could probably spawn a thread for that given
the architecture described above.  I'm just not sure that there is any need for
a submanifest action at this point.
>    1)  Move realmain onto a background thread.

Actually, nevermind that.  I'm going start off by avoiding any multithreading. 
That's probably overkill for this application.  We should have the opportunity
to update the UI often enough such that this sort of thing would be unnecessary.

Another TODO item is adding code to the updater to make it wait for its parent
process to exit.  That way we can be sure that firefox.exe is not running when
we start patching it.  I'm thinking of passing the process ID of the parent as a
command line flag to the updater.
> Another TODO item is adding code to the updater to make it wait for its parent
> process to exit.  That way we can be sure that firefox.exe is not running when

For *nix/windows this is not necessary... we can just execv Firefox into the
update executable. On mac this will require more testing, and perhaps the
parent-waiting code you describe.
> For *nix/windows this is not necessary... we can just execv Firefox into the
> update executable. On mac this will require more testing, and perhaps the
> parent-waiting code you describe.

Ah, yeah.. that does make sense.  By the way, I have windows progress UI
working.  I did end up putting realmain on a background thread for simplicity --
yeah, it made things simpler.  The resulting updater.exe is less than 20k in an
optimized build.
Attached patch Revised patch (obsolete) — Splinter Review
OK, here's a revised version of the updater.  It has UI for Windows and GTK2. 
I've only verified the ADD function.  I verified that I was able to implement
"cp -r" using this tool ;-)

Remaining work:

1) Support re-launching firefox with command line flag

2) Add OSX progress UI

3) Test patching and file removal

4) Make the progress increments make sense.  Right now, I slice it into thirds:
1/3 for Prepare, 1/3 for Execute, and 1/3 for Finish, but Execute dominates in
terms of time, so the progress meter progresses in a funny way.
Attachment #181914 - Attachment is obsolete: true
> For *nix/windows this is not necessary... we can just execv Firefox into the
> update executable. On mac this will require more testing, and perhaps the
> parent-waiting code you describe.

So, I think that MS's execv might not work the way we think or hope that it
does.  It appears to call CreateProcess and then call _exit(0) on the process
that called execv.  Therefore, I think there would be a race between the new
process's WinMain and the call to _exit(0), which I presume is what would
release "firefox.exe" to allow it to be overwritten.  It may be difficult to
lose this race in practice, but it might bite us if we aren't careful.  Under
UNIX, it would be nice to use execv because it allows people running firefox-bin
to wait on the process and have that correctly correspond to waiting for firefox
"the application" to close.  But, that is not really needed on Windows, so if we
need to do the --wait-pid=PID thing on Windows, then so be it.
Agreed. By the way, I plan to examine why we had to switch from execv to the
MacLaunchHelper code in the first place... I think that switching back to execv
on the mac (with the appropriate problems fixed) could fix a number of annoying
mac bugs (the disappearing/reapparing bouncy globe, among others).
This patch is the latest from my tree.	Not sure when bsmedberg may be back to
start thinking about this stuff again, so I wanted to get my latest patch up
here.

I've revised the updater UI to only appear if the update is taking a while to
process.  In other words, for very short updates, we won't show any UI at this
level.	The UI will be shown by the update service if needed.

I've used this code to apply a complete firefox ZIP archive into a directory.

This probably only compiles on Windows.  I need to spend some time on Linux and
OSX making things happy there.
Attachment #182229 - Attachment is obsolete: true
This looks decent to me. I'm interested in getting something checked in on the
software update branch soon, so that I can play. We also talked about not
extracting the ZIP files, and letting this executable extract them as it uses
them: this means linking against standalone libjar, but that's not a big hit.
Yeah, I'm close to having the zipreader stuff incorporated.  Not bad so far at
least.  I'll land this on the branch once I have that in shape.
Attached patch v1 patch (obsolete) — Splinter Review
This verison of the updater works on Windows, Linux, and Mac OSX.  Mac OSX is
missing platform specific progress meter UI.  Josh Aas is working on that.  For
now, we just perform the update completely silently on Mac OSX (and any other
platform for that matter).

This patch depends on libmar and libbz2.
Attachment #182550 - Attachment is obsolete: true
Attachment #185092 - Flags: first-review?(benjamin)
Assignee: benjamin → darin
Severity: normal → major
Depends on: 296294, 296303
Target Milestone: --- → mozilla1.8beta3
Comment on attachment 185092 [details] [diff] [review]
v1 patch

1) in the GTK AutoFILE impl, you need to null-check fp_ in the destructor:
fclose(0) will crash on some OSes (BSD).

2) in main(), the usage statement should not be #ifdef DEBUG

3) why don't we wait-pid on mac also? We can't use execv there because of the
threading problems so I think we have to use waitpid.

4) Do we have a localization story for updater.ini? It is hardcoding "Firefox".
I think this should be in browser/locales and mail/locales, perhaps with a
hardcoded generic English text if the ini file is missing.
Attachment #185092 - Flags: first-review?(benjamin) → first-review-
> 1) in the GTK AutoFILE impl, you need to null-check fp_ in the destructor:
> fclose(0) will crash on some OSes (BSD).

ok


> 2) in main(), the usage statement should not be #ifdef DEBUG

ok... i didn't see any reason to output it in optimized builds.  but, i guess
someone might want to run it from the command line.


> 3) why don't we wait-pid on mac also? We can't use execv there because of the
> threading problems so I think we have to use waitpid.

ok, sounds reasonable.


> 4) Do we have a localization story for updater.ini?

I figured localizing this file would be similar to how we localize the INI files
for the installers.  Why is that not okay?
Status: NEW → ASSIGNED
We localize the installer.ini files from a makefile in browser/locales (which
calls into toolkit/locales). I really think we should ship this file from that
directory.
bsmedberg: can you please outline what needs to be done exactly to localize
updater.ini?  i sort of get what's going on, but it looks like i need to do some
preprocessing steps and call iconv as well.  what's the recommended approach
exactly?  can you help with this task?
> 3) why don't we wait-pid on mac also? We can't use execv there because of the
> threading problems so I think we have to use waitpid.

what threading problem?  clue me in :)
What charset is the ini file? it looks like utf8 on gtk2 and the "platform"
charset on windows. I'm willing to ignore that issue to get this landed, but
we're going to need to fix it.

I suggest that we just put the ini file in browser/locales/en-US/update.ini and
ship it with the same rule we use for README.txt for the time being.

The reason we use the MacLaunhHelper on mac is because execv fails once the mac
windowing system has been initialized: execv apparently doesn't work on mac if
there are non-main threads still running, and the windowing system uses a
background thread to process events.
bsmedberg: sounds good.  i will make those changes.
On the topic of charsets, you are right.  If we assume that localizers are
creating updater.ini using UTF-8, then we need to run it through iconv to
convert to the platform charset before shipping it with a localized Windows
build of Firefox.  I guess that means using the same system that we are using
for installer.inc currently.
Right now iconv is only a build-time requirement for the installer: we will need
to update the build docs and make the proper noise on the newsgroups and blogs
that iconv is now a build-time requirement for anyone building toolkit apps.
it would be great if we could use unicode throughout these apps and use MSLU...
Perhaps I should create an --enable-updater option to configure.  That will
probably be appreciated by folks who either use Mozilla on a platform where the
updater doesn't build or by folks who simply don't want to build the updater
(hello Minimo!).
Attached patch v2 patch (obsolete) — Splinter Review
OK, this version implements --enable-updater.  It moves updater.ini into
browser/locales/en-US, and adds a rule to browser/locale/Makefile.in to run
updater.ini through iconv.  It only does this if MOZ_UPDATER is defined.

This patch also contains changes to client.mk, Makefile.in, and allmakefiles.sh

to build the various dependencies of the updater system: libmar, libbz2, and
bsdiff.
Attachment #185092 - Attachment is obsolete: true
Attachment #185540 - Flags: first-review?(benjamin)
Attached patch v2.1 patch (obsolete) — Splinter Review
The last patch was missing configure.in changes.

FYI, this entire patch is checked in on the SOFTWARE_UPDATE_20050428_BRANCH.
Attachment #185540 - Attachment is obsolete: true
Attachment #185541 - Flags: first-review?(benjamin)
Attachment #185540 - Flags: first-review?(benjamin)
Comment on attachment 185541 [details] [diff] [review]
v2.1 patch

In configure.in, you need to set a default value for MOZ_UPDATER: I think it
should be on-by-default for MOZ_XUL_APP.

Be careful not to checkin the client.mk changes.

In Makefile.in, it should be "Cross-compiling the mbsdiff executable is not yet
supported." But, I don't think we should compile it by default at all: it's a
tainted license, and should require an opt-in build rule.
Attachment #185541 - Flags: first-review?(benjamin) → first-review-
Attached patch v2.2 patchSplinter Review
OK, revised patch.
Attachment #185541 - Attachment is obsolete: true
Attachment #185623 - Flags: first-review?(benjamin)
Attachment #185623 - Flags: first-review?(benjamin) → first-review+
Comment on attachment 185623 [details] [diff] [review]
v2.2 patch

Marking approvals based on previous discussions.
Attachment #185623 - Flags: approval-aviary1.1a2+
I've modified updater/Makefile.in to only build progressui_gtk.cpp when
MOZ_ENABLE_GTK2 is defined, because that code makes use of several GTK2 specific
functions, including the new and much saner progressbar API.  If someone wants
to port it to GTK1 they can.  For now, GTK1 builds will just use progressui_null.
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
I'm getting this compilation error (with VS8 Beta2)


updater.cpp
c:/mozilla/trunk/mozilla/toolkit/mozapps/update/src/updater/updater.cpp(240) : e
rror C2440: 'initializing' : cannot convert from 'const char *' to 'char *'
        Conversion loses qualifiers
c:/mozilla/trunk/mozilla/toolkit/mozapps/update/src/updater/updater.cpp(465) : e
rror C2440: 'initializing' : cannot convert from 'const char *' to 'char *'
        Conversion loses qualifiers
make[7]: *** [updater.obj] Error 2
I checked in a fix for the VC8 Beta 2 build bustage.
*** Bug 297128 has been marked as a duplicate of this bug. ***
(In reply to comment #21)
> Right now iconv is only a build-time requirement for the installer: we will need
> to update the build docs and make the proper noise on the newsgroups and blogs
> that iconv is now a build-time requirement for anyone building toolkit apps.

I ran into this today on win32 as well.  You should add a configure check for
this since it's a new mandatory requirement.
cls: --disable-updater should bypass the iconv requirement.
Oh,... you're saying that we should make configure complain if --disable-updater
is not specified and iconv cannot be found.  That does sound like a good idea.
Depends on: 297609
Breaks BeOS-builds as it doesn't have implementation.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #186339 - Flags: first-review+
Attachment #186339 - Flags: approval1.8b3+
Please do not reopen this bug for platform specific work like this.  File a new
bug.  Unsupported platforms should be compiled with --disable-updater.
Status: REOPENED → RESOLVED
Closed: 19 years ago19 years ago
Resolution: --- → FIXED
Sorry, there was no way of knowing that. (It might be good to note things like
that along with the error that stops the build. Saves everyone a lot of trouble.)
Yeah, sorry about that.  I'm trying to move quickly, and as a result
communication about --disable-updater hasn't been so good.
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: