Closed Bug 86021 Opened 24 years ago Closed 23 years ago

enable -turbo for multiple profile cases

Categories

(Core Graveyard :: Profile: BackEnd, defect)

Product:
Core Graveyard
Component:
Profile: BackEnd
Platform:
x86
Windows 2000
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.4

People

(Reporter: cathleennscp, Assigned: ccarlen)

References

Details

(Whiteboard: nsbranch+)

Attachments

(10 files)

Patch to support multiple profiles
18.42 KB, patch
Details | Diff | Splinter Review
updated patch
18.37 KB, patch
Details | Diff | Splinter Review
mailnews patch option#1
1.03 KB, patch
Details | Diff | Splinter Review
mailnews patch option#2
2.05 KB, patch
Details | Diff | Splinter Review
full patch for profile switching
20.06 KB, patch
Details | Diff | Splinter Review
updated patch
29.48 KB, patch
Details | Diff | Splinter Review
better patch
31.54 KB, patch
Details | Diff | Splinter Review
Small update to not use GetArguments()
5.09 KB, patch
Details | Diff | Splinter Review
updated tree-wide patch
25.95 KB, patch
Details | Diff | Splinter Review
updated patch - 2
25.44 KB, patch
Details | Diff | Splinter Review
Blocks: 75599
Target Milestone: --- → mozilla0.9.2
Accepting.
Status: NEW → ASSIGNED
Whiteboard: PDT+
The first cut of this patch is working - I'm using it to post this! A bit more testing and checking, but it looks like it will work.
The patch does the job. When starting the visible instance of mozilla, the profile picker comes up instantly so the -turbo difference is apparent. After selecting the profile, the time to create the first window is slightly longer than in -turbo mode with only one profile. After starting the visible instance, all of my chrome, sidebar, bookmarks, passwords, mail, etc. are properly set to the profile I picked. I could use some additional testing though.
vishy, are you still free to do an opt build with ccarlen's patch this morning??
Conrad, AWESOME. Cathleen - I will do so, thanks for reminding - I'll be into the office in a bit, I'll kick the build off around 930am PST and it shd take about 3 hours. Lets anticipate that I will distribute the build around 2pm.
vishy, got us a build yet??
still building, shd be ready in abt an hour.
One thing I found today but it's fixed already - clicking "Exit" in profile picker does not exit.
Ouch. Applied this patch but it did not work. I think the problem is in nsNativeApSupport.cpp, the patch is based on rev 1.24, but meanwhile the CVS repository is up to 1.26 and I think there are conflicts between this patch and 1.26. I tried to resolve as much as possible but net effect was that it did not work as expected. It just starts in some profile, does NOT bring up profile picker. Conrad - can you update your tree and give me a new patch? thanks, Vishy. also adding putterman as once we have this built, mailnews people shd test since multiple profiles can impact mailnews most likely.
Ugh. I had updated my tree friday afternoon and I see nsNativeAppSupportWin.cpp has changed twice since then. I'm working on a new patch.
For the ease of people who have not applied the patch and wish to, the update is the whole patch although only nsNativeAppSupportWin.cpp and profileSelection.js are changed since the last patch.
Starting optimized build...
I've got an optimized build. Where should I put it?
Questions: Does the profile picker dialog come up after closing all windows and starting again? (Without exiting that is.) How about triggering the two different modes of turbo from the command line with turbo=whatever? Law opened this possibility in Bug 81712.
> Does the profile picker dialog come up after closing all windows and > starting again? (Without exiting that is.) No. Once the profile is set, it stays set until the app is exited. Having what you mention would be fully dynamic profile switching. We're not quite there yet, but almost. It's not going to happen for this bug. > How about triggering the two different modes of turbo I'm not sure what you mean WRT the two different modes.
I just tested conrad's optimize mozilla build, it works pretty good! test case: created an additonal profile (if you only have one) 1) envoke turbo mode > mozilla -turbo 2) launch mozilla, > mozilla 3) verify that profile picker comes up instantly 4) select a profile, mozilla window comes up (my machine is 600mHz, so it went pretty fast here too, about 4 secs till I see my first app window) 5) close mozilla by click on x on top right corner 6) launch mozilla again > mozilla 7) verify app window comes up instantly, (there is no profile picker now, you go straight to your app window) 8) File|Exit 9) launch mozilla again, > mozilla 10) verified that you see splash screen and profile picker, as in normal launch
I like the fact that while in turbo mode, if you start your app again the 2nd time, or 3rd time if you count the first -turbo launch, you don't need to select a profile, and app window just comes up instantly, but just to be caution, should we need to worry about this feature in shared machine environment?? to remind users in those environment to shut down the app by File|Exit?
Adding alec and bhuvan for sr/r. The gist of the patch is: From bug 81706 the profile mgr is told whether it can show UI at startup. If it needs to and can't it returns a special error code. From that patch (checked in) when apprunner gets this error code, it simply exits. In this patch, when I get that error code, I tell the native app support that it needs to call the profile mgr startup again when a visible instance of mozilla is started. Initialization continues from this point as normal - there is just no profile. nsNativeAppSupportWin, whenever it would make a window as a result of a DDE message, calls a method EnsureProfile which checks this flag and, if true, starts up profile telling it that it *can* show UI. Profile starts up, profile change observers are notified and then we can make a window. It's pretty simple. One thing I had to do was make profile mgr not use the appshell service for doing its dialogs. Doing this kills off the native app support. I still had the problem of closing the last window quitting the app so I put an attribute on appshell to turn this off. In main, this attribute is set to not quit and then, before starting the event loop, it's set to its normal state of quitting on closing the last window. We need this for any dialog which may need to be shown before starting the event loop.
I also tested the binaries that Conrad posted. Works well in the usual cases i.e. I did a sequence similar to cathleen. I also tested cases where you pass the profile on the command line 1. mozilla -turbo -P vishy\; then mozilla. This skips the profile picker and starts in the correct profile. 2. mozilla -turbo ; then mozilla -P vishy. This brings up the profile picker which is not correct, it shd just start in the profile vishy. 2 is not important for most consumers, but it is incorrect and I'm worried if its hiding other similar cases. Could you address case 2, Conrad? thanks, Vishy
so, conrad, while you're taking a look at DDE stuff for catching -ProfileManager and -p arguments, I talked with chofmann and he came up with an idea of making a pref to switch between this feature and bug 81706, so that both code can exist in the tree, depends on user experience we can switch on/off at the last minute. Is this cool?
> Is this cool? I thought work was already being done on a pref to turn off -turbo mode by either having or not having the -turbo shortcut present. Yeah, I think we should have that. That doesn't have to do with bug 81706. If the pref is off and the shortcut is not present, we'd never run into the situation which bug 81706 is meant to prevent.
> Could you address case 2, Conrad? Fixed it - easy enough :-)
> I thought work was already being done on a pref to turn off -turbo mode by > either having or not having the -turbo shortcut present. Yeah, I think we should > have that. That doesn't have to do with bug 81706. If the pref is off and the > shortcut is not present, we'd never run into the situation which bug 81706 is > meant to prevent. conrad, you're correct, but chofmann and I are asking something a little different. We want something for the case that if we're finding user experience problems this bug fix, instead of backing out the code, doing a rebuild and back track to fix in bug 81706, we would like a pref to switch this code off and turn the other on. Does this make sense? Is it possible to do?
Wow, since this patch is pretty wide spread, it feels like the that would put code ot check that that pref in a lot of files, unless theres an easy way to otherwise do it.
Today clicking 'x' of the last window is equivalent to executing File->Exit..right ?. We are changing that notion here. I know it's the case only -turbo is set. But, I still think we should tell users about it. Probably throw a dialog with little checkbox at the bottom that says don't show this again and explain the scenario there OR doing something at the end to tell users that app is not closed completely (if that's possible). This could be really probalamatic in a shared environment, where one user tries turbo mode and clicks on 'x' as usual to quit the app and goes away. Next user comes up and clicks on the icon only to land in last user's profile. Had the first user stroed his mail passords using password manager, the second user now has access to first user's mail. Let me know If I am missing something here. How is console -profilemanager going to behave on already turbo mode (i.e., having run -turbo once) mozilla instance ? bhuvan
> Today clicking 'x' of the last window is equivalent to executing > File->Exit..right ?. We are changing that notion here. That notion has already been changed with Bill's earlier checkins for -turbo mode. There is supposed to be an indicator (a system tray icon) which lets users know the app is still running even though the last window is closed. > Next user comes up and clicks on the icon only to land in last user's profile. I don't think the security is any more an issue here than normally. Since we don't password protect our profiles, they're wide open in any case.
Is the system tray fix getting in 092 ? That will be useful. I was actually concerned about the users who logged in once (didn't store passwords in the database) in to their mail apps and then walk away by clicking on 'x'. The password is still remembered which was not the case in non -turbo world.
The system tray fix is a different bug, targeted to m0.9.2. Lets check this bug in after reviews, super-reviews etc and continue to work on the other one.
and don't forget that we do provide the capability to password-protect wallet - but like bhuvan said, if the user ends up in the same profile... anyhow, looking at the patch is looks good (and thanks for the explanation, it really helped...) My comments: - can you change HandleArbitraryStartup to GetStartupUrl (or something better if you think of it) since it's now just getting the task URL, not actually opening the window. - I don't understand why you're creating an nsIDialogParamBlock object - you should just be able to pass in null to OpenWindow() no? other than that, patch looks good.
If mailnews was a profile change observer, when the last window is closed, I could call nsIProfile::ShutDownCurrentProfile(). With the other observers, this causes wallet, cookie, http auth, etc. to flush all user data like passwords. But not mailnews. Now, when the last window is closed, the last used profile (with all its passwords) is alive and well. It should be shut down. Up to now, I never considered this nescesary because, if somebody wants to leave their machine and have their passwords safe, wouldn't they just quit the application, log out (of Windows) or shut down? Also, this problem happens with -turbo mode whether we support one profile (current case) or multiple profiles
ah! good to know. For the sake of security, can we make nsMsgAccountManager into a profile change observer? at the very least we could flush the user's passwords, even if the 'correct' behavior would be to flush the user's accounts.
Are there plans to add a pref to turn the switch on/off on this turbo mode enabling on mutliprofile scenario..? i.e., more patches coming up ? if not, the patch look good to me. r=bhuvan
that's a different bug too...please see the dependant bugs off of bug 75599
- can you change HandleArbitraryStartup to GetStartupUrl Yeah - that sounds good. - I don't understand why you're creating an nsIDialogParamBlock object - you should just be able to pass in null to OpenWindow() no? It's due to a quirk with windowwatcher. Since it doesn't have OpenDialog() and OpenWindow(), it determines that it's a dialog if you pass an arg to it. And, it has to be a dialog in order for chrome JS security permissions. I could have just use an nsISupportsInt or something, but I have plans to make profile mgr use dialog param blocks anyway. Yes, at this point it's a bit odd and wasteful, but soon that dialog param block will do some good. If the wastefulness is a problem, I can use a lighter weight object as the param. I'll comment it in any case.
ok, that all sounds fine sr=alecf (on a side note, I really think we need a better interface than nsIDialogParamBlock.. but that's another beef of mine)
> ah! good to know. For the sake of security, can we make nsMsgAccountManager into a profile change observer? That would be perfect. If this gets checked in tonight (I'm slightly more hopeful than I was a while ago), can this work be done and checked in before we branch?
Bhuvan, how hard would it be to make the account manager a shut down observer and clear the passwords?
Instead of being a shutdown observer, it should be a profile change observer. That's the mechanism by which all the rest of these things work. We don't want to shut down the app, we want to shut down the profile.
sorry, that's what I meant.
a=chofmann for drivers
Blocks: 83989
Fix checked in - just barely. My pre-checkin builds were taking forever, got broken along the way by other redness, and then a last minute conflict.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
>if you start your app again the 2nd time, or 3rd time if you count the first >-turbo launch, you don't need to select a profile, and app window just comes up >should we need to worry about this feature in shared machine environment?? to This is sort of what I had in mind about "two modes". Here are two scenarios I see coming up: 1. Some might like a smaller memory footprint when not using Mozilla; if the multi-profile -turbo option reduces memory use, let's allow those people to set it. 2. In a shared maching environment it would be nice to drop out of the profile after closing the last window. I don't know if this is doable in mozilla.exe; it may require a separate "startup agent" like M$ uses for Office to preload the libraries.
> 1. Some might like a smaller memory footprint when not using Mozilla; if > the multi-profile -turbo option reduces memory use, let's allow those people > to set it. Multi-profile vs. single-profile doesn't affect memory use. > 2. In a shared maching environment it would be nice to drop out of the > profile after closing the last window. I agree. I think we should do this.
I have other work in nsPreloader to do what you're talking about - have an intermediate, minimal memory use, profile-independent preloader.. I have a bug on it somewhere...
One approach to make it so that hitting x on the last open window keeps mozilla resident but will bring up the Profile Manager again (in the multiprofile case) when someone clicks the system tray icon is to - actually exit Mozilla (i.e. do not stay resident when the last window is closed) - run mozilla -turbo again so that you get resident once more This would enable not requiring a bunch of code to be made "Profile Change Observer" aware. Doubt that this shd be a priority though, as long as we document the turbo behaviour well, I hope that that will suffice for now.
- actually exit Mozilla (i.e. do not stay resident when the last window is closed) - run mozilla -turbo again so that you get resident once more Wouldn't that defeat our whole purpose here? > This would enable not requiring a bunch of code to be made > "Profile Change Observer" aware. For all other observers, it really didn't require much code. It usually just involves caling a cleanup method which already exists.
Conrad, I think Vishy is suggesting that this happens in the background, so the effect to the user is the same, except they see profile manager when they start up again after the series of steps vishy describes. This of course would not defeat the purpose of turbo, unless it tied up a users machine while it was restarting turbo mode in the background. I could be wrong though, vishy?
you can't just make it happen "in the background" - you can't simply create 2 minutes of CPU time. Let's let conrad fix this the right way, he's got a handle on the situation.
Thanks Alec :-) There are two possibilities and two patches here. I'll just explain the two approaches for the sake of discussion. #1 - As I thought, making the account manager a profile change observer did not involve adding a bunch of code. It was as simple as calling an existing shutdown method. This will unload all accounts, passwords and all. The problem with this, as Alec says, is that while it uses existing methods to do this, those methods have not been well tested under this context. If we have enough confidence in this existing code or are willing to enough testing into it to prove that it works, it could be ideal. When the last window is closed in -turbo mode, I can shut down the current profile, notification happens, and passwords are dumped (from everything, not just mail) When the next person comes up to the machine and starts a visible instance the profile picker comes up and - here's the good part - they can choose any profile they wish. #2 This one is safer because, in response to a profile change notificaion, it just goes through the servers and calls ForgetPassword. Everything else that is a profile observer would do the same. Since we have not unloaded the accounts, the next time a visible instance is started, we have no choice to select (without showing a dialog) the profile that was chosen initially. The only advantage of this is that all of the passwords have been flushed. Obviously, I'm in favor of option #1. It would be the best as far as user experience and flexibility. Given time constraints, I understand if we don't want to go that route.
how about #1 on the trunk, and #2 after we branch? The other option is to get #1 in today, have QA beat on it for a day or two, and then make a decision if we're going to back it out and put in #2. I personally recommend that route, as this ONLY affects the case of closing down a profile in turbo mode, and so we won't break existing functionality. let's ask drivers@mozilla.org if they're willing to do it. The key is that we need to make the 2nd decision (whether to back out or not) before 0.9.2 really branches... In either case sr=alecf on both of the patches.
so, I tried conrad's 2 new builds, and here is what I experienced: A) flush password build I found a couple of problems with this build. 1) start -turbo mode, launch app then x-close. launch app again then x-close doing it several times (normally takes 2 tries), my home page doesn't load anymore.... I verified this is a problem, even if you only have single profile, so I'm not sure if this is a problem in the build or conrad's fix. 2) while in -turbo mode, open mail window, get mail, type in password. x-close all windows. launch app again, open mail window, my mail messages gets filled in right away. password didn't seem to get flushed... B) switch profile build I didn't find any problems with this build, so far. x-close window, launch app again will bring you straight to profile picker (no splash screen either), so you can switch to a different profle while in turbo mode. open mail window, enter password, read a few email, x-close. next time you launch mail (-mail or task|mail), you get a password dialog too! I did a quick check with single profile case, and found no problems so far. :-) At this point, in my opinion, I definitely go with switch profile build.
> At this point, in my opinion, I definitely go with switch profile build. OK. With the flush passwords build, in my testing here, it did flush the passwords - both mail and http. Not sure what the problem is there. Since the profile switching build looks more promising, I think some serious testing is in order. Things like: * start up the first time in a profile with one skin, next time around, choose a profile with another skin. Do this several times. * start up with a profile made by mozilla and, next time pick one made by commercial. I think there have been skin problems with this in general but it would be good to test in this context. * switching between profiles and using PSM certs. * general pounding on by different people/different machines. One thing I have found is that, if the 2nd time around, I create a new profile with commercial and go through activation, it asks if I want to remember the values for next time :-) Handy but not what we want. I guess the reason this doesn't happen normally is that wallet is initialized after activation.
cc'ing paw and gbush. paw, can we get some more testing help too? :-) also, I think we should keep this bug open, till all details are sorted out. :-)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Using the turbo build dated 6-24-01 with password flush: The mail password is not flushed. Using the turbo build dated 6-24-01 with switch password: Launching a profile in Modern theme, closing then launching again doesn't display the scroll bar in the Profile manager. This does not happen with a profle in Classic Launching a profile using -mail (no browser window opened) crashes giving the password on the same mail account on the 2nd launch. A crash while in Turbo mode, disables Turbo mode, just and fyi...user may not realize it.
Investigating what esther found...
tried both builds (WinNT) flush passwords mode after starting turbo, I had to run 'netscp6' many times to get it going. Does not come on desktop rather in bottom tray. Tried running many times/ closing and restarting turbo. At each close using file/exit - I would crash - reproducible saw same things as Cathleen, ie passwords did not flush- able to go back into mail many times and send/receive without entering password not able to switch skins switch profile mode This was better, app coming up on desktop, mail asks for password I was not able to switch skins here either flush opened with modern/ switch opened with classic- same profile used on both cases
Conrad, with your special turbo builds dated 6-24, I and another QA tester crashes opening a Secure page. I don't see any reproducible bugs on the branch or trunk builds for this. Just to be sure, could we get a more recent build with your fixes to make sure the crash is not in the core product.
Esther, I should have a new build tomorrow. Since the profile-switching build seems to be better behaved and is more desirable, I'm working on that one. I will put a link to it here when I have it. > I and another QA tester crashes opening a Secure page. When this happens, is this the 2nd time around (running after the 2nd time through the profile picker)?
The secure page crashes with or without turbo mode on with the build you gave us. Happens anytime I go to a secure page whether it's in a first time session or later sessions Also, My browser page doesn't display content with multiple launches of the same profile in turbo mode. Example. In command line launch turbo mode using -turbo, Launch app now in turbo mode, X close the Browser (mine comes up with Browser page). Launch app again select the same profile, Browser page may be void of content at this time, if not X close again then relaunch, select the same profile, you should see the content missing this time. Note: this only happens if the profile manager comes up because you have more than 1 profile.
To see the secure page crash, I think you can go to www.techfed.com, then click on the member sign in link on the right side of the page. Crash! all the time also happens signing in to member sign in on the www.shutterfly.com page.
> The secure page crashes with or without turbo mode on with the build you gave > us. Happens anytime I go to a secure page whether it's in a first time session > or later sessions Then it's not related to this. Must be something bad in general with that build. > Also, My browser page doesn't display content with multiple launches of the > same profile in turbo mode. This does happen ocasionally (but rarely) for me so I'm debugging it now.
Status: REOPENED → ASSIGNED
> Launching a profile in Modern theme, closing then launching again doesn't > display the scroll bar in the Profile manager. This is fixed. On a profile shutdown, not all things were being released from chrome. This may have been causing some other skin problems as well.
conrad, will you be able to post another test build out for people to try by tomorrow? also, PDT wanted to know if everything is good, if you can check this in by tomorrow night? and if possible, add an estimate of time for completion to the status whiteboard. -thanks!!! :-)
Yes. I'll be able to post another build and a full patch tomorrow. Tomorrow night or early (EDT) firday is possible - if the next build I post fairs better in testing.
Notes about the patch: (1) The change to single signon is needed because, as a profile change observer, if the last instance is deleted, there's nothing to receive the notification. Not sure why this had gone unnoticed - maybe we were leaking single-signons? (2) The change to prefapi.c is also a general profile switching bug. For still unknown reasons, it's possible for hash table enumeration to go into an infinite loop if entries are being deleted. A bug in hash table implementation? This avoids it. (3) The changes to chrome, localstore, and nsNSSComponent were needed to support profile switching in which the profile is shut down, and those services are used in this state. The change to nsNSSComponent is bug 88230 since I can't check that in. (4) The changes to mailnews account mgr are straightforward. It has never been a profile change observer.
Depends on: 88230
cool... this all looks great - watch the tabbing in nsINativeAppSupport.idl Also, could you add a comment in single signon that mgProfileObserver is a weak reference (by the way, do you even need to hold that reference? I don't see it used anywhere) - I'd hate to see someone blindly change that to an nsCOMPtr<> thinking they were doing some good :) sr=alecf with those changes (and optional removal of mgProfileObserver)
so, conrad, does your latest patch work in commercial build too? when you sent out a test build earlier today, which was only a mozilla build, and you mentioned there were some problems in commercial builds... what wasn't working?
Keywords: nsBranch
In commercial: (1) IM needs to log out on a profile switch - working on it. (2) I was able to reproduce the problem Esther reported about, after a profile switch, sometimes the next window will not draw content. It's not easily reproducible, and happens only on commercial.
pushing out. 0.9.2 is done. (querying for this string will get you the list of the 0.9.2 bugs I moved to 0.9.3)
Target Milestone: mozilla0.9.2 → mozilla0.9.3
Conrad, regarding #2. First I want to make sure it's understood that I'm not swithching profiles when I see the blank browser pages, I'm opening the same one over and over again. This was so reproducibe on my system, but not Stephens so I check with Stephen and the only difference was that he only had one profile so he never saw the Profile Manager come up during a relaunch in turbo mode. Once we added another Profile so he would get a Profile manager, he could reproduce this starting with the 3rd launch of the same profile using turbo mode. Hope this helps you to reproduce it easier.
This is with the build I made yesterday?
I took the mozturbo build "moz-turbo-switch.zip" dated 6-28 and still crash mail when I Launch a profile using -mail (no browser window opened) crashes giving the password on the same mail account on the 2nd launch. 1. I launched your build and when I got the Profile Manager I created a New Profile 2. I launched this new profile and added (2) mail accounts 1st one imap 2nd one pop. Opened each, giving password to verify they work. 3. I Exited the app 4. In Windows Start/Run command line I gave the location of the mozilla.exe and set the turbo mode using -turbo 5. Using the same command line I changed -turbo to -mail to launch the app in mail only. 6. I logged into my imap account giving password and opening 1 message. 7. File|Close from menu 8. Using same command line I launched app -mail again. IMAP password dialog came up 9. I gave password for my IMAP account and crashed. No talkback in this app so I can't give you that information. The OS error details says the error was in gkcontent.dll offset 000fa690 if that helps. Also note: I'm in Modern Theme.
I took the mozturbo build "moz-turbo-switch.zip" dated 6-28 and still crash mail when I Launch a profile using -mail (no browser window opened) crashes giving the password on the same mail account on the 2nd launch. I still crash going to a secure page (with or without turbo mode).
I took the mozturbo build "moz-turbo-switch.zip" dated 6-28 I don't have the problems with the Browser page coming up blank on repetitive launches.
I took the mozturbo build "moz-turbo-switch.zip" dated 6-28 I don't have the problem with the Modern Theme profile close bringing up a Profile Manager without a scroll bar the next time I launched.
> I took the mozturbo build "moz-turbo-switch.zip" dated 6-28 and still crash > mail when I Launch a profile using -mail (no browser window opened) crashes > giving the password on the same mail account on the 2nd launch. Found what it takes for this one: The crash happens if "Use Secure Connection" is on, never if not. Now that I know that, I'm on it. > I don't have the problems with the Browser page coming up blank on repetitive > launches. Right. That's with mozilla. With commercial, after some investigation, it's still a mystery. FWIW, I have 4 profiles. Here's what happens: 1) "netscp6 -turbo" 2) "netscp6" - Profile picker comes up, I pick a profile, browser opens, all is well. 3) Close the last window 4) "netscp6" - Profile picker comes up, I pick the same profile as last time, browser window comes up, has no content, most buttons don't work. The reason that there's no content is that, the 2nd time around, navigator.js fails to be compiled. At this point: http://lxr.mozilla.org/mozilla/source/content/xul/content/src/nsXULElement.cpp#4823, the compilation fails with a js error "missing ) after condition". How can this be? It compiles the first time around with commercial and, with mozilla, whichever time around, it always works.
*** Bug 88731 has been marked as a duplicate of this bug. ***
I wonder if we've lost some of the characters in the prototype or something? I'll see if I can reproduce...
cc'ing Brendan on the chance this compilation problem may be related to one he encountered in bug 68045: ------- Additional Comments From Brendan Eich 2001-06-17 01:28 ------- See FASTLOAD_20010529_BRANCH for work in progress. Anyone who figures out why the changes there (which save a navigator.mfasl file on Unix in one's profile directory after first startup, which file currently contains serialized scripts, principals and URL objects; this file is read by subsequent startups to recover compiled scripts, successfully AFAICT from watching in gdb) seem to cause a XUL syntax error to be reported (navigator.xul line 318 column 5, unclosed token) gets a secret reward from me. Back in five days. /be
> I wonder if we've lost some of the characters in the prototype or something? > I'll see if I can reproduce... Losing bytes from the .js we're compiling? If that's what you mean, I could find out pretty easily.
I can't shed much light, I'm afraid. My FASTLOAD_20010529_BRANCH problems are fixed, and the cause of what seemed like a spurious navigator.xul syntax error in my case turned out to be an impurity. It seems to me the parser/XUL/JS code paths are sensitive in ways that lead to spurious errors. My case was *not* one where a string buffer had a NUL stored early (impurely), terminating the input at a syntactically invalid point. Rather, a bounds pointer limited input to the parser after it should have buffered all of navigator.xul, such that only a few lines of input where buffered (not enough to contain an entire tag, hence the syntax error I saw). I never figured out what caused this problem in detail; once I found a bug in my code and fixed it, all was well. Sorry I can't help more. /be
Conrad, this is PDT+ bug. Tomorrow, on Tuesday, we'll try to build the first RTM candidate. It would be good, if this could be resolved ASAP.
Belive me, I'm working on it. This problem: > I took the mozturbo build "moz-turbo-switch.zip" dated 6-28 and still crash > mail when I Launch a profile using -mail (no browser window opened) crashes > giving the password on the same mail account on the 2nd launch. is just fixed. That still leaves the problem of no content drawing in a browser window 2nd time around with commercial (a tough problem). It also leaves the problem of logging out of AIM on commercial when the profile is shut down (not too hard).
I just tried 2001070203 Windows build on Windows 98. There is a regression - with turbo mode, the profile manager does not come up anymore even when you start the profilemanager shortcut. We need to fix that. thanks, Vishy
vishy - trunk or branch?
this bug is staying open for the trunk only... :-) bug 81706 or we should open a new bug to make the branch fall back to the way it was.
opened bug 89157 for nsBranch tracking.
*** Bug 89298 has been marked as a duplicate of this bug. ***
Removing PDT+, this is not the fix we want for the branch. (The branch needs the fix to exit -turbo when multiple profiles exist.)
Whiteboard: PDT+
*** Bug 90799 has been marked as a duplicate of this bug. ***
> The branch needs the fix to exit -turbo when multiple profiles exist. That doesn't make sense. All PCs I've set up have multiple profiles, and disabling -turbo when exiting would completely ruin the whole purpose of having -turbo in the first place. The final solution must be that with multiple profiles and -turbo enabled, the user can exit with "file - exit" or "X" and the behaviour is the *same* - the *next launch asks for which profile to load*. Anything else is just going to confuse the hell out of everybody.
The updated patch is working well. All problems QA found (and then some) with the optimized builds are fixed. This patch also plugs the privacy hole that we have in the single profile case. Currently, when you close the last window, all passwords are still available, IM doesn't log off, etc. This patch fixes that as well. In this case, shutting down the profile and the re-selecting it has some additional overhead. I'll have to make some optimized builds and see if it's noticeable.
Bhuvan, Blake - Can you r=/sr=?
Did some testing on optimized builds (opt build with this patch vs. nightly from same time) in single profile case where, with this patch, passwords are flushed (because the profile is shutdown). The difference in speed (time to create first window, 2nd time around) was barely noticable. Possibly 1/2 sec? I'd say plenty acceptable given the benefit.
Differences in "better patch": Added nsINativeAppSupport::OnLastWindowClosing() This is where the current profile is shut down. It makes use of nsDOMWindowInternal::GetArguments() which is new, to distinguish the closing of the initial, invisible turbo window. In the previous patch, I closed this window after making it - the same as was done for bug 89166. I found that this kills the window before the chrome has fully loaded which isn't good. It's better to allow the window to load fully, and close it from the onLoad handler. Looking for a "turbo=yes" arg on the window in OnLastWindowClosing() allows it to be distinguished. I could really use the review soon. I'm going away at the end of next week and need to get this in well before that.
I think this needs to be turned around abit, nsIDOMWindowInternal::GetArguments() should not reflect the value of window.arguments, it should reflect the arguments that were passed into window.openDialog(), either from C++ (in which case there's already an nsISupportsArray that you can pass to the GlobalWindowImpl for holding on to) or from JS (in which case you haveto do the JS to XPCOM conversion that your patch already does). This would mean moving the window.arguments JS array to nsISupportsArray conversion into the window watcher and making it possible to give this arguments array to an existing window, this you could do by adding a setter for this array in nsIScriptGlobalObject, or something. If we'd go with this patch then web content could potentially fool code in mozilla to thinking a window was opened with some arguments that it wasn't opened with by building up the window.arguments array, I don't think we want that. Here's a few nits I saw when browsing through this diff (no, I didn't review the whole thing yet): - Don't remove the second empty line after the #include's in nsIDOMWindowInternal.idl. - The indentation in nsIProfileInternal.idl and nsINativeAppSupport.idl is not lined up with the rest of the code in those files. - nsProfile.h could use some PRPackedBool love... - In nsNativeAppSupportWin::EnsureProfile(), don't mix 2 and 4 space indentation, same goes for nsNativeAppSupportWin::OnLastWindowClosing()
> If we'd go with this patch then web content could potentially fool code in > mozilla to thinking a window was opened with some arguments that it wasn't Depends on what code is using GetArguments(). Using it to look for a "turbo=yes" arg wouldn't get us into much trouble. It's possible to do this without GetArguments(). I moved the logic for determining if the window being closed is the initial turbo window from nsAppShellService to nsNativeAppSupportWin after I had written GetArguments(). Since nsNativeAppSupportWin is the thing which creates this window, it can simply keep ptr to it and compare that to the window being closed. Quite a bit simpler.
The small update identifies the initial turbo window without using the additional (ignore it in the last patch) nsIDOMWindowInternal::GetArguments().
-> mozilla0.9.4
Target Milestone: mozilla0.9.3 → mozilla0.9.4
all we need are reviewers! bhuvan? blake? jst? and someone to sr= too!
I'll review when jst is finished with his...
Nothing really new in the most recent patch. Just a tree-wide diff with nsIDOMWindowInternal::GetArguments out of the picture and some whitespace problems fixed. Somehow some tabs got into the .idl files I touched. I have DevStudio set to insert spaces for tabs. Does this pref only apply to C/C++ files?? Now I'm using another editor with which I can show invisible characters.
There's also a patch on this bug's Bugscape counterpart (7192) which needs review.
Nit: + nsresult rv; Declare further down. Also may want to consider init'ing to NS_OK and just returning rv in those places. xpfe changes look good, r/sr=blake on those
Conrad, what is the status of this, can we land today? thanks! Vishy
*** Bug 93618 has been marked as a duplicate of this bug. ***
r/sr=waterson. nice work!
r=valeski
I just discovered a problem with this which is delaying checking it in. I had seen this once before and, through no change on my part, it just sort of went away. Now, it's back. The problem is: with commercial, if I startup, pick a profile, browse, close the last window, then next time around, pick the same profile, the window comes up with a blank homepage and the url bar is also empty and unresponsive. The only clue to the console is a JS syntax error in navigator.js. Narrowing it down a bit, I found it happens: * only on commercial, never mozilla * only with a profile which was made with commercial, never on commercial with a profile made with mozilla. * never with mozilla with the same profile which shows this problem on commercial. After a process of elimination on the differences between mozilla and commercial, it's due to a difference in prefs.js, specifically "browser.startup.homepage." In the case which hoses commercial, this pref is not present. If I set that pref to a non-default, the problem goes away. Another thing: If I put in dump() msgs in navigator.js, I was expecting it to enter the startup routine but not make it to the bottom. It doesn't hit the first dump() at the top of startup. Since the window's chrome loads, how can that be? Still trying to figure this out...
*** Bug 95540 has been marked as a duplicate of this bug. ***
The bug mentioned on 08-16 is still happening with no answer in sight. The only further info I have is that it happens if the homepage from the prefs is http://home.netscape.com. When I said before that the same profile on which this problem happens with commercial is OK with mozilla, it's because the default pref for that is different. If that is the homepage pref, it will happen with mozilla as well. The latest patch has no changes other than ones to fix conflicting checkins since the previous patch. If anybody is feeling really helpful, can you apply this patch, see if it happens for you, etc? For commercial, there is also the portion on bugscape 7192.
I could especially use help from those who know more than I about how JS from the chrome is loaded and executed. I put dump statements in the function startup() in navigator.js at the top and bottom. When this problem happens, I don't even hit the dump statement at the top which leads me to believe something is going wrong with loading the JS.
I'm seeing something similar to what ccarlen described: 1) Start mozilla 2) load http://home.netscape.com/ 3) open a new window If I have my XUL cache disabled, the result of step 3 will be a blank browser window, but if it is enabled (default) everything is fine.
cc'ing rginda -
Depends on: 96514
Depends on: 96562
No longer depends on: 96514
Whiteboard: nsbranch+
a=dbaron (on behalf of drivers, after discussion with Asa and Brendan)
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago → 23 years ago
Resolution: --- → FIXED
*** Bug 97363 has been marked as a duplicate of this bug. ***
verified with build 2001090703
Status: RESOLVED → VERIFIED
The launch-with-turbo speed regression caused by this fix is bug 99387.
No longer blocks: 75599
Blocks: 75599
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: