Closed
Bug 312154
Opened 19 years ago
Closed 19 years ago
Start script does not take care of running instance
Categories
(Toolkit :: Startup and Profile System, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: Mats.Larsson, Assigned: leon.sha)
References
Details
(Keywords: fixed1.8.0.1, fixed1.8.1, regression)
Attachments
(1 file, 1 obsolete file)
6.63 KB,
patch
|
benjamin
:
first-review+
dveditz
:
approval1.8.0.1+
dveditz
:
approval1.8.1+
|
Details | Diff | Splinter Review |
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•19 years ago
|
||
xremote should automatically go to the new process: can you think of any obvious reasons why xremote would not be working?
Reporter | ||
Comment 2•19 years ago
|
||
(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•19 years ago
|
||
No, like I said remote is done automatically by the binary and the script is no longer involved (tbrd/ffox).
Reporter | ||
Comment 4•19 years ago
|
||
(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."
Reporter | ||
Comment 5•19 years ago
|
||
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.
Flags: blocking1.8rc2?
Updated•19 years ago
|
Flags: blocking1.8rc2? → blocking1.8rc2-
Reporter | ||
Updated•19 years ago
|
Version: unspecified → 1.5 Branch
Reporter | ||
Comment 6•19 years ago
|
||
mtschrep@gmail.com removed the blocking1.8rc2? flag without any explanation. Why is this not a blocker?
Assignee: nobody → leon.sha
Status: UNCONFIRMED → ASSIGNED
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.
Attachment #206068 -
Flags: review?(bsmedberg)
Reporter | ||
Comment 9•19 years ago
|
||
(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•19 years ago
|
||
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?
Assignee | ||
Comment 11•19 years ago
|
||
(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.
Assignee | ||
Comment 12•19 years ago
|
||
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.
Attachment #206068 -
Attachment is obsolete: true
Attachment #206068 -
Flags: review?(bsmedberg)
Reporter | ||
Comment 13•19 years ago
|
||
(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•19 years ago
|
||
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).
Component: Startup and Profile System → XRE Startup
Product: Firefox → Toolkit
Version: 1.5 Branch → 1.8 Branch
Assignee | ||
Comment 15•19 years ago
|
||
Assignee | ||
Comment 16•19 years ago
|
||
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.
Attachment #206484 -
Flags: first-review?(benjamin)
Comment 17•19 years ago
|
||
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•19 years ago
|
||
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.
Assignee | ||
Comment 19•19 years ago
|
||
(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•19 years ago
|
||
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.
Attachment #206484 -
Flags: first-review?(benjamin) → first-review+
Assignee | ||
Comment 21•19 years ago
|
||
(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?
Flags: blocking1.8.1?
Flags: blocking1.8.0.1?
Assignee | ||
Comment 22•19 years ago
|
||
*** Bug 310860 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 23•19 years ago
|
||
(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?
Assignee | ||
Comment 24•19 years ago
|
||
*** Bug 305598 has been marked as a duplicate of this bug. ***
Comment 25•19 years ago
|
||
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.
Flags: blocking1.8.1? → blocking1.8.1+
Keywords: regression
![]() |
||
Comment 26•19 years ago
|
||
Is there a reason this is reinventing the wheel instead of using the NS_SWAP32 macro in nsIStreamBufferAccess.idl ?
Comment 27•19 years ago
|
||
No, other than I certainly didn't know those macros existed and it's unfortunate that they're in IDL files.
Assignee | ||
Comment 28•19 years ago
|
||
(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•19 years ago
|
||
> 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•19 years ago
|
||
Boris, that won't work because NS_SWAP32 is #ifdef IS_LITTLE_ENDIAN
![]() |
||
Comment 31•19 years ago
|
||
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. :(
Assignee | ||
Comment 32•19 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 33•19 years ago
|
||
(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.
Assignee | ||
Comment 34•19 years ago
|
||
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
Keywords: fixed1.8.1
Comment 35•19 years ago
|
||
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.
Assignee | ||
Comment 36•19 years ago
|
||
(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•19 years ago
|
||
Probably, yes. And request approval flags on the patch itself, with the relevant risk-benefit analysis, etc.
Assignee | ||
Comment 39•19 years ago
|
||
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.
Attachment #206484 -
Flags: approval1.8.1?
![]() |
||
Comment 40•19 years ago
|
||
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.
Attachment #206484 -
Flags: approval1.8.0.1?
Comment 41•19 years ago
|
||
*** Bug 321317 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 42•19 years ago
|
||
What versions of FF and TB will include this fix?
Assignee | ||
Comment 43•19 years ago
|
||
(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
Reporter | ||
Comment 44•19 years ago
|
||
When is it up for approval? And where does one vote for 1.8.0.1 ;)?
Comment 45•19 years ago
|
||
(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•19 years ago
|
||
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).
Reporter | ||
Comment 47•19 years ago
|
||
(In reply to comment #45) > Mats, you or I don't vote. I know that, hence the smiley.
Reporter | ||
Comment 48•19 years ago
|
||
(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•19 years ago
|
||
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.
Attachment #206484 -
Flags: approval1.8.1?
Attachment #206484 -
Flags: approval1.8.1+
Attachment #206484 -
Flags: approval1.8.0.1?
Attachment #206484 -
Flags: approval1.8.0.1+
![]() |
||
Comment 50•19 years ago
|
||
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
Keywords: fixed1.8.0.1,
fixed1.8.1
![]() |
||
Comment 51•19 years ago
|
||
Wait. Does the XPFE Xremote service also need changes (since there were widget changes in this bug)?
Assignee | ||
Comment 52•19 years ago
|
||
(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•19 years ago
|
||
xpfe xremote is not affected by this bug (since it does not use entire-command-line remoting).
Updated•19 years ago
|
Flags: blocking1.8.0.1? → blocking1.8.0.1+
Comment 54•19 years ago
|
||
*** Bug 322967 has been marked as a duplicate of this bug. ***
Comment 55•19 years ago
|
||
*** Bug 329023 has been marked as a duplicate of this bug. ***
Comment 56•18 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•