738501 - Implement ability to create Windows shortcuts from javascript

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v1 - Initial implementation of shortcut creation js module

As part of the installation process for natively-installed webapps, we need to be able to create Windows shortcuts (on the desktop and in the start menu).  Windows only exposes shortcut creation/modification through the COM interface `IShellLink`.  Using js-ctypes, we can create an instance of `IShellLink` to create/modify a shortcut, QueryInterface it to a `IPersistFile`, and save the icon to disk.  This bug tracks progress toward a js-ctypes implementation of shortcut creation/modification.

Comment 1

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 608538 [details] [diff] [review]
Patch v1 - Initial implementation of shortcut creation js module

bholley, you were identified here ( https://bugzilla.mozilla.org/show_bug.cgi?id=725408#c20 ) as someone who could review this code.  Would you be able to take a look?

Comment 2

[Bobby Holley (:bholley)]

I know close to nothing about windows platform stuff, so all I can really do here is review the js-ctypes usage.

But I'm sort of surprised that this works. JS-Ctypes doesn't support c++ calling conventions and linkage. Do Win32 API c++ objects set up their methods with c-style stdcall conventions? At the very least, this would preclude overloading. But I'm no expert in these things.

This begs the question though - why is this being done in js-ctypes? It would seem infinitely easier to do in C++. The js-ctypes approach is much more brittle - is there a good reason for it?

Comment 3

[Dão Gottwald [:dao]]

What does "webapprt" mean?

Comment 4

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Bobby Holley (:bholley) from comment #2)
> I know close to nothing about windows platform stuff, so all I can really do
> here is review the js-ctypes usage.

I'll ping someone in #windev to take a look at the windows platform API usage.

> But I'm sort of surprised that this works. JS-Ctypes doesn't support c++
> calling conventions and linkage. Do Win32 API c++ objects set up their
> methods with c-style stdcall conventions? At the very least, this would
> preclude overloading. But I'm no expert in these things.

IShellLinkW and IPersistFile are COM interfaces.  The objects that implement them have a specific layout in memory and calling convention.  The memory layout happens to be friendly to C++ (by design - https://blogs.msdn.com/b/oldnewthing/archive/2004/02/05/68017.aspx ) but the calling convention of all the functions is stdcall.  Any language that can understand stdcall, make system calls (CoCreateInstance, etc), and understand the memory layout of COM objects is capable of using COM interfaces/objects (e.g. C, C++, python, VB, Delphi, js-ctypes).

> This begs the question though - why is this being done in js-ctypes? It
> would seem infinitely easier to do in C++. The js-ctypes approach is much
> more brittle - is there a good reason for it?

The consuming code for this functionality is in javascript.  The two approaches that I can think of for writing this piece in C++ are 1) write an XPCOM component that is consumed from our js code or 2) compile the C++ code into a DLL that we subsequently access from js-ctypes.  My understanding is that we're moving away from XPCOM components (preferring js modules when possible), so option 1) is not preferred.  Given that the memory layout and calling convention of COM are well-defined and that COM interfaces (at least the ones provided by MSFT) are guaranteed not to change, is option 2) less brittle than the pure-js-ctypes approach?

This is being done in js-ctypes at least partly because I wanted to prove that it could be done; there's a rich array of functionality in Windows that is only available through COM (shortcut creation is one example, invoking a virus scan on a specified file is another).  Thanks to js-ctypes and its ability to understand COM objects, that functionality is available to us in javascript (if we do a little work up front parsing the C interfaces for the COM objects we want to use).  It seems silly to write a larger piece of functionality in C++ just because it wants to create a Desktop shortcut, and it seems equally silly to wrap MSFT COM in XPCOM if we have a more direct alternative.  That being said, you're certainly the js-ctypes expert here so I'm not comfortable with this approach unless you are :)


(In reply to Dão Gottwald [:dao] from comment #3)
> What does "webapprt" mean?

"webapprt" stands for "Webapp Runtime" - The "webapprt" directory is being created (maybe under a different name, depending on what reviewers have to say) by the patches in bug 725408.

I don't think the "webapprt" directory is where the functionality described in this bug should live, but that's where I happen to have placed it during development (since I don't know where in the tree it should eventually go).

Comment 5

[Bobby Holley (:bholley)]

(In reply to Tim Abraldes from comment #4)

> IShellLinkW and IPersistFile are COM interfaces.  The objects that implement
> them have a specific layout in memory and calling convention.  The memory
> layout happens to be friendly to C++ (by design -
> https://blogs.msdn.com/b/oldnewthing/archive/2004/02/05/68017.aspx ) but the
> calling convention of all the functions is stdcall.  Any language that can
> understand stdcall, make system calls (CoCreateInstance, etc), and
> understand the memory layout of COM objects is capable of using COM
> interfaces/objects (e.g. C, C++, python, VB, Delphi, js-ctypes).

Oh, interesting. Thanks for explaining. ;-)

> The consuming code for this functionality is in javascript.  The two
> approaches that I can think of for writing this piece in C++ are 1) write an
> XPCOM component that is consumed from our js code or 2) compile the C++ code
> into a DLL that we subsequently access from js-ctypes.  My understanding is
> that we're moving away from XPCOM components (preferring js modules when
> possible), so option 1) is not preferred. 

