312154 - Start script does not take care of running instance

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Mats Larsson]

User-Agent:       Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.8b5) Gecko/20051011 Firefox/1.4.1
Build Identifier: Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.8b5) Gecko/20051011 Firefox/1.4.1

If I start Firefox with command .../<installdir>/firefox it comes up fine. Then
if I issue the same command again nothing happens (no new browser window) and
the following pop-up appears after I've exited Firefox:

"Firefox is already running, but is not responding. To open a new window, you
must first close the existing Firefox process, or restart your system."

Reproducible: Always

Steps to Reproduce:
1. Start Firefox with command .../<installdir>/firefox
2. Wait for browser windoe to appear
3. Issue the sam command again

Actual Results:  
Nothing.

Expected Results:  
Map a new window (or tab).

Comment 1

[Benjamin Smedberg]

xremote should automatically go to the new process: can you think of any obvious
reasons why xremote would not be working?

Comment 2

[Mats Larsson]

(In reply to comment #1)

> xremote should automatically go to the new process: can you think of any
> obvious reasons why xremote would not be working?

The xremote code is gone in the .../installdir/firefox start script. The same goes for SeaMonkey and Thunderbird. Is this a blocker?

Comment 3

[Benjamin Smedberg]

No, like I said remote is done automatically by the binary and the script is no longer involved (tbrd/ffox).

Comment 4

[Mats Larsson]

(In reply to comment #3)
> No, like I said remote is done automatically by the binary and the script is no
> longer involved (tbrd/ffox).

Then it seems broken and btw where did you say that it was handled by the binary? If i debug (set -x) from the command line I get the following output:

% ./firefox http://www.mozilla.org/ 
...
(last line in the ./firefox script)
+ ./run-mozilla.sh ./firefox-bin http://www.mozilla.org/

(last line in the ./run-mozilla.sh script)
+ ./firefox-bin http://www.mozilla.org/ 

and nothing happens for a long long time and then it pop ups:

"Firefox is already running, but is not responding. To open a new window, you
must first close the existing Firefox process, or restart your system."

Comment 5

[Mats Larsson]

If I add the "check for the running instance" code from the Firefox 1.0.7 start script it all works as before. I click an HTTP link in Tbird and Ffox appears (doesn't matter if it's running or not) with the link loaded. Without this code it doesn't work.

This is a blocker in my opinion.

Comment 6

[Mats Larsson]

[e-mail] removed the blocking1.8rc2? flag without any explanation. Why is this not a blocker?

Comment 7

[Leon Sha]

Attachment: Patch

Comment 8

[Leon Sha]

Comment on attachment 206068 [details] [diff] [review]
Patch

The first 4 bytes of data is argc. It should be PRint32. There is a byte order problem. On big endian platform if the original argc is 1, *data here will be 0. Bug 315693 and bug 309427 may be related to this bug.

Comment 9

[Mats Larsson]

(In reply to comment #8)
> Bug 315693 and bug 309427 may be related to this bug.

I strongly believe that. In bug 315693 it was discussed to move the bug to the Core component.

Comment 10

[Benjamin Smedberg]

Does this mean that the X property is endian-specific? We should probably fix that so that xremote works between multiple systems using the same X server. htonl on "argc" would be the right answer, no?

Comment 11

[Leon Sha]

(In reply to comment #10)
> Does this mean that the X property is endian-specific? We should probably fix
I think the answer is no. The convertion between PRInt32 and char will have endian problem not X property. 
> that so that xremote works between multiple systems using the same X server.
> htonl on "argc" would be the right answer, no?
> 
I don't what it means.

Here if we want to know if "argc"(int) is 0, we can not only use the first byte of "argc"("*data") to confirm if "argc" is 0.

Comment 12

[Leon Sha]

Comment on attachment 206068 [details] [diff] [review]
Patch

This patch only work for the same endian situation. If the two systems have different endian, it doesn't work. I am preparing the new patch for it.

Comment 13

[Mats Larsson]

(In reply to comment #12)
> (From update of attachment 206068 [details] [diff] [review] [edit])
> This patch only work for the same endian situation. If the two systems have
> different endian, it doesn't work. I am preparing the new patch for it.

Leon, will this patch make it into Tbird and Seamonkey as well?

Comment 14

[Benjamin Smedberg]

This code is the "toolkit" version of xremote, so it will work in ffox/tbird/xulrunner but doesn't affect seamonkey at all (seamonkey does not do automatic remoting either).

Comment 15

[Leon Sha]

Attachment: patch v2

Comment 16

[Leon Sha]

Comment on attachment 206484 [details] [diff] [review]
patch v2

I didn't use htonl here. "htonl" will do the byte swap on little endian system and do nothing on big endian system. Since most of the systems are little endian, I don't want to change the little endian system too much.

There are many situations here. I'll list all the results here.

Before apply the patch:
1. Both Xremote client and service are on the little systems. Xremote works ok.
2. If the Xremote client is on big endian system, when you run client first time, firefox will hang. And if you run another client, it will tell you firefox is already running...
3. If Xremote client is on the little endian system and Xremote service is on the big endian system, firefox on the big endian system will crash and firefox on the little endian system will just start up.
After apply this patch:
Everything will work OK on the latest build. Also there will be some problems working with the current firefox 1.5. For example, if latest firefox runs on little endian systen as a Xremote service, when you run firefox 1.5 on big endian system, the latest firefox on little endian system will crash.

Comment 17

[timeless]

that's not acceptable. xremote is supposed to be compatible. if you must make a change which causes things to fry, you're supposed to change the api version to protect things from being fried. preferably in such a way that newer clients can figure out a way to send a proper fallback message to older servers.

Comment 18

[Benjamin Smedberg]

timeless, this is the first time xremote version 2 has been released, and it's obviously broken on big-endian machines due to a mistake on my part. This is a bug fix on big-endian machines, and does not affect the little-endian API at all.

Comment 19

[Leon Sha]

(In reply to comment #17)
> that's not acceptable. xremote is supposed to be compatible. if you must make a
> change which causes things to fry, you're supposed to change the api version to
> protect things from being fried. preferably in such a way that newer clients
> can figure out a way to send a proper fallback message to older servers.
> 

I agree with you that xremote should be compatible. That is why I do not use "htonl" here. For little endian system, there is no changes. And for big endian system, it is broken in firefox 1.5 already, we do not make things worse.

Comment 20

[Benjamin Smedberg]

Comment on attachment 206484 [details] [diff] [review]
patch v2

-    if (!data || !*data)
+    if (!data || !TO_LITTLE_ENDIAN32(*NS_REINTERPRET_CAST(PRInt32*, data)))

You don't need the macro here, but I suppose it won't hurt.

Comment 21

[Leon Sha]

(In reply to comment #20)
> (From update of attachment 206484 [details] [diff] [review] [edit])
> -    if (!data || !*data)
> +    if (!data || !TO_LITTLE_ENDIAN32(*NS_REINTERPRET_CAST(PRInt32*, data)))
> 
> You don't need the macro here, but I suppose it won't hurt.
> 
Yes, but I think put the right result here is better.
By the way, do I need a second review to check it in?

Comment 22

[Leon Sha]

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

Comment 23

[Mats Larsson]

(In reply to comment #22)
> *** Bug 310860 has been marked as a duplicate of this bug. ***

Shouldn't the bugs you point out in comment #8 also be marked as dupes?

Comment 24

[Leon Sha]

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

Comment 25

[Benjamin Smedberg]

Duplicates should only be marked if we're sure that the same issue is causing the problems (big-endian architectures).

My review is sufficient to check these changes in.  This change will need to bake on the trunk for two weeks before we consider it for the 1.8.0 branch.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Is there a reason this is reinventing the wheel instead of using the NS_SWAP32 macro in nsIStreamBufferAccess.idl ? 

Comment 27

[Benjamin Smedberg]

No, other than I certainly didn't know those macros existed and it's unfortunate that they're in IDL files.

Comment 28

[Leon Sha]

(In reply to comment #27)
> No, other than I certainly didn't know those macros existed and it's
> unfortunate that they're in IDL files.
> 

Me too. I really don't know such macros exist.
I have checked the macros, they are no different than PR_htonl and PR_htonl. I have explained the reason why not using PR_htonl.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> I have checked the macros, they are no different than PR_htonl and PR_htonl.

All I'm saying is that you replace:

+#ifdef IS_BIG_ENDIAN
+#define TO_LITTLE_ENDIAN32(x) \
+    ((((x) & 0xff000000) >> 24) | (((x) & 0x00ff0000) >> 8) | \
+    (((x) & 0x0000ff00) << 8) | (((x) & 0x000000ff) << 24))

With

+#ifdef IS_BIG_ENDIAN
+#define TO_LITTLE_ENDIAN32(x) NS_SWAP32(x)

and leave it as a no-op in the IS_LITTLE_ENDIAN case.

I agree that these macros could use a better place to live; might want a followup bug on that.

Comment 30

[Benjamin Smedberg]

Boris, that won't work because NS_SWAP32 is #ifdef IS_LITTLE_ENDIAN

Comment 31

[Boris Zbarsky [:bzbarsky]]

Ugh.  Damn fastload.

OK, sounds like we need a followup bug to centralize these various byte-swapping macros.  After this checkin we'll have them defined in slightly different but equivalent ways in at least 3 places.  :(

Comment 32

[Leon Sha]

Checking in widget/src/xremoteclient/XRemoteClient.cpp;
/cvsroot/mozilla/widget/src/xremoteclient/XRemoteClient.cpp,v  <--  XRemoteClient.cpp
new revision: 1.19; previous revision: 1.18
done
Checking in toolkit/components/remote/nsGTKRemoteService.cpp;
/cvsroot/mozilla/toolkit/components/remote/nsGTKRemoteService.cpp,v  <--  nsGTKRemoteService.cpp
new revision: 1.4; previous revision: 1.3
done

Comment 33

[Leon Sha]

(In reply to comment #31)
> Ugh.  Damn fastload.
> 
> OK, sounds like we need a followup bug to centralize these various
> byte-swapping macros.  After this checkin we'll have them defined in slightly
> different but equivalent ways in at least 3 places.  :(
> 
Yes, we need to do this in the same place.
Also I think NS_SWAP32 is not a good name now.
NS_SWAP32 look like it only do the byte swap, but in fact it do the byte swap for little endian.

Comment 34

[Leon Sha]

Checking in widget/src/xremoteclient/XRemoteClient.cpp;
/cvsroot/mozilla/widget/src/xremoteclient/XRemoteClient.cpp,v  <--  XRemoteClient.cpp
new revision: 1.17.8.2; previous revision: 1.17.8.1
done
Checking in toolkit/components/remote/nsGTKRemoteService.cpp;
/cvsroot/mozilla/toolkit/components/remote/nsGTKRemoteService.cpp,v  <--  nsGTKRemoteService.cpp
new revision: 1.2.8.1; previous revision: 1.2
done

Comment 35

[Benjamin Smedberg]

Umm, this patch was not granted approval1.8.1, only blocking 1.8.1. You should not have checked this in on the 1.8 branch.

Comment 36

[Leon Sha]

(In reply to comment #35)
> Umm, this patch was not granted approval1.8.1, only blocking 1.8.1. You should
> not have checked this in on the 1.8 branch.
> 
I thought blocking 1.8.1 means that I got the approval to check this in on the 1.8 branch. So I should back it out?

Comment 37

[Boris Zbarsky [:bzbarsky]]

Probably, yes.  And request approval flags on the patch itself, with the relevant risk-benefit analysis, etc.

Comment 38

[Leon Sha]

Done the back out.

Comment 39

[Leon Sha]

Comment on attachment 206484 [details] [diff] [review]
patch v2

This is a feature lost bug for solaris sparc. About 50% of the feed back on solaris sparc build is about this problem. Also this fix only change the code in big endian side. Ths risk should be very low.

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 206484 [details] [diff] [review]
patch v2

This is a pretty serious issue for MIT, actually (having a sparc and x86 network)...  I think this is worth taking on 1.8.0.x as well.

Comment 41

[Benjamin Smedberg]

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

Comment 42

[Mats Larsson]

What versions of FF and TB will include this fix?

Comment 43

[Leon Sha]

(In reply to comment #42)
> What versions of FF and TB will include this fix?
> 
If it get approval1.8.1, the fix will be in firefox 2.0.
And if it get approval1.8.0.1, the fix will be in firefox 1.5.x

Comment 44

[Mats Larsson]

When is it up for approval?
And where does one vote for 1.8.0.1 ;)?

Comment 45

[Tony Mechelynck [:tonymec]]

(In reply to comment #44)
> When is it up for approval?
> And where does one vote for 1.8.0.1 ;)?
> 

Mats, you or I don't vote. The attachment has "approval1.8.1?" and "approval1.8.0.1?" meaning approval has been requested. If and when some guru with the appropriate privilege gives that attachment the go-ahead, it will become "approval1.8.1+" and/or "approval1.8.0.1+". You and I can just sit on the sidelines and wait for it without making ourselves too obnoxious (because if kibitzers make themselves too annoying the guys who can really fix it will turn away from the bug).

Comment 46

[Tony Mechelynck [:tonymec]]

P.S. What we _can_ do is vote for the _bug_ ("Vote for this bug" above the large fill-in box near the top of this page).

Comment 47

[Mats Larsson]

(In reply to comment #45)
> Mats, you or I don't vote.

I know that, hence the smiley.

Comment 48

[Mats Larsson]

(In reply to comment #46)
> P.S. What we _can_ do is vote for the _bug_ ("Vote for this bug" above the
> large fill-in box near the top of this page).

But the bug is already fixed, no need to vote for it then is it. I just wanted to point out my interest for it to hit FF/TB ASAP ;)

Comment 49

[Daniel Veditz [:dveditz]]

Comment on attachment 206484 [details] [diff] [review]
patch v2

a=dveditz for 1.8/1.8.0.1 branches. Please add the fixed1.8.1 and fixed1.8.0.1 keywords when checked in.

Comment 50

[Boris Zbarsky [:bzbarsky]]

Leon, I took the liberty of checking this in on the branches for you, since the 1.8.0.1 code freeze is pretty soon.  I hope that's ok.  If you'd prefer I didn't do that in the future, let me know, please.

*** Committing to MOZILLA_1_8_BRANCH... 
XRemoteClient.cpp
new revision: 1.17.8.4; previous revision: 1.17.8.3
nsGTKRemoteService.cpp
new revision: 1.2.8.3; previous revision: 1.2.8.2

*** Committing toolkit/components/remote/nsGTKRemoteService.cpp on MOZILLA_1_8_0_BRANCH... 
new revision: 1.2.16.1; previous revision: 1.2
*** Committing widget/src/xremoteclient/XRemoteClient.cpp on MOZILLA_1_8_0_BRANCH... 
new revision: 1.17.8.1.4.1; previous revision: 1.17.8.1

Comment 51

[Boris Zbarsky [:bzbarsky]]

Wait.  Does the XPFE Xremote service also need changes (since there were widget changes in this bug)?

Comment 52

[Leon Sha]

(In reply to comment #50)
> Leon, I took the liberty of checking this in on the branches for you, since the
> 1.8.0.1 code freeze is pretty soon.  I hope that's ok.  If you'd prefer I
> didn't do that in the future, let me know, please.

That's OK. Thanks a lot.
I've checked XPFE Xremote. I found they are using the same service of toolkit.
Maybe Benjamin can confirm this.

Comment 53

[Benjamin Smedberg]

xpfe xremote is not affected by this bug (since it does not use entire-command-line remoting).

Comment 54

[Gowri Kandasamy]

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

Comment 55

[Benjamin Smedberg]

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

Comment 56

[Eric]

I don't know that this is the place, but can anyone explain why Firefox behaves this way?  This is unique behavior in UNIX -- no other program I know of works this way.  It effectively prevents a user from running firefox displayed to several different remote X hosts, which is not that uncommon (think shared user accounts).  Really it is just the breaking of decades of expectations of being able to display a program wherever you want that bothers me.