750554 - If service is not used for updates then do not show it in preferences

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Brian R. Bondy [:bbondy]]

There are some conditions where the service will not be used for updates and a UAC prompt will be displayed instead.  Examples of such cases are: i) Bug 748948, or ii) when the user already has write access to the installation directory.

This bug is to remove the option in preferences to use the service when these cases are detected.

Comment 1

[Brian R. Bondy [:bbondy]]

Marking this as a mentored bug since I will not have time to do this in the near future.  If anyone would like to take it, please feel free to email for help on getting started.

Comment 2

[Josh Matthews [:jdm]]

Link to a source file?

Comment 3

[Brian R. Bondy [:bbondy]]

The fix should be done inside the #ifdef MOZ_MAINTENANCE_SERVICE at:
http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/advanced.js#535

Inside there we should check to see if the filesystem is NTFS. 
The easiest way to check that is probably with js-ctypes with the Win32 API GetVolumeInformationW.  A check for write access can also be made there to see if there is write access into the installation directory.  If the FS is NTFS or if there is write access then hide the option.

Comment 4

[Brian R. Bondy [:bbondy]]

One change is that we no longer need to check if the filesystem is NTFS.  Instead we have to check GetDriveType() for DRIVE_FIXED.

Comment 5

[Pranav Ravichandran [:pranavrc]]

I'm willing to work on this. Assigning to myself if that's not a problem.

Comment 6

[Brian R. Bondy [:bbondy]]

Hi Pranav Ravichandran,

Thanks for your interest in helping with this problem!

> I can't seem to find a Preferences UI which contains the list of update 
> services, so I thought I'd ask you which UI the option must be removed from. 
> Is the 'useService' entity with the label 'Use a background service to install 
> updates' in Firefox->Options->Preferences->Advanced the option to be removed in
> case of DRIVE_FIXED? 

Yes that is correct, but in the case of the update directory not being in a drive that is of type DRIVE_FIXED.

> In that case, can I just use GetDriveTypeW() along with the if condition 
> used in line 549 of advanced.js? Like so:
>
> if (installed !=1 || GetDriveTypeW(rootPath) == DRIVE_FIXED) {
>    document.getElementById("useService").hidden = true;
> }

Yes but the Win32 API is not directly accessible from JavaScript.  To get that to work you need to use js-ctypes.

See here for info on js-ctypes: 
https://developer.mozilla.org/en/js-ctypes/Using_js-ctypes

And here for info on the GetDriveTypeW API:
http://msdn.microsoft.com/en-us/library/windows/desktop/aa364939%28v=vs.85%29.aspx

And here for info on how to get the directory path that should be checked: 
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#516

By the way after you built everything, you can make changes to the files outside of the objdir (so just in /browser), and then recompile objdir/browser.
Then just run objdir/dist/bin/firefox.exe to see the new build with your changes.

Comment 7

[Pranav Ravichandran [:pranavrc]]

Thanks for the reply, Brian! As the parameter 'pathArray' to 'getUpdateDirCreate', should I be passing DIR_UPDATES?

Comment 8

[Brian R. Bondy [:bbondy]]

I need to verify but please proceed that way for now and I'll look into it at the review.

Comment 9

[Pranav Ravichandran [:pranavrc]]

Attachment: Patch v1

Comment 10

[Pranav Ravichandran [:pranavrc]]

I used the URI for advanced.js in Components.utils.import by mistake, instead of nsUpdateService.js. How do I find the resource:// URI for nsUpdateService.js? I've tried searching the codebase if it's been used elsewhere, but it hasn't been.

Comment 11

[Brian R. Bondy [:bbondy]]

Comment on attachment 625430 [details] [diff] [review]
Patch v1

Review of attachment 625430 [details] [diff] [review]:
-----------------------------------------------------------------

Hey good progress so far, thanks!

Here are some notes, feel free to post another intermediate patch and request feedback or review again.

---

