238741 - ExistAllXPIs should check that the .xpi files are actually ok

Status: RESOLVED FIXED

(SeaMonkey :: Installer, defect)

Description

[Andrew Schultz]

If you use the net installer, and cancel part-way through the last one, the
installer leaves behind a marker file, but the installer ignores the marker
because ExistAllXPIs (called from nsInstaller:Show) concludes that everything is
finished simply because all the files exist.  nsXIEngine::Download then skips
the files and barfs (either by trying to redownload without the UI properly
initted, or by trying to install an incomplete .xpi).

Comment 1

[Andrew Schultz]

Attachment: patch

this makes VerifyArchive a helper function and uses it to check the XPIs for
zip-validity.  Any incomplete downloads are clobbered since there is no way to
tell what the real file size is supposed to be (at least for talkback).

it also makes the number displayed in the GUI reasonable for cases like this.
(1/1 instead of 9/1 when you're just downloading Venkman).

Comment 2

[Benjamin Smedberg]

Comment on attachment 144942 [details] [diff] [review]
patch

I don't get it. Why are you moving this function out of the nsXIEngine class
but not making it static? Keep things the way they are unless you have a good
reason not to.

Comment 3

[Andrew Schultz]

Comment on attachment 144942 [details] [diff] [review]
patch

I've got a new patch

Comment 4

[Andrew Schultz]

Attachment: patch2

just make VerifyArchive static.  It has to be static and/or not a member to
compile since ExistAllXPIs is static.

Also, I noticed running with the previous patch that there's a substantial perf
hit.  ExistAllXPIs was being called 3 times when the XPIs were already there
(SEA installer).  This patch sets bDownload the first time and only calls
engine->Download() if bDownload is TRUE.

Comment 5

[Andrew Schultz]

Comment on attachment 146821 [details] [diff] [review]
patch2

ugh.
if you choose one setup type that has all the XPIs and go forward, then back,
change to a setup type that needs more XPIs that are available, the current
installer will download them, but won't show the proxy settings panel.

with this patch, they wouldn't be downloaded.

Comment 6

[Andrew Schultz]

Attachment: patch v3

this one works, promise!

in addition to what I mentioned previously, the previous patch was not setting
mTotalComps for the SEA installer because it never called Download. 
mTotalComps was only needed by nsXIEngine::Install, so I moved it there and
deleted the member variable.

Also, nsComponentList::GetLengthSelected was totally broken.  I'm guessing the
scary comment is there because it was broken.  Nothing currently uses the
method.

Comment 7

[Benjamin Smedberg]

Comment on attachment 147526 [details] [diff] [review]
patch v3

>@@ -586,50 +578,48 @@ nsInstallDlg::PerformInstall()
>-    comps = gCtx->sdlg->GetSelectedSetupType()->GetComponents(); 
>-    if (!comps)
>-    {
>-        ErrorHandler(E_NO_COMPONENTS);
>-        return E_NO_COMPONENTS;
>-    }
>+    comps = gCtx->sdlg->GetSelectedSetupType()->GetComponents();

Can you explain why the (!comps) check is not needed? Other than that, r=me

Comment 8

[Andrew Schultz]

> Can you explain why the (!comps) check is not needed? Other than that, r=me

The check is in PerformInstall.  The components parser checks for lack of
components and bails if that is the case.  In fact, that is the only way the
component list could be NULL.  Furthermore, the component list is used elsewhere
(before PerformInstall) without checking it first.

In general, I find the installer code overly-paranoid.

Comment 9

[Andrew Schultz]

Attachment: patch merged to trunk

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 148103 [details] [diff] [review]
patch merged to trunk

sr=dveditz

Comment 11

[Andrew Schultz]

Attachment: patch re-merged to trunk

This changes quite a bit, but should mainly affect doing a net install with
some but not all xpis, which is currently completely broken (and this patch
fixes it work well).

I tried a bunch of different install types for stub / SEA installer with this
patch applied and coudln't find anything that was broken.