Closed
Bug 292021
Opened 18 years ago
Closed 18 years ago
Software Update self-contained executable
Categories
(Toolkit Graveyard :: Security, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.8beta3
People
(Reporter: benjamin, Assigned: darin.moz)
References
()
Details
Attachments
(2 files, 6 obsolete files)
72.09 KB,
patch
|
benjamin
:
first-review+
benjamin
:
approval-aviary1.1a2+
|
Details | Diff | Splinter Review |
842 bytes,
patch
|
benjamin
:
first-review+
benjamin
:
approval1.8b3+
|
Details | Diff | Splinter Review |
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•18 years ago
|
||
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 | ||
Comment 2•18 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•18 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•18 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•18 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•18 years ago
|
||
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•18 years ago
|
Attachment #181914 -
Attachment is obsolete: true
Assignee | ||
Comment 7•18 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•18 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•18 years ago
|
||
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•18 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•18 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•18 years ago
|
||
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•18 years ago
|
Reporter | ||
Comment 13•18 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•18 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•18 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•18 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•18 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•18 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•18 years ago
|
||
bsmedberg: sounds good. i will make those changes.
Assignee | ||
Comment 20•18 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•18 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.
Comment 22•18 years ago
|
||
it would be great if we could use unicode throughout these apps and use MSLU...
Assignee | ||
Comment 23•18 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•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
Attachment #185540 -
Flags: first-review?(benjamin)
Reporter | ||
Comment 26•18 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•18 years ago
|
||
OK, revised patch.
Assignee | ||
Updated•18 years ago
|
Attachment #185541 -
Attachment is obsolete: true
Attachment #185623 -
Flags: first-review?(benjamin)
Reporter | ||
Updated•18 years ago
|
Attachment #185623 -
Flags: first-review?(benjamin) → first-review+
Reporter | ||
Comment 28•18 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•18 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•18 years ago
|
||
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 31•18 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•18 years ago
|
||
I checked in a fix for the VC8 Beta 2 build bustage.
Comment 33•18 years ago
|
||
*** Bug 297128 has been marked as a duplicate of this bug. ***
Comment 34•18 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•18 years ago
|
||
cls: --disable-updater should bypass the iconv requirement.
Assignee | ||
Comment 36•18 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.
Comment 37•18 years ago
|
||
Breaks BeOS-builds as it doesn't have implementation.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 38•18 years ago
|
||
Reporter | ||
Updated•18 years ago
|
Attachment #186339 -
Flags: first-review+
Attachment #186339 -
Flags: approval1.8b3+
Assignee | ||
Comment 39•18 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
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED
Comment 40•18 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•18 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.
Updated•7 years ago
|
Product: Toolkit → Toolkit Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•