237754 - reintroduce getRegistryEntry

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Henrik Gemal]

My extension Launchy:
http://gemal.dk/mozilla/launchy.html
which enables users to open webpages and mailto with external applications is
using the getRegistryEntry function.

This function used to be part of winhooks. It seems that winhooks has been
completely killed with Mozilla Firefox. This has also killed off Launchy on
Mozilla Firefox.

I desperatly need getRegistryEntry on Mozilla Firefox for Launchy to work.

Is there a chance that getRegistryEntry could be reintroduced?

all the code is already there since I dont need any new functionality. I just
need the function to be accessable from Mozilla Firefox. Perhaps from the
@mozilla.org/browser/shell-service;1 object?

For my extension this is a complete blocker....:(

Comment 1

[Henrik Gemal]

Attachment: first cut

one additional file is needed. nsWindowsShellServiceUtil.cpp

Comment 2

[Henrik Gemal]

Attachment: new file

Comment 3

[Henrik Gemal]

I've tested it in my own compiled build of Mozilla Firefox and it works. Need
feedback

Comment 4

[Henrik Gemal]

Comment on attachment 145534 [details] [diff] [review]
first cut

Ben: could you look at the patch. There's also a new file called
nsWindowsShellServiceUtil.cpp attached to the bug. That's needed to compile.

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

Hi Henrik, I'll look at this later tonight. I have some concerns about API that
I'd like to think about. 

Comment 6

[HJ]

I wonder why this was removed. Btw, is there a bug # for this removal?

Comment 7

[Henrik Gemal]

(In reply to comment #6)
> I wonder why this was removed. Btw, is there a bug # for this removal?

winhooks got removed in bug 236744
I then filed bug 237617 about the missing getRegistryEntry

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 145534 [details] [diff] [review]
first cut

There's no need for a new file, nor a need to add back any of the old
excessively complex code. No structs, no classes, no byzantine object
structure, just some good ol' fashioned Windows C code. 

Simply add a method to nsIWindowsShellService called getRegistryEntry that
takes a string key name and a value name, so you can call:

var val = wss.getRegistryEntry("HKEY_CURRENT_USER\\blah\\blah\\blah\\", "val");

Comment 9

[Henrik Gemal]

Hmm... I'm no C++ coder so I could really really need some help here.

Ben, do you have time to look at it?

Can anybody with interest in this bug, help me?

Comment 10

[(not reading, please use [e-mail] instead)]

gemal, what about the existing:

nsIWindowsRegistry getRegistryEntry()?

see
http://lxr.mozilla.org/seamonkey/source/xpfe/components/winhooks/nsIWindowsHooks.idl#231

Comment 11

[Jon Henry]

Attachment: Proposed patch

I miss Launchy, so I figured I'd give this a shot. This patch is basically a
knockoff of the code Seth pointed out, simplified a little. It compiles and
seems to put Launchy back in working order (at least it does on my WinXP
system). It keeps the API the same as it used to be, rather than changing it to
resemble what Ben mentioned, since I don't see why extensions should have to
special-case Firefox in their code.

Comment 12

[Jon Henry]

Comment on attachment 146203 [details] [diff] [review]
Proposed patch

How does this look, Ben?

Comment 13

[David Hallowell]

Comment on attachment 146203 [details] [diff] [review]
Proposed patch

You beat me to this, I was just about to give this a shot! You've taken the
same approach as I would as it seems the simplest way to implement things to
me.

>+
>+  LONG rv = ::RegOpenKey( baseKey, aSubKeyName, &key );

RegOpenKey is depreciated, according to MSDN it's for compatibility with 16 bit
apps only. Use RegOpenKeyEx instead:

LONG rv = ::RegOpenKeyEx( baseKey, aSubKeyName, 0, KEY_READ, &key );
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/sysinfo/base/r
egopenkeyex.asp

Comment 14

[David Hallowell]

Attachment: Patch with RegOpenKeyEx

As I nearly finished here's my patch anyway, basically similar to the earlier
one, but with RegOpenKeyEx rather than RegOpenKey (see my previous comment).

If this gets checked in credit should goto Jon Henry as he did post his patch
first which is almost identical bar the call to RegOpenKey.

The original winhooks code uses RegOpenKey too, perhaps I should file a bug for
seamonkey to replace it with RegOpenKeyEx there as well?

Dave

Comment 15

[Cristian Klein]

I would have a question. Does getRegistryEntry compromise security? I mean, can
this function be accessed via Javascript from a web page?

Comment 16

[David Hallowell]

Attachment: Forgot to declare 'rv'! (supercedes my previous patch)

OK, forgot to declare one of the variables I was using! Here's the fixed
version.

On a bit of a tangent but vaguely related I've had a look and bug 216388 is the
bug that describes the issue of using RegOpenKey in Seamonkey

Comment 17

[David Hallowell]

(In reply to comment #15)
> I would have a question. Does getRegistryEntry compromise security? I mean, can
> this function be accessed via Javascript from a web page?

By default chrome apps (anything accessible via chrome:// url's - e.g.
extensions and the app itself) have a higher level of access than normal web
pages or locally saved HTML files. getRegistryEntry is not exposed to
non-prililedged code.

http://www.mozilla.org/projects/security/components/reviewguide.html

Comment 18

[(not reading, please use [e-mail] instead)]

why not use the existing, scriptable, nsIWindowsRegistry interface?

I think you can get the winhooks service and QI to it.  (for launchy, you could
do that in js)

If the existing code in nsWindowsHooks::GetRegistryEntry() isn't right, we
should just fix it.

Comment 19

[David Hallowell]

(In reply to comment #18)
> why not use the existing, scriptable, nsIWindowsRegistry interface?
The problem is that in Firefox winhooks has been replaced with the shell service
which is designed to be a cross platform equivalent of winhooks.

> I think you can get the winhooks service and QI to it.  (for launchy, you could
> do that in js)

That's what he used to do
> 
> If the existing code in nsWindowsHooks::GetRegistryEntry() isn't right, we
> should just fix it.
I guess a solution that'd fit the needs of both seamonkey and firefox would be a
good idea

Comment 20

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 146227 [details] [diff] [review]
Forgot to declare 'rv'! (supercedes my previous patch)

Fix the indentation:

>+NS_IMETHODIMP 
>+nsWindowsShellService::GetRegistryEntry( PRInt32 aHKEYConstant, const char *aSubKeyName, const char *aValueName, char **aResult ) {
>+  
>+  HKEY hKey, fullKey;
>+
>+  *aResult = 0;
>+  // Calculate HKEY_* base key
>+  switch (aHKEYConstant) {
>+  case HKCR:
>+    hKey = HKEY_CLASSES_ROOT;
>+    break;
>+  case HKCC:
>+    hKey = HKEY_CURRENT_CONFIG;
>+    break;
etc (line cases up with switch, only 2 spaces for each sub block)

and r=[e-mail]

Comment 21

[David Hallowell]

Attachment: Patch addressing Ben's comments

Fixed the indentation as per Ben's comments

Comment 22

[David Hallowell]

Comment on attachment 146338 [details] [diff] [review]
Patch addressing Ben's comments

Carrying over the r= from Ben

Comment 23

[Jon Henry]

Changes that affect Firefox only don't need super-review.

Comment 24

[Henrik Gemal]

Ben: could you check the patch in?

Comment 25

[Henrik Gemal]

the changes in the bug are Firefox only. Could anybody check this in? I really
really like to get it into Firefox 1.0

Comment 26

[Henrik Gemal]

David: where does the NS_INVALID_ARG come from. I seems to have problem with 
that? Did it compile with you?

Comment 27

[Henrik Gemal]

it should be:
NS_ERROR_INVALID_ARG

could you make a new patch?

Comment 28

[Brian Ryner (not reading)]

checked in (I did some formatting cleanup on it, and fixed the INVALID_ARG thing)

Comment 29

[Henrik Gemal]

Bug thanx to David Hallowell!

Launchy will now be able to work with latest builds.

I've filed a new bug to improve the function:
http://bugzilla.mozilla.org/show_bug.cgi?id=241237

Comment 30

[Mike Connor [:mconnor]]

reopening for the moment, will reclose once this lands on the 0.9 branch
(probably tomorrow)

Comment 31

[Mike Connor [:mconnor]]

checked in on the 1.7 branch at 2004-04-25 12:43

Comment 32

[Henrik Gemal]

> checked in on the 1.7 branch

what do you mean? Mozilla 1.x already has getRegistryEntry!!!

Comment 33

[David Hallowell]

I think they mean Firefox builds that are built off the 1.7 branch rather than
the Mozilla trunk. AFAIK 0.9 and 1.0 will be built off this stable branch.