You can't use EXPORTED_SYMBOLS in that file because it is an XPCOM component and not a JS Module.
The update code was not meant to be a JS module (https://developer.mozilla.org/en/JavaScript_code_modules)
JS modules usually have the file extension jsm and are defined by the Makefile.in files listed by EXTRA_PP_JS_MODULES.

It is an XPCOM component though so we can either add a new method to nsIApplicationUpdateService in nsIUpdateService.idl, or we can just copy the code in since it's only a couple lines.
If you decide to make a new method in ther interface you need to change this GUID to a new one:

> [scriptable, uuid(b5811144-ed30-4343-aff9-c514034aa19a)]
> interface nsIApplicationUpdateService : nsISupports

You can then get the service using the contract string: @mozilla.org/updates/update-service;1
But if you'd rather, just copying that code is fine. 

---

For the js-ctypes stuff, please change the variable name to kernel32 and also make that lowercase. 

> let kernel32 = ctypes.open("kernel32");

Also when you're done with it you should call the close() method.

> kernel32.close();

You'll also need to setup your defines manually:

> const DRIVE_FIXED = 3;
> const LPCWSTR = ctypes.jschar.ptr;
> const UINT = ctypes.uint32_t;

And then you also need to import the GetDriveTypeW Win32 API:

> let GetDriveType = kernel32.declare("GetDriveTypeW",
>                                     ctypes.default_abi,
>                                     UINT,
>                                     LPCWSTR);


You should put all of this in a try / catch by the way in case the GetDriveType or the library is not found, perhaps on some future version of Windows.

Also currently you are passing the whole path, but this Win32 API requires you to only take the drive path. You can use nsIFile to get the parent path until you reach the root.

Comment 12

[Pranav Ravichandran [:pranavrc]]

Attachment: Proposed patch v2

Apologies for the delay in posting the patch. 

Also, while removing trailing whitespaces in the patch, i found others too, so I removed them.

Comment 13

[Brian R. Bondy [:bbondy]]

Comment on attachment 626705 [details] [diff] [review]
Proposed patch v2

Review of attachment 626705 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the follow up patch, nice work :)

>  while removing trailing whitespaces in the patch, i found others too, so I removed them.
It's usually better to only change what is needed for a patch, but there aren't many changes for this so I'm OK with it.

::: browser/components/preferences/advanced.js
@@ +523,5 @@
> +      var UpdatesDir = Components.classes["@mozilla.org/updates/update-service;1"].
> +	                 getService(Components.interfaces.nsIApplicationUpdateService);
> +      rootPath = UpdatesDir.getUpdatesDir();
> +      while (rootPath.parent != null)
> +      {

nit: Please put this brace on the line above:
while (rootPath.parent != null) {

@@ +526,5 @@
> +      while (rootPath.parent != null)
> +      {
> +        rootPath = rootPath.parent;
> +      }
> +      if (installed != 1 || kernel32.GetDriveType(rootPath) != DRIVE_FIXED) {

Let's leave the:
if (installed != 1) {
  document.getElementById("useService").hidden = true;
}

Before this whole block instead of combining them.

@@ +532,5 @@
> +      }
> +      kernel32.close();
> +    } catch(e) {
> +      if (installed != 1) {
> +        document.getElementById("useService").hidden = true;

You can just ignore if there is an exception and don't hide the preference.

::: toolkit/mozapps/update/nsIUpdateService.idl
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "nsISupports.idl"
> +#include "nsIFile.idl"

Please remove this and instead add interface nsIFile; to the list below.

@@ +425,5 @@
>    readonly attribute boolean canApplyUpdates;
> +
> +  /**
> +   * Get the Active Updates directory
> +   * @return The active updates directory, as a nsIFile object

nit: * @return The active updates directory

@@ +427,5 @@
> +  /**
> +   * Get the Active Updates directory
> +   * @return The active updates directory, as a nsIFile object
> +   */
> +  nsIFile getUpdatesDir();

I'm going to CC rstrong for feedback, I'm not sure if USE_UPDROOT is always defined on Windows.
If it is we could remove the interface change and just do this:

var fileLocator = Components.classes["@mozilla.org/file/directory_service;1"].
                          getService(Components.interfaces.nsIProperties);
var dir = fileLocator.get("UpdRootD", Components.interfaces.nsIFile);

Comment 14

[David Lawrence [:dkl]]

Over the weekend an issue occurred with the BMO database which resulted in duplication of dependencies. The dependency issue may have resulted in "Depends On" and "Blocks" values being removed while updating a bug. This issue should now be resolved, however dependencies may need to be manually restored to some bugs.

This bug had dependencies removed during the failure period and will need verification that the dependency removal(s) were intentional. Please help out by taking a look at this bug and adding anything back that was mistakenly removed.

Comment 15

[Pranav Ravichandran [:pranavrc]]

Attachment: Proposed patch v3

Fixed the mistakes, will wait for feedback.

Comment 16

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

Comment on attachment 626837 [details] [diff] [review]
Proposed patch v3

After a quick skim I am ok with these changes. I kind of prefer exposing getUpdatesDir to script. I've wanted the ability to determine where the updates dir is located when guiding bug reporters to provide info in the past and this will simplify that process.

Thanks!

Comment 17

[Brian R. Bondy [:bbondy]]

Comment on attachment 626837 [details] [diff] [review]
Proposed patch v3

Thanks for the feedback :)
Marking myself for review to look at the new patch.

Comment 18

[Brian R. Bondy [:bbondy]]

Comment on attachment 626837 [details] [diff] [review]
Proposed patch v3

Review of attachment 626837 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work we're getting really close :)

The main thing is that you'll need a getUpdatesDir property and function just after pauseDownload inside toolkit/mozapps/update/nsUpdateService.js
Currently it won't be able to find the function since it's not part of the object.


By the way here's a tip for running chrome javascript code and working with XPCOM components on the fly:
1) Go to this in your browser URL bar:
about:

2) Go to Web Developer -> Scratchpad

