Software Update self-contained executable

RESOLVED FIXED in mozilla1.8beta3

Status

Toolkit Graveyard
Security
--
major
RESOLVED FIXED
13 years ago
2 years ago

People

(Reporter: Benjamin Smedberg, Assigned: Darin Fisher)

Tracking

unspecified
mozilla1.8beta3
x86
All
Dependency tree / graph

Details

(URL)

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

13 years ago
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.
(Reporter)

Comment 1

13 years ago
Created attachment 181914 [details] [diff] [review]
Self-container updater, draft

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.
(Assignee)

Updated

13 years ago
Blocks: 292028
(Assignee)

Comment 2

13 years ago
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.
(Assignee)

Comment 3

13 years ago
>    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.
(Reporter)

Comment 4

13 years ago
> 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.
(Assignee)

Comment 5

13 years ago
> 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.
(Assignee)

Comment 6

13 years ago
Created attachment 182229 [details] [diff] [review]
Revised patch

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.
(Assignee)

Updated

13 years ago
Attachment #181914 - Attachment is obsolete: true
(Assignee)

Comment 7

13 years ago
> 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.
(Reporter)

Comment 8

13 years ago
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).
(Assignee)

Comment 9

13 years ago
Created attachment 182550 [details] [diff] [review]
Revised patch (probably broken on Linux)

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
(Reporter)

Comment 10

13 years ago
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.
(Assignee)

Comment 11

13 years ago
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.
(Assignee)

Comment 12

13 years ago
Created attachment 185092 [details] [diff] [review]
v1 patch

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)

Updated

13 years ago
Assignee: benjamin → darin
Severity: normal → major
Depends on: 296294, 296303
Target Milestone: --- → mozilla1.8beta3
(Reporter)

Comment 13

13 years ago
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-
(Assignee)

Comment 14

13 years ago
> 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
(Reporter)

Comment 15

13 years ago
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.
(Assignee)

Comment 16

13 years ago
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?
(Assignee)

Comment 17

13 years ago
> 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 :)
(Reporter)

Comment 18

13 years ago
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.
(Assignee)

Comment 19

13 years ago
bsmedberg: sounds good.  i will make those changes.
(Assignee)

Comment 20

13 years ago
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.
(Reporter)

Comment 21

13 years ago
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...
(Assignee)

Comment 23

13 years ago
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!).
(Assignee)

Comment 24

13 years ago
Created attachment 185540 [details] [diff] [review]
v2 patch

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)
(Assignee)

Comment 25

13 years ago
Created attachment 185541 [details] [diff] [review]
v2.1 patch

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)
(Assignee)

Updated

13 years ago
Attachment #185540 - Flags: first-review?(benjamin)
(Reporter)

Comment 26

13 years ago
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-
(Assignee)

Comment 27

13 years ago
Created attachment 185623 [details] [diff] [review]
v2.2 patch

OK, revised patch.
(Assignee)

Updated

13 years ago
Attachment #185541 - Attachment is obsolete: true
Attachment #185623 - Flags: first-review?(benjamin)
(Reporter)

Updated

13 years ago
Attachment #185623 - Flags: first-review?(benjamin) → first-review+
(Reporter)

Comment 28

13 years ago
Comment on attachment 185623 [details] [diff] [review]
v2.2 patch

Marking approvals based on previous discussions.
Attachment #185623 - Flags: approval-aviary1.1a2+
(Assignee)

Comment 29

13 years ago
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.
(Assignee)

Comment 30

13 years ago
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Comment 31

13 years ago
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
(Assignee)

Comment 32

13 years ago
I checked in a fix for the VC8 Beta 2 build bustage.

Comment 33

13 years ago
*** Bug 297128 has been marked as a duplicate of this bug. ***

Comment 34

13 years ago
(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.
(Assignee)

Comment 35

13 years ago
cls: --disable-updater should bypass the iconv requirement.
(Assignee)

Comment 36

13 years ago
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.
(Assignee)

Updated

13 years ago
Depends on: 297609

Comment 37

13 years ago
Breaks BeOS-builds as it doesn't have implementation.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 38

13 years ago
Created attachment 186339 [details] [diff] [review]
Fixes BeOS bustage.
(Reporter)

Updated

13 years ago
Attachment #186339 - Flags: first-review+
Attachment #186339 - Flags: approval1.8b3+
(Assignee)

Comment 39

13 years ago
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
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED

Comment 40

13 years ago
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.)
(Assignee)

Comment 41

13 years ago
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.