265859 - Command-line extension installation requires XServer running

Status: RESOLVED WONTFIX

(Toolkit :: Add-ons Manager, enhancement)

Description

[Alexey Gladkov]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; rv:1.7.3) Gecko/20041021 Firefox/0.10.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; rv:1.7.3) Gecko/20041021 Firefox/0.10.1

I get error:
(firefox-bin:2426): Gtk-WARNING **: cannot open display:

if try install extension using commandline interface. 
It is happens because function gtk_init() are called before Extension Manager
arguments was parsed.

Reproducible: Always
Steps to Reproduce:
1. stop Xserver or switch to console.
2. try install extension:
$firefox --install-global-extension /zzz/foobar.xpi

Actual Results:  
Nothing happens. 

Expected Results:  
extension installion

Comment 1

[Alexey Gladkov]

Attachment: firefox-1.0-alt-nox.patch

This patch fix problem and add new option "-nox" to able install/uninstall
extensions and themes without Xserver running.

Comment 2

[Nickolay_Ponomarev]

you should do a "cvs diff" to make sure your patch is appliable to the most
recent source tree. You should also set a "review?" flag to get the patch
reviewed and checked in.

See also http://www.mozilla.org/hacking/life-cycle.html

->enh

Comment 3

[Alexey Gladkov]

Yes, You are right. I will update patch as soon as possible.

Comment 4

[Alexey Gladkov]

Attachment: firefox-CVS-nox.patch

I adapted patch to cvs (20041030). Some additional changes was added:
* get filename from URI for argument of "--install-global-theme" option.

Comment 5

[Nickolay_Ponomarev]

if the patch is ready for review you should set r? flag and select a person to
review it.

Comment 6

[Wolfgang Rosenauer [:wolfiR]]

The patch works for me.
But one related thing (which is not caused by this bug) would be of interest for
me. If I've installed a theme via install-global-theme and I do a firefox
-register afterwards the theme is not longer available in chrome.rdf.
Any solution for this?

Comment 7

[Alexey Gladkov]

Attachment: firefox-CVS-20050121-nox.patch

I merge this patch with current cvs (20050121) and add the option
"unstall-global-theme".

Comment 8

[Alexey Gladkov]