That's not really the whole picture.

We're moving away from _extensions_ using XPCOM binary components, largely because we want them to only touch gecko internal interfaces from JS (so that they don't break every time we change the vtables). Historically, people have used binary components when they want to do native stuff, and then poked around at nsINode from C++ while they were at it.

To guard against this, we've been bumping kVersion every 6 weeks with each release, so that extension have to recompile every 6 weeks (which sucks). So we've been encouraging addons that just have to do a little bit of native stuff here and there to use js-ctypes to do it.

However, we've been specifically suggesting that people not expose use big complicated APIs directly with js-ctypes. Instead, we've been suggesting that people write a simple C shim around whatever native APIs they want to expose, so that they don't have to duplicate complicated interface declarations (more or less like what this patch does).

More to the point, none of this applies to code that lives in mozilla central (which is where I assume this code will live). That code gets rebuilt on every checkin (and is available to developers on mxr), so there's no worry of code getting out of sync when we change other things in gecko. I have not heard of anyone saying that we should be replacing binary XPCOM platform code with js-ctypes, and think it's a bad idea. We should use XPCOM/XPIDL/XPConnect for this kind of stuff.


> This is being done in js-ctypes at least partly because I wanted to prove
> that it could be done;

I agree that it's very cool! :-)

> It seems silly to write a larger piece of
> functionality in C++ just because it wants to create a Desktop shortcut

Agreed.

> and it seems equally silly to wrap MSFT COM in XPCOM if we have a more direct
> alternative.

I disagree with you here. The 'more direct alternative' involves duplicating the interface definitions from the Win32 headers into javascript. This is incredibly error-prone (which is what I really meant before when I said 'brittle'). And not really what js-ctypes is designed to do.

> That being said, you're certainly the js-ctypes expert here so
> I'm not comfortable with this approach unless you are :)

I think that this functionality should be exposed via an XPCOM service to avoid duplicating the declarations from the headers and scary vtable munging.

Comment 6

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 608538 [details] [diff] [review]
Patch v1 - Initial implementation of shortcut creation js module

Canceling review per discussion.  I'll get started on an XPCOM service implementation for "Windows shortcut creation"

Comment 7

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: XPCOM Service Patch v1 - WIP, working implementation

Just wanted to post the WIP patch.  I've been able to successfully create shortcuts from JS while testing this patch.

Some remaining code issues:
  CoInitialize/CoUninitialize should be called before using COM and after we're done using COM, respectively
  It doesn't make sense to create shortcuts with no target, so if the shortcut file doesn't exist then `setShortcut` should check whether it was passed a target file and error out if it was not

Some remaining organizational issues:
  I put the shortcut service in xpcom/io; not sure if there are strong opinions about better locations for it
  "@mozilla.org/file/windows_shortcut_service;1" is the current contract ID, but I'm open to better ones

Comment 8

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v2 - Reviewable patch

This patch implements Windows shortcut creation as an XPCOM service in xpcom/io with the contract ID "@mozilla.org/file/windows_shortcut_service".

You'll notice that the build system changes also include changes for the Windows Shortcut Creation service in bug 738500.  That is because these features are being developed together and will likely be submitted together as part of the patch in bug 725408.

Benjamin; same questions as here - [https://bugzilla.mozilla.org/show_bug.cgi?id=738500#c3].  Namely, are you the right person to review this code? and have I placed this code in the right place in the tree?

Also, the build system changes in this patch include changes for the Windows icon embedding service in bug 738500.  That is because these features are being developed together and will likely be submitted together as part of the patch in bug 725408.

Comment 9

[Benjamin Smedberg]

Comment on attachment 614047 [details] [diff] [review]
Patch v2 - Reviewable patch

It seems to me that it would be a simpler API to just have nsILocalFileWin.setShortcut, and avoid the new service and its associated gunk. Is there a reason we can't just do that? re-request review on this patch if necessary, but I'd like that approach better.

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Tim: the destructor causes an infinite recursion when trying to delete the object of the same type
(although this point might be irrelevant if you change to the nsILocalFileWin approach)

Comment 11

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v1 - First iteration of adding SetShortcut function to nsILocalFileWin

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)
> [...]
> 
> It seems to me that it would be a simpler API to just have
> nsILocalFileWin.setShortcut [...]

