168139 - Provide progress updates for external GRE app installers

Status: VERIFIED FIXED

(Core Graveyard :: Installer: GRE, defect, P1)

Description

[[e-mail] (away - not reading bugmail)]

GRE installer should provide a facility for updating the external application
installer it was spawned by.

Progress updates could be provided by calling a well-defined exported library
procedure in a proxy DLL provided by the external GRE app installer. This proxy
DLL would be ideally located in Windows temp directory. The suggested naming
convention is as follows: "ProgUpd.dll" for the proxy DLL and
"StdUpdateProgress" for the exported procedure.

A functional patch following this brief outline is to follow soon.

Comment 1

[[e-mail] (away - not reading bugmail)]

Attachment: first functional patch

Comment 2

[[e-mail] (away - not reading bugmail)]

Attachment: patch v1

Curt, would you have a minute to look at this? This patch iteration should be
r=/sr= worthy.

Comment 3

[Curt Patrick (gone)]

Comment on attachment 98920 [details] [diff] [review]
patch v1

Looks like maybe a couple tabs slipped into the following lines"?:

+	DWORD dwLen = GetTempPath(MAX_BUF, szProxyDLLPath);
+	
+  if (szProxyDLLPath[dwLen - 1] != '\\')
+	strcat(szProxyDLLPath, "\\");

Otherwise, it looks okay to me.  r=curt

Comment 4

[[e-mail] (away - not reading bugmail)]

Attachment: patch v2

Curt, thanks for catching the tabs!

Comment 5

[[e-mail] (away - not reading bugmail)]

Comment on attachment 98950 [details] [diff] [review]
patch v2

transferring r=curt

Comment 6

[[e-mail] (away - not reading bugmail)]

Dan, would you have some time to sr= this?

Comment 7

[Daniel Veditz [:dveditz]]

I don't understand how this works. If you call LoadLibrary() you get your own
instance of the .dll, how is that supposed to pass information back to the
calling installer?

nsinstall is just a quick unpacker, does it really need to pass status?

What are the magic numbers 40 and 51?

Since this is a windows-only program maybe a user-defined window message would
work. If the calling program wants them it launches the GRE installer with an
argument passing in a window handle to target messages to.

Comment 8

[[e-mail] (away - not reading bugmail)]

>I don't understand how this works. If you call LoadLibrary() you get your own
>instance of the .dll, how is that supposed to pass information back to the
>calling installer?

Dan, this DLL is supposed to be a proxy to the external installer. We don't know
how it passes information to the installer itself, it might be creating Windows
events the installer is listening to or follow some other route we haven't
considered here. 

I suspect that this indirection is used to hide implementation details of the
external installer and possibly also  for security reasons.

>nsinstall is just a quick unpacker, does it really need to pass status?

The external installer could impose stringent requirements on the frequency of
updates. If we have a lot of files to unpack and the are allowed to have say a
maximum of 7 s lapse between individual updates, then yes the unpacker has to
send updates as well.

I'll try to address the remaining questions once we agree on the design I was
trying to pursue here. I was following a certain model, which might - or might
not - turn out to be generic enough. I cannot really comment on its quality
compared to the design ideas you have suggested. They seem valid and appropriate
to me...

Comment 9

[[e-mail] (away - not reading bugmail)]

Attachment: patch v3, which reflects Dan's suggestions

Dan,

I've dropped nsinstall from the patch - that simplified it quite a bit. Would
you have time for another look?

Comment 10

[Curt Patrick (gone)]

Comment on attachment 100064 [details] [diff] [review]
patch v3, which reflects Dan's suggestions

r=curt

Patch v2 is obsolete, isn't it?

Comment 11

[[e-mail] (away - not reading bugmail)]

Comment on attachment 98950 [details] [diff] [review]
patch v2

obsoleting - thanks Curt!

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 100064 [details] [diff] [review]
patch v3, which reflects Dan's suggestions

>+//GRE app installer progress bar integration
>+typedef LONG (CALLBACK* LPFNDLLFUNC)(LPCTSTR,INT);

Please doc what the args mean here.

>+  if((szProxyDLLPath = NS_GlobalAlloc(MAX_BUF)) == NULL)
>+    return(1);

4k is overkill for this buffer. What's wrong with MAX_PATH?

>+  GetTempPath(MAX_BUF, szProxyDLLPath);
>+  AppendBackSlash(szProxyDLLPath, MAX_BUF);
>+  strcat(szProxyDLLPath, GRE_APP_INSTALLER_PROXY_DLL);

This is an unsafe action, an unchecked strcat() could overflow
the buffer. The 4k buffer allows you to be sloppy like this, but
that will only lead to bad habits when you don't have that cushion
and further encourages obscenely large 4k buffers.