3) Type in this:
Components.utils.import('resource://gre/modules/Services.jsm');
this.__proto__ = Services.wm.getMostRecentWindow('navigator:browser');

4) Hit execute, now you can run any code you would normally do such as the code in advanced.js.

5) Example: Type in this in the scratch pad and then hit run:

var UpdatesDir = Components.classes["@mozilla.org/updates/update-service;1"].
                 getService(Components.interfaces.nsIApplicationUpdateService);
rootPath = UpdatesDir.getUpdatesDir();
alert(rootPath.path);

::: browser/components/preferences/advanced.js
@@ +523,5 @@
> +      const UINT = ctypes.uint32_t;
> +      let kernel32 = ctypes.open("kernel32");
> +      let GetDriveType = kernel32.declare("GetDriveTypeW", ctypes.default_abi, UINT, LPCWSTR);
> +      var UpdatesDir = Components.classes["@mozilla.org/updates/update-service;1"].
> +	                 getService(Components.interfaces.nsIApplicationUpdateService);

nits:
- Cc is declared at the top of the file so you can juse us Cc instead of Components.classes
- Ditto Ci instead of Components.interfaces
- Left align getService with Cc (see the other uses of getService in this file for an example)

::: toolkit/mozapps/update/nsIUpdateService.idl
@@ +427,5 @@
> +  /**
> +   * Get the Active Updates directory
> +   * @returns The active updates directory.
> +   */
> +  nsIFile getUpdatesDir();

Please re-order this to be just before the last method in this same interface (which is pauseDownload).

Comment 19

[Pranav Ravichandran [:pranavrc]]

Attachment: Proposed patch v4

Tested for the condition DRIVE_FIXED, and it works. 

@Brian - Thanks for the scratchpad tip! Really helped me smoothen some errors out. Also, Cc and Ci do not seem to be declared in preferences/advanced.js. Either I'm missing something, or I think you might have mistaken it for the Cc and Ci in nsUpdateService.js.

Also, there are a few trailing whitespaces in nsUpdateService.js, just a heads up about it.

Comment 20

[Brian R. Bondy [:bbondy]]

Comment on attachment 627443 [details] [diff] [review]
Proposed patch v4

Review of attachment 627443 [details] [diff] [review]:
-----------------------------------------------------------------

> Also, Cc and Ci do not seem to be declared in preferences/advanced.js.
> Either I'm missing something, or I think you might have mistaken 
> it for the Cc and Ci in nsUpdateService.js.

Ya I was looking at nsUpdateService.js when I wrote that so you can ignore that one.

Just about there, once you do the next update I'll test it out and then push to the try server so it runs through all the tests.
I'll also test out updates with this patch on the elm branch before it hits the Nightly builds.

::: toolkit/mozapps/update/nsUpdateService.js
@@ -545,5 @@
> -function getUpdatesDir() {
> -  // Right now, we only support downloading one patch at a time, so we always
> -  // use the same target directory.
> -  return getUpdateDirCreate([DIR_UPDATES, "0"]);
> -}

Removing this will break a few other places in this file that use it outside of the object. 
Maybe the easiest thing is to have the interface method be getUpdatesDirectory and then in the object use getUpdatesDirectory: getUpdatesDir,
And then update advanced.js to use that instead.

Comment 21

[Pranav Ravichandran [:pranavrc]]

Attachment: Proposed patch v5

Comment 22

[Brian R. Bondy [:bbondy]]

Sorry for the delay I want to test this on Oak but it is busy at the moment from changes that need to go in v15.  Might be a couple more days for this review.  I'd rather this land in v16 anyway.

Comment 23

[Brian R. Bondy [:bbondy]]

Comment on attachment 627483 [details] [diff] [review]
Proposed patch v5

Review of attachment 627483 [details] [diff] [review]:
-----------------------------------------------------------------

Hey this looks good and I think it's ready to land after one minor small change let.  Thanks again for your work on this.

::: toolkit/mozapps/update/nsUpdateService.js
@@ +2109,5 @@
> +  getUpdatesDirectory: function AUS_getUpdatesDir() {
> +    // Right now, we only support downloading one patch at a time, so we always
> +    // use the same target directory.
> +    return getUpdateDirCreate([DIR_UPDATES, "0"]);
> +  },

Here you can just do:
getUpdatesDirectory: getUpdatesDir,

to avoid code duplication

Comment 24

[Pranav Ravichandran [:pranavrc]]

Attachment: Proposed patch v6

Comment 25

[Brian R. Bondy [:bbondy]]

Comment on attachment 628719 [details] [diff] [review]
Proposed patch v6

Review of attachment 628719 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good and I feel confident it will not cause any harm, I'll land this later today as long as the try tests pass fine.

Comment 26

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/mozilla-central/rev/68abc86fde1c

Thanks for the patch Pranav!

Comment 27

[Pranav Ravichandran [:pranavrc]]

Thanks Brian!