479361 - Replace hack to get available space in CheckDiskSpace with GetDiskFreeSpaceExW

Status: RESOLVED FIXED

(Firefox :: Installer, defect, P2)

Description

[Robert Strong (they/them - no direct email)]

When the Mozilla apps supported Win9x the installer had to use a clumsy method for getting available disk space. Now the all apps support Win2K and above GetDiskFreeSpaceExW can be used.

The hack may be the cause of the bugs that this bug blocks

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Comment 2

[Robert Strong (they/them - no direct email)]

Comment on attachment 363281 [details] [diff] [review]
patch rev1

Hey Jim, I think this should fix the bugs this blocks but there are a couple of added benefits. First, this respects disk quotas. Second, this makes it so the installer works with UNC paths. Third, SeaMonkey uses more than 3 install sections and they use this macro so there required space calculation is incorrect.

I've tested everything but the disk quotas which I will test tomorrow.

Comment 3

[Robert Strong (they/them - no direct email)]

Frank, I noticed that you have more than three install sections and wanted to give you a heads up that the CheckDiskSpace only supports the first 3. This patch fixes that as well as the other items mentioned in comment #2.

Comment 4

[Robert Strong (they/them - no direct email)]

Hey Mike, the patch in this bug fixes several issues (see comment #2) that I think justify wanted1.9.1 or even blocking1.9.1 but I'll leave that up to you.

Comment 5

[Robert Strong (they/them - no direct email)]

Forgot to add that this will also get the correct free space when the path has a junction point to a different partition which wasn't the case before.

Comment 6

[Frank Wein [:mcsmurf]]

Thanks for that, maybe Bug 434338 (SeaMonkey only) will then be fixed (I did not look at that bug too closely).

Comment 7

[Robert Strong (they/them - no direct email)]

(In reply to comment #6)
> Thanks for that, maybe Bug 434338 (SeaMonkey only) will then be fixed (I did
> not look at that bug too closely).
That is quite likely.

Comment 8

[Robert Strong (they/them - no direct email)]

Note: this patch should fix the bugs this blocks except for bug 479406 which I found while fixing this.

Comment 9

[Robert Strong (they/them - no direct email)]

I just verified that this patch does respect disk quotas.

Comment 10

[Robert Strong (they/them - no direct email)]

Comment on attachment 363281 [details] [diff] [review]
patch rev1

While writing the patch for bug 479406 I noticed that getting the free space for the root of a UNC path was extremely slow which occurs in this macro so I am going to fix that and bug 479406 in this patch.

Comment 11

[Robert Strong (they/them - no direct email)]

Frank, I've verified this gets the correct size based on the optional components that are selected.

Comment 12

[Robert Strong (they/them - no direct email)]

Attachment: patch rev2

What this patch does:
* Replaces the Win95 hack for getting freespace with GetDiskFreeSpaceExW. This makes it so the correct available freespace is returned when there are user quotas and when the path is through a junction point where the directory lives on a different partition than the root of the path.
* Allows installing to a UNC path including the root of a UNC path (e.g. \\server\share\)
* Enumerates all sections for their size. Previously the CheckDiskSpace macro was limited to the first three sections.
* Checks for the ability to create and delete both files and directories since one can be present without the other.

Comment 13

[Robert Strong (they/them - no direct email)]

Comment on attachment 363524 [details] [diff] [review]
patch rev2

Found another bug with using a UNC root path (e.g. \\server\share\)

Comment 14

[Robert Strong (they/them - no direct email)]

Attachment: patch rev3

See comment #12 for what this fixes.

Comment 15

[Jim Mathies [:jimm]]

+      ; The directory passed to GetDiskFreeSpaceExW must exist for the call to
+      ; succeed.  Since the CanWriteToInstallDir macro is called prior to this
+      ; macro the call to CreateDirectory will always succeed.
+
+      ; IfFileExists returns false for $INSTDIR when $INSTDIR is the root of a
+      ; UNC path so always try to create $INSTDIR
+      CreateDirectory "$INSTDIR\"
+      GetTempFileName $R7 "$INSTDIR\"
+      Delete "$R7"
+      CreateDirectory "$R7"

I'm a little confused about this. The top comment says CreateDirectory will always succeed, but the lower comment says we have to create the install dir to make sure it'll succeed. 

Also, we create $INSTDIR, then create a temp file, delete that, and then create a temp directory here of the same name. I assume we're performing some sort of privileges check, but I don't understand why from the comments - I think there's some cruft in the comments for CheckDiskSpace - 

+ * $R7 = the counter for enumerating the sections
+ *       the temporary file name for the directory created under $INSTDIR passed
+ *       to GetDiskFreeSpaceExW.
+ *       path (e.g. \\server\share\)

I think that first line is cruft left over from previous work.

Comment 16

[Robert Strong (they/them - no direct email)]

(In reply to comment #15)
> +      ; The directory passed to GetDiskFreeSpaceExW must exist for the call to
> +      ; succeed.  Since the CanWriteToInstallDir macro is called prior to this
> +      ; macro the call to CreateDirectory will always succeed.
> +
> +      ; IfFileExists returns false for $INSTDIR when $INSTDIR is the root of a
> +      ; UNC path so always try to create $INSTDIR
> +      CreateDirectory "$INSTDIR\"
> +      GetTempFileName $R7 "$INSTDIR\"
> +      Delete "$R7"
> +      CreateDirectory "$R7"
> 
> I'm a little confused about this. The top comment says CreateDirectory will
> always succeed, but the lower comment says we have to create the install dir to
> make sure it'll succeed. 
I don't think I've had enough coffee yet... this is how I see the above two comments.

The top comment states that "the directory passed to GetDiskFreeSpaceExW must exist for the call" to GetDiskFreeSpaceExW to "succeed" and that "Since the CanWriteToInstallDir macro is called prior to this macro the call to CreateDirectory will always succeed".

The bottom comment states that "IfFileExists returns false for $INSTDIR when $INSTDIR is the root of a UNC path so always try to create $INSTDIR". So, it is necessary to always try to create the $INSTDIR since we can't rely on IfFileExists to tell us if it exists or not when $INSTDIR is the root of a UNC path.

> Also, we create $INSTDIR, then create a temp file, delete that, and then create
> a temp directory here of the same name. I assume we're performing some sort of
> privileges check, but I don't understand why from the comments - I think
> there's some cruft in the comments for CheckDiskSpace - 
Per the macro description
"Checks whether it is possible to create and delete a directory and a file in the install directory. Creation and deletion of files and directories are checked since a user may have rights for one and not the other."

Can you point out the cruft besides the one below? Once again, I think I need more coffee.

> + * $R7 = the counter for enumerating the sections
> + *       the temporary file name for the directory created under $INSTDIR
> passed
> + *       to GetDiskFreeSpaceExW.
> + *       path (e.g. \\server\share\)
> 
> I think that first line is cruft left over from previous work.
Yep

Thanks!

Comment 17

[Robert Strong (they/them - no direct email)]

Attachment: patch - checked in

Will check in today

Comment 18

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/de28e36225a6

Comment 19

[Robert Strong (they/them - no direct email)]

Comment on attachment 363947 [details] [diff] [review]
patch - checked in

Drivers, requesting 1.9.1 approval. This patch fixes:
* Replaces the Win95 hack for getting freespace with GetDiskFreeSpaceExW. This
makes it so the correct available freespace is returned when there are user quotas and when the path is through a junction point where the directory lives on a different partition than the root of the path.
* Allows installing to a UNC path including the root of a UNC path (e.g.
\\server\share\)
* Enumerates all sections for their size. Previously the CheckDiskSpace macro
was limited to the first three sections.
* Checks for the ability to create and delete both files and directories since
one can be present without the other.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 363947 [details] [diff] [review]
patch - checked in

a191=beltzner

Comment 21

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-1.9.1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/90666837bc86