Done.  Benjamin: I believe you're still the right person from whom to request review?

Comment 12

[Benjamin Smedberg]

Comment on attachment 614891 [details] [diff] [review]
Patch v1 - First iteration of adding SetShortcut function to nsILocalFileWin

>diff --git a/xpcom/io/nsLocalFileWin.cpp b/xpcom/io/nsLocalFileWin.cpp

>+    nsresult SetShortcut(bool updateExisting,
>+                         const WCHAR* shortcutPath = NULL,
>+                         const WCHAR* targetPath = NULL,
>+                         const WCHAR* workingDir = NULL,
>+                         const WCHAR* args = NULL,
>+                         const WCHAR* description = NULL,
>+                         const WCHAR* iconFile = NULL,
>+                         PRInt32 iconIndex = 0);

You don't use the default args and I don't think they would be correct. Please remove them.

>+nsresult
>+ShortcutResolver::SetShortcut(bool updateExisting,
>+                              const WCHAR* shortcutPath,
>+                              const WCHAR* targetPath,
>+                              const WCHAR* workingDir,
>+                              const WCHAR* args,
>+                              const WCHAR* description,
>+                              const WCHAR* iconPath,
>+                              PRInt32 iconIndex) {

nit, brace on column 0


>+    if(FAILED(mPersistFile->Save(shortcutPath,
>+                                 TRUE))) { // Indicates that the file
>+                                           // path specified in
>+                                           // the first argument should
>+                                           // become the
>+                                           // "current working file" for
>+                                           // this IPersistFile

nits, unsnuggle the "if(" and move the comment to column 4 instead of after the brace

>+nsLocalFile::SetShortcut(nsILocalFile* targetFile,
...
>+{
>+    bool exists;
>+    nsresult rv = this->Exists(&exists);
>+    NS_ENSURE_SUCCESS(rv, rv);

Please remove all uses of NS_ENSURE_* from this patch. If it's important to issue a warning, please use

  if (NS_FAILED(rv)) {
    NS_WARNING("Exists failed!");
    return rv;
  }

Otherwise just use if (NS_FAILED(rv)) return rv;

>+    nsAutoString targetPath;
>+    if(targetFile) {

nit, unsnuggle

what if (!targetFile)? That seems strange.

>+    nsAutoString workingDirPath;
>+    if(workingDir) {
>+        rv = workingDir->GetPath(workingDirPath);
>+        NS_ENSURE_SUCCESS(rv, rv);
>+    }

What if !workingDirPath? Wouldn't we end up calling SetShortcut with ""? What would that do in the Windows API? I think this needs to be documented in the IDL (are the args optional or not?) and then the tests (which don't seem to be present!) need to test for the null case.

This needs tests.

Comment 13

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v2 - Adds tests, documents behavior in IDL, fixes nits.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #12)
> Comment on attachment 614891 [details] [diff] [review]
> Patch v1 - First iteration of adding SetShortcut function to nsILocalFileWin
> 
> >diff --git a/xpcom/io/nsLocalFileWin.cpp b/xpcom/io/nsLocalFileWin.cpp
> 
> >+    nsresult SetShortcut(bool updateExisting,
> >+                         const WCHAR* shortcutPath = NULL,
> >+                         const WCHAR* targetPath = NULL,
> >+                         const WCHAR* workingDir = NULL,
> >+                         const WCHAR* args = NULL,
> >+                         const WCHAR* description = NULL,
> >+                         const WCHAR* iconFile = NULL,
> >+                         PRInt32 iconIndex = 0);
> 
> You don't use the default args and I don't think they would be correct.
> Please remove them.

Removed.

> >+nsresult
> >+ShortcutResolver::SetShortcut(bool updateExisting,
> >+                              const WCHAR* shortcutPath,
> >+                              const WCHAR* targetPath,
> >+                              const WCHAR* workingDir,
> >+                              const WCHAR* args,
> >+                              const WCHAR* description,
> >+                              const WCHAR* iconPath,
> >+                              PRInt32 iconIndex) {
> 
> nit, brace on column 0

Done.

> >+    if(FAILED(mPersistFile->Save(shortcutPath,
> >+                                 TRUE))) { // Indicates that the file
> >+                                           // path specified in
> >+                                           // the first argument should
> >+                                           // become the
> >+                                           // "current working file" for
> >+                                           // this IPersistFile
> 
> nits, unsnuggle the "if(" and move the comment to column 4 instead of after
> the brace

Done and done.

> >+nsLocalFile::SetShortcut(nsILocalFile* targetFile,
> ...
> >+{
> >+    bool exists;
> >+    nsresult rv = this->Exists(&exists);
> >+    NS_ENSURE_SUCCESS(rv, rv);
> 
> Please remove all uses of NS_ENSURE_* from this patch. If it's important to
> issue a warning, please use
> 
>   if (NS_FAILED(rv)) {
>     NS_WARNING("Exists failed!");
>     return rv;
>   }
> 
> Otherwise just use if (NS_FAILED(rv)) return rv;

I've made this change, but I'm curious why we're not using the NS_ENSURE_* macros here?

> >+    nsAutoString targetPath;
> >+    if(targetFile) {
> 
> nit, unsnuggle

Done.

> what if (!targetFile)? That seems strange.

In the case that we are updating an existing shortcut, targetFile could be NULL because we only want to change other fields (e.g. args, icon).  In the case that we are creating a new shortcut we probably don't want targetFile to be NULL, so I have made this condition throw an error (and documented it in the IDL).  The Windows API does not treat this as an error condition - if you create a shortcut without specifying a target, the shortcut will point to "My Computer" (at least on my Win7 box).

> >+    nsAutoString workingDirPath;
> >+    if(workingDir) {
> >+        rv = workingDir->GetPath(workingDirPath);
> >+        NS_ENSURE_SUCCESS(rv, rv);
> >+    }
> 
> What if !workingDirPath? Wouldn't we end up calling SetShortcut with ""?
> What would that do in the Windows API?

The only required information for creating a shortcut is the path to the shortcut file itself.  Everything else is optional: Calling SetShortcut with no arguments will create a shortcut that points at "My Computer."  Shortcuts with no working directory specified are not a problem.

> I think this needs to be documented
> in the IDL (are the args optional or not?)

Documented in the IDL.

> and then the tests (which don't
> seem to be present!) need to test for the null case.

Added tests for null cases.

> This needs tests.

I added tests for what I believe to be common situations.  Unfortunately there isn't really a way to check the properties of a shortcut file from xpcshell, except the target.  All the tests verify that the target makes sense given what the test is trying to accomplish, but the other properties are mostly not covered by these tests.  I have tested these other properties manually and they have been set/updated correctly in my testing.

Comment 14

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

This tryserver run contains the current patch:
  https://tbpl.mozilla.org/?tree=Try&rev=c05c1812d0bc

Comment 15

[Benjamin Smedberg]

Comment on attachment 615999 [details] [diff] [review]
Patch v2 - Adds tests, documents behavior in IDL, fixes nits.

+    const WCHAR* targetFilePath = NULL;
+    const WCHAR* workingDirPath = NULL;
+    const WCHAR* iconFilePath = NULL;

These technically could just use nsXPIDLString (for which .get() returns NULL if the string is void), but this way is ok also.

Comment 16

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 615999 [details] [diff] [review]
Patch v2 - Adds tests, documents behavior in IDL, fixes nits.

[Approval Request Comment]

This is part of the webapps work which is targeted for Firefox 14.  Without this patch, the webapp installer will be unable to create desktop and start menu shortcuts.

Risk for Fennec: none, the code here affects only nsILocalFileWin/nsLocalFileWin

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 615999 [details] [diff] [review]
Patch v2 - Adds tests, documents behavior in IDL, fixes nits.

Shouldn't you use a manifest annotation to avoid running the test on non-Windows, rather than baking that into the test itself?

Comment 18

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #17)
> Comment on attachment 615999 [details] [diff] [review]
> Patch v2 - Adds tests, documents behavior in IDL, fixes nits.
> 
> Shouldn't you use a manifest annotation to avoid running the test on
> non-Windows, rather than baking that into the test itself?

Almost certainly yes.  I followed the example here [https://mxr.mozilla.org/mozilla-central/source/xpcom/tests/unit/test_hidden_files.js#32] but I suspect that was simply written before manifest annotations were an option.  I can fix this as a follow-up

Comment 19

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 615999 [details] [diff] [review]
Patch v2 - Adds tests, documents behavior in IDL, fixes nits.

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=5c21a9ce79c7

Comment 20

[:Felipe Gomes (needinfo for replies!)]

uuid updated: https://hg.mozilla.org/integration/mozilla-inbound/rev/45ac4e63aee3

Comment 21

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/5c21a9ce79c7

Comment 22

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/45ac4e63aee3

Comment 23

[Jason Smith [:jsmith]]

Verified through a separate bug, as shortcut creation is confirmed to be working on a basic level for Win XP, Vista, 7 32-bit, 7 64-bit.