(In reply to comment #6)
> If I've installed a theme via install-global-theme and I do a firefox
> -register afterwards the theme is not longer available in chrome.rdf.
> Any solution for this?

Sorry, I don't understand what problem is. 
Can You explain this problem more narrowly ?

Comment 9

[Wolfgang Rosenauer [:wolfiR]]

the problem is that I want to add a theme directly after build stage before
packaging the files into an RPM.
But at installation time of the RPM I have to call firefox --register to get
the components registered properly. This will wipe out the entries for the
global-theme.

Comment 10

[Frederic Crozat]

As a packager, I confirmed this patch is really needed when packaging
extensions/themes. 

Comment 11

[Alexey Gladkov]

(In reply to comment #9)
> But at installation time of the RPM I have to call firefox --register to get
> the components registered properly. This will wipe out the entries for the
> global-theme.

Hm. I never get this problem. Can You tell me for which linux vendor do You
build RPM ?

Comment 12

[timeless]

*** Bug 247610 has been marked as a duplicate of this bug. ***

Comment 13

[Alexey Gladkov]

(In reply to comment #11)
> Hm. I never get this problem. Can You tell me for which linux vendor do You
> build RPM ?
>

I had reproduced this bug. I will try to fix it.

Comment 14

[alain.forcioli]

Hello,

(In reply to comment #7)
> Created an attachment (id=172001) [edit]
> firefox-CVS-20050121-nox.patch
> 
> I merge this patch with current cvs (20050121) and add the option
> "unstall-global-theme".

I can't compile firefox with this patch.

Here is the procedure I used :

0. Get the patch firefox-CVS-20050121-nox.patch
1. Get firefox 1.0 archive (FIREFOX_1_0_RELEASE branch).
2. tar jxf firefox-1.0-source.tar.bz
3. cvs -z3 checkout -PA mozilla/client.mk
4. make -f mozilla/client.mk checkout MOZ_CO_FLAGS=-PA MOZ_CO_PROJECT=browser
5. patch -p0 < firefox-CVS-20050121-nox.patch
6. Write mozilla/mozconfig.
7. cd mozilla; gmake -f client.mk build

Compilation exit with error related to em->HandleCommandLineArgs() in the
nsAppRunner.cpp file because the function take no arguments.

I have deleted the nohup.out file containing the error message (arg!) but could
you confirm the procedure I use is OK ?

Comment 15

[Alexey Gladkov]

(In reply to comment #14)
> I have deleted the nohup.out file containing the error message (arg!) but could
> you confirm the procedure I use is OK ?

Yes. Please wait a while. I plan to update patch tomorrow. 

Comment 16

[Alexey Gladkov]

Attachment: firefox-CVS-20050203-nox.patch

In this patch I tried to remove the ugly code duplication. But when I do it, I
encountered a problem with firefox CVS version and I can not resolve it. The
problem is firefox is far from ready to work at command-line mode. Patch to fix
it must be much more complex. At this moment firefox started with any
command-line arguments (except "help" and "version") require running XServer.
Even the "register" option requires it. I can not fix this problem and test
this patch :( .
Who can help me with it?

Comment 17

[Alexey Gladkov]

(In reply to comment #6)
> The patch works for me.
> But one related thing (which is not caused by this bug) would be of interest for
> me. If I've installed a theme via install-global-theme and I do a firefox
> -register afterwards the theme is not longer available in chrome.rdf.
> Any solution for this?

I think I know what happens. I reproduce this with some extensions too. I
executed "regchrome" after installation. This program regenerate chrome.rdf and
broke metadata of themes and extensions. After that themes(and extensions) is
present at the themes list (Tools->Themes) but no one can not be used.
You must reinstall broken extensions and themes to fix situation. 
At the firefox-CVS-20050203-nox.patch I add the new option "update-register"
instead of "register". By this option firefox rebuild metadata to all global
themes and extensions. If You want I can give You this patch for firefox 1.0 .

Comment 18

[Alexey Gladkov]

Sorry for my long silence. I have understood that I go on a wrong way when I
create a new firefox patch.
In the first patches I tried to isolate a code working with GUI. But it
contradicts the basic purpose of the program.
I have reconsidered the approach to a problem. For this purpose it is necessary
to formulate this problem once again.

Problem.
In some case it is necessary to restrict program to work with the graphic
interface. But we must not constrict 
program functionality.

Solution.
Code responsible for work with the user interface are in directories gfx and
widget. These libraries are made 
as xpcom components. The reference to these components occurs on the conditional
identifier (ContractID)

Example: "nsCOMPtr<nsICmdLineService> cmdLine =
do_GetService("@mozilla.org/appshell/commandLineService;1",&rv);"

For the decision of a problem we need to create new xpcom components to work
with the user interface. 
They do only visibility of work. Usually these components are not necessary to
the program, 
but in a command-line mode this components is very useful. Also we need to
create the mechanism for loading 
and substitutions of one xpcom component by another.

I have tried to realize this idea and I have made the new patch.

Comment 19

[Alexey Gladkov]

Attachment: firefox-1.0.2-nox.patch

Comment 20

[Nickolay_Ponomarev]

The extension installation system is being reworked in bug 286034, installing an
extension is going to be as easy as writing a text file.

Comment 21

[Alexey Gladkov]

(In reply to comment #20)
> The extension installation system is being reworked in bug 286034, installing an
> extension is going to be as easy as writing a text file.

This is very good! :)
But I think it will not strongly change this patch.

Comment 22

[timeless]

Comment on attachment 180800 [details] [diff] [review]
firefox-1.0.2-nox.patch

1. don't use tabs
2. interfaces should be interCaps, not InitialCaps (ReplaceContractID)
3. I don't think you should need to have replaceContractID at all, i think
something we have should already let you do that
4. what are the rules for g_init?
5. for new files, if they're really new, please use MPL/LGPL/GPL 1.1 see
mozilla.org/MPL
(not NPL), ofr new files you should be the initial developer :)
6. for alloc failures
+	mImage = mm->Alloc(mLockWidth * mLockHeight);
+	if (!mImage) {
+  	  mLocked = PR_FALSE;
+  	  return NS_ERROR_FAILURE;
6/7 (please make sure you check all allocs for failure)
+    mNullFont = new nsNullFont(aFont, aLangGroup, aContext);

please return NS_ERROR_OUT_OF_MEMORY;
7. most xpcom calls can fail
+            prefs->GetBranch(name.get(), getter_AddRefs(branch));
+            branch->GetIntPref(langGroup, &minimum);
you need to make sure you got a branch before you use it.
8. use initializers instead of assignment in constructors
+nsNullFontMetrics::nsNullFontMetrics()
+{
+	mNullFont = nsnull;
9. this code doesn't make any sense:
+    nsIAtom *langGroup = NS_NewAtom(aLangGroup);
+    NS_IF_RELEASE(langGroup);
10. i think you're better off saying you don't support flavors.
+nsNullDragService::IsDataFlavorSupported(const char *aDataFlavor, PRBool *_retval)
+{
+  /* XXX Please implement this - for now - support all flavors */
+  *_retval = PR_TRUE;
11. using stl is not generally done in our world (<list>)
I don't think there's any reason to use it, since you should be able to just
loop over GetNext. Can you explain why you felt the need to use it?
12.
+		rv |= xpcom.RegisterNullWidget();
this should really just be rv =.
you already have an OK value, but it could be 0x1111, and bitmasking it with
another rv is a bad idea. (Also, not checking the rv from that isn't a verify
good idea either).

Comment 23

[Alexey Gladkov]

(In reply to comment #22)
> 1. don't use tabs

This patch don't finished yet. This is first try. So I did not care about coding
style. But You are right. I will fix it.

> 2. interfaces should be interCaps, not InitialCaps (ReplaceContractID)

Excuse, I have not understood.

> 3. I don't think you should need to have replaceContractID at all, i think
> something we have should already let you do that

Unfortunately it is necessary. In existen Component Manager API always runs
constructor of Factory class. I would not like to change frozen API, 
but I can't find any other solutions. 

> 4. what are the rules for g_init?

We must call g_type_init then we want to use gconf without gtk.
http://developer.gnome.org/doc/API/2.0/gconf/gconf-GConfClient.html#gconf-client-get-default

> 5. for new files, if they're really new, please use MPL/LGPL/GPL 1.1 see
> mozilla.org/MPL
> (not NPL), ofr new files you should be the initial developer :)

I copied all license blocks from the original files. 
I don't think that I have not permission to remove license block from not mine
sources.

> 6. for alloc failures
> 7. most xpcom calls can fail
> 8. use initializers instead of assignment in constructors

Thanks, I shall correct it.

> 9. this code doesn't make any sense:
> +    nsIAtom *langGroup = NS_NewAtom(aLangGroup);
> +    NS_IF_RELEASE(langGroup);

Maybe. But I really don't know what language we must return in Null Font :)

> 10. i think you're better off saying you don't support flavors.

Maybe You right. :)

> 11. using stl is not generally done in our world (<list>)
> I don't think there's any reason to use it, since you should be able to just
> loop over GetNext. Can you explain why you felt the need to use it?

I don't understude how use mozilla Iterators. 
If you will help me to correct it I shall be very grateful to you :)

> 12.
> +		rv |= xpcom.RegisterNullWidget();
> this should really just be rv =.
> you already have an OK value, but it could be 0x1111, and bitmasking it with
> another rv is a bad idea. (Also, not checking the rv from that isn't a verify
> good idea either).

Agree.

Comment 24

[Benjamin Smedberg]

This patch will be unnecessary when bug 286034 lands. Please don't spend any
more time on it until the new EM features are committed ;-)

Comment 25

[Nickolay_Ponomarev]

As you noticed, the patch in that bug has landed. You can now install extensions
by unzipping XPIs to the install location (firefox/extensions/{extension-guid}).
They are registered on next startup automatically. Does that suit your needs?

The support for -install-global-extension is not removed, as far as I can see
though. And /that/ probably still requires X Server running.

Comment 26

[Alexey Gladkov]

(In reply to comment #25)
> The support for -install-global-extension is not removed, as far as I can see
> though. And /that/ probably still requires X Server running.

I almost sure about that :(. 
I will test new EM and then I will tell definitely.

Comment 27

[Frederic Crozat]

So, it won't work for packagers like me were firefox is not started by user
which own write permissions for files.

Comment 28

[Alexey Gladkov]

(In reply to comment #27)
> So, it won't work for packagers like me were firefox is not started by user
> which own write permissions for files.
 
I have not understood :(. Please describe the problem in more details.

Comment 29

[Frederic Crozat]

This is quite simple :

Firefox (or any other programs) should not depend on user to start the program
in an X11 environment to get its registry in a consistent state.

Moreover, when packaging Firefox (or extensions), user running firefox will not
have write permissions to firefox installation directory (since it will be owned
by root) but only to its profile directory. Auto-registration on startup will
failed, unless it is done in the user profile directory and not in the
installation directory.

Currently, to register extensions/themes from a package, I need this "nox"
package so I can call firefox registration code from package scripts (where X11
environment can't be assumed to be present).

Comment 30

[Benjamin Smedberg]

With the new EM patch, you do not need to run anything to get global
extensions/themes to work. Just unzip the extension files into the
appdir/extensions/{GUID} directory, and it will work without any registration
needed.

Comment 31

[Frederic Crozat]

But will it work if user don't have write permissions (ie fallback to profile
directory) ?

Currently (1.0.x) , extension manager is completely broken in this aspect and it
is only thanks to Debian dudes who fixed it that it is somehow usable by packagers..

hmm, according to
http://www.mozilla.org/projects/firefox/extensions/em-changes.html, it should
work, so I retract my previous comments.

Comment 32

[Alexey Gladkov]

(In reply to comment #25) 
> And /that/ probably still requires X Server running. 
 
New extension manager looks very good. I found the following problem:  
1) I've installed a new extention; 
2) I've started firefox with no permission to write at firefox installation
directory;
3) Then I got the error: Browser could not install this item because of a
failure in Chrome Registration. 
 
In other words, You must have permission to write at firefox installation
directory or you should run firefox without X as user which installed the
extension. 
 
So, problem is present. 

Comment 33

[Benjamin Smedberg]

You must use the new chrome.manifest and not contents.rdf and <em:file> if you
want extensions to be installable in the app directory.

Comment 34

[Benjamin Smedberg]

WONTFIX, with the new drop-install of extensions and themes.