>+  if (FileExists(szProxyDLLPath) != FALSE)
>+    hGREAppInstallerProxyDLL = LoadLibrary(szProxyDLLPath);
>+  
>+  if (hGREAppInstallerProxyDLL != NULL)
>+    lpfnProgressUpd = (LPFNDLLFUNC) GetProcAddress(hGREAppInstallerProxyDLL,

LoadLibrary() doesn't return null on failure.

What if FileExists() does return false? You won't set hGREApp...,
but since you didn't initialize it you can't rely on your next test.
And since lpfn... isn't initialized no one later can tell whether
that worked or not.

>+  if (hGREAppInstallerProxyDLL != NULL) 
>+    FreeLibrary(hGREAppInstallerProxyDLL);

Windows "HANDLES" aren't pointers so checking against NULL
isn't right. Use 0 instead for non-pointer types, but as
mentioned above most error values in this case are non-zero
so you'll have to do something else anyway.

>           UpdateGaugeArchiveProgressBar((unsigned)(((double)(dwCurrentArchive)/(double)dwTotalArchives)*(double)100));
>+          UpdateGREAppInstallerProgress((unsigned)(((double)(dwCurrentArchive)/(double)dwTotalArchives)*(double)100));

If you're going to use this value twice it would be better to
create a variable and only do the relatively expensive division
and multiplication once. This will have the nice side-effect of
salvaging the readability of these lines -- it should have used
a separate variable originally on those grounds alone.

Ditto the next two places like this.

This is probably not going to give enough feedback. How many archives
are part of the GRE? Looks like one in the config.it, so these
statements will send back the status 1/1*100 == 100, once. Probably
not what's wanted.

cbXPIProgress is called by the engine both during the "prepare" step
(max = 0 -- no way to know how long it'll take) and during the
"complete" step (when a percentage makes sense). If it's necessary
for the target installer to get real feedback during the prepare
stage you will have to fake it: assign "prepare" step half the progress,
keep count of how many times it's called and divide by some fixed
number from the config.it you can divide by. Going slightly over
is OK, no one's going to notice a jump from 47% to 50% when the next
phase starts.  If this works you can eliminate UpdateGaugeFileBarber()
and split this the same way for values of UpdateFileProgressBar().

During prepare step (max = 0):
  filePercent  = (val/prepMax) * 50

During complete step (max > 0)
  filePercent = ((val/max) * 50) + 50)

both:
  gaugePercent = (filePercent * (currArchive/maxArchives))
		 + (currArchive-1)/maxArchives

(assumes currArchive starts w/1, if 0 then adjust both terms)

Comment 13

[[e-mail] (away - not reading bugmail)]

Arg, time is not on our side. I wished I had paid more attention to this. 

Dan, you might be right when saying that Windws handles are not pointers, but I
got the code straight out of MSDN - specifically to save time and effort on
standard code like this. MSDN recommends checking against NULL and claims that
LoadLibrary return NULL on failure.

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/vccore/html/_core_getprocaddress.asp

typedef UINT (CALLBACK* LPFNDLLFUNC1)(DWORD,UINT);
...

HINSTANCE hDLL;               // Handle to DLL
LPFNDLLFUNC1 lpfnDllFunc1;    // Function pointer
DWORD dwParam1;
UINT  uParam2, uReturnVal;

hDLL = LoadLibrary("MyDLL");
if (hDLL != NULL)
{
   lpfnDllFunc1 = (LPFNDLLFUNC1)GetProcAddress(hDLL,
                                           "DLLFunc1");
   if (!lpfnDllFunc1)
   {
      // handle the error
      FreeLibrary(hDLL);
      return SOME_ERROR_CODE;
   }
   else
   {
      // call the function
      uReturnVal = lpfnDllFunc1(dwParam1, uParam2);
   }
}

Comment 14

[[e-mail] (away - not reading bugmail)]

I'm still debating how to pass more frequent updates back to the external
installer. GRE installer with the current patch applied sends two updates back
to the caller, one for each xpi archive. My tests indicate that on an average
machine the largest gap between updates is ~14 s. This could be too long for
some uses and we might want to increase the frequency of progress updates.

I agree with Dan that having a constant into config.ini might be a good fix. We
could use it to divide the time intervals between archive updates. Better yet,
we should know how many files each xpi archive contains and provide updates for
each archive installation.

Sean made a comment about it in bug 11210
http://bugzilla.mozilla.org/show_bug.cgi?id=11210#c30

I will look into it before presenting an updated patch. It would help us to get
rid of the progress bar "sloshing" down the road.

Comment 15

[Daniel Veditz [:dveditz]]

I'm embarrased to admit that my LoadLibrary comment was based on my win16
programming days where low values were error codes rather than handles. You're
right, back in the real world LoadLibrary() returns null (good thing, everyone
tried to use it that way anyway and were always getting tripped up).

Comment 16

[[e-mail] (away - not reading bugmail)]

raising priority, we really need this ASAP. I'll post an updated patch (sans the
changes needed to provide more frequent updats) this afternoon.

I really hope we can get this is before the trunk freeze tonight.

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 100064 [details] [diff] [review]
patch v3, which reflects Dan's suggestions

sr=dveditz

Comment 18

[[e-mail] (away - not reading bugmail)]

Curt, I'm closing this. We might want to file a follow-up bug to iron out some
of the implementation details. I'd be happy to work on those during the 1.2 freeze.

Comment 19

[Charles Rosendahl]

verified