Closed Bug 479361 Opened 14 years ago Closed 14 years ago

Replace hack to get available space in CheckDiskSpace with GetDiskFreeSpaceExW

Categories

(Toolkit :: NSIS Installer, defect, P2)

x86
Windows Vista
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: robert.strong.bugs, Assigned: robert.strong.bugs)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 3 obsolete files)

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
Attached patch patch rev1 (obsolete) — Splinter Review
Attachment #363281 - Flags: review?(jmathies)
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.
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.
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.
Flags: wanted1.9.1?
Priority: -- → P2
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.
Status: NEW → ASSIGNED
Thanks for that, maybe Bug 434338 (SeaMonkey only) will then be fixed (I did not look at that bug too closely).
(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.
Note: this patch should fix the bugs this blocks except for bug 479406 which I found while fixing this.
I just verified that this patch does respect disk quotas.
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.
Attachment #363281 - Attachment is obsolete: true
Attachment #363281 - Flags: review?(jmathies)
Frank, I've verified this gets the correct size based on the optional components that are selected.
Attached patch patch rev2 (obsolete) — Splinter Review
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.
Attachment #363524 - Flags: review?(jmathies)
Comment on attachment 363524 [details] [diff] [review]
patch rev2

Found another bug with using a UNC root path (e.g. \\server\share\)
Attachment #363524 - Attachment is obsolete: true
Attachment #363524 - Flags: review?(jmathies)
Attached patch patch rev3 (obsolete) — Splinter Review
See comment #12 for what this fixes.
Attachment #363525 - Flags: review?(jmathies)
+      ; 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.
(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!
Attachment #363525 - Flags: review?(jmathies) → review+
Will check in today
Attachment #363525 - Attachment is obsolete: true
Attachment #363947 - Flags: review+
Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/de28e36225a6
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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.
Attachment #363947 - Flags: approval1.9.1?
Attachment #363947 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 363947 [details] [diff] [review]
patch - checked in

a191=beltzner
Flags: wanted1.9.1?
You need to log in before you can comment on or make changes to this bug.