Closed
Bug 479361
Opened 16 years ago
Closed 16 years ago
Replace hack to get available space in CheckDiskSpace with GetDiskFreeSpaceExW
Categories
(Firefox :: Installer, defect, P2)
Tracking
()
RESOLVED
FIXED
People
(Reporter: robert.strong.bugs, Assigned: robert.strong.bugs)
References
Details
(Keywords: fixed1.9.1)
Attachments
(1 file, 3 obsolete files)
9.58 KB,
patch
|
robert.strong.bugs
:
review+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
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
![]() |
Assignee | |
Comment 1•16 years ago
|
||
![]() |
Assignee | |
Updated•16 years ago
|
Attachment #363281 -
Flags: review?(jmathies)
![]() |
Assignee | |
Comment 2•16 years ago
|
||
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.
![]() |
Assignee | |
Comment 3•16 years ago
|
||
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.
![]() |
Assignee | |
Comment 4•16 years ago
|
||
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
![]() |
Assignee | |
Comment 5•16 years ago
|
||
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.
![]() |
Assignee | |
Updated•16 years ago
|
Status: NEW → ASSIGNED
Comment 6•16 years ago
|
||
Thanks for that, maybe Bug 434338 (SeaMonkey only) will then be fixed (I did not look at that bug too closely).
![]() |
Assignee | |
Comment 7•16 years ago
|
||
(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.
![]() |
Assignee | |
Comment 8•16 years ago
|
||
Note: this patch should fix the bugs this blocks except for bug 479406 which I found while fixing this.
![]() |
Assignee | |
Comment 9•16 years ago
|
||
I just verified that this patch does respect disk quotas.
![]() |
Assignee | |
Comment 10•16 years ago
|
||
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)
![]() |
Assignee | |
Comment 11•16 years ago
|
||
Frank, I've verified this gets the correct size based on the optional components that are selected.
![]() |
Assignee | |
Comment 12•16 years ago
|
||
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)
![]() |
Assignee | |
Comment 13•16 years ago
|
||
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)
![]() |
Assignee | |
Comment 14•16 years ago
|
||
See comment #12 for what this fixes.
Attachment #363525 -
Flags: review?(jmathies)
![]() |
||
Comment 15•16 years ago
|
||
+ ; 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.
![]() |
Assignee | |
Comment 16•16 years ago
|
||
(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!
![]() |
||
Updated•16 years ago
|
Attachment #363525 -
Flags: review?(jmathies) → review+
![]() |
Assignee | |
Comment 17•16 years ago
|
||
Will check in today
Attachment #363525 -
Attachment is obsolete: true
Attachment #363947 -
Flags: review+
![]() |
Assignee | |
Comment 18•16 years ago
|
||
Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/de28e36225a6
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
![]() |
Assignee | |
Comment 19•16 years ago
|
||
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?
Updated•16 years ago
|
Attachment #363947 -
Flags: approval1.9.1? → approval1.9.1+
Comment 20•16 years ago
|
||
Comment on attachment 363947 [details] [diff] [review]
patch - checked in
a191=beltzner
![]() |
Assignee | |
Comment 21•16 years ago
|
||
Pushed to mozilla-1.9.1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/90666837bc86
Keywords: fixed1.9.1
![]() |
Assignee | |
Updated•16 years ago
|
Flags: wanted1.9.1?
Updated•2 years ago
|
Component: NSIS Installer → Installer
Product: Toolkit → Firefox
You need to log in
before you can comment on or make changes to this bug.
Description
•