Closed Bug 1238428 Opened 4 years ago Closed 4 years ago

Workaround the NS_APP_PROFILE_DEFAULTS_{NLOC_,}50_DIR directory keys removed in Bug 1234012

Categories

(SeaMonkey :: Startup & Profiles, defect, blocker)

defect
Not set
blocker

Tracking

(seamonkey2.40 unaffected, seamonkey2.41 unaffected, seamonkey2.42 unaffected, seamonkey2.43 fixed, seamonkey2.44 fixed)

RESOLVED FIXED
seamonkey2.44
Tracking Status
seamonkey2.40 --- unaffected
seamonkey2.41 --- unaffected
seamonkey2.42 --- unaffected
seamonkey2.43 --- fixed
seamonkey2.44 --- fixed

People

(Reporter: philip.chee, Assigned: frg)

References

Details

Attachments

(1 file, 4 obsolete files)

No description provided.
Fixes build failures locally.
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Attachment #8706193 - Flags: review?(neil)
Comment on attachment 8706193 [details] [diff] [review]
Patch v1 Restore keys and fix compile errors

Whoops major braino.
Attachment #8706193 - Flags: review?(neil)
This one compiles and *seems* to work. However I'm not a C++ person at all.
Attachment #8706193 - Attachment is obsolete: true
Attachment #8706292 - Flags: review?(neil)
Comment on attachment 8706292 [details] [diff] [review]
Patch v1.1 Restore keys and fix compile errors

>+  else if (!strcmp(aKey, NS_APP_PROFILE_DEFAULTS_50_DIR) ||
>+           !strcmp(aKey, NS_APP_PROFILE_DEFAULTS_NLOC_50_DIR))
>+    return GetProfileDefaultsDir(aResult);
Are these going to be interesting for anyone?

>+    rv = dirSvc->Get(NS_XPCOM_CURRENT_PROCESS_DIR, NS_GET_IID(nsIFile),
>+                     getter_AddRefs(mAppDir));
>+    if (NS_FAILED(rv)) {
>+      rv = dirSvc->Get(NS_OS_CURRENT_PROCESS_DIR, NS_GET_IID(nsIFile),
>+                       getter_AddRefs(mAppDir));
You don't need to do both, they'll always have the same value anyway. (As will GreD which would have been the right one to use if it was necessary to do the rigmarole.)
Frustrating to build comm-central this week. Even with the patch the next error popped up today. Looks like another variable got removed.

FRG

>>
mozmake[4]: Leaving directory 'c:/seabuild/release/comm-central-15/obj-x86_64-pc-mingw32/js/xpconnect/shell'
c:/seamonkey/comm-central/suite/profile/nsSuiteDirectoryProvider.cpp(31): error C2065: 'NS_APP_BOOKMARKS_50_FILE': undeclared identi
fier
c:/seamonkey/comm-central/mozilla/config/rules.mk:956: recipe for target 'nsSuiteDirectoryProvider.obj' failed
<<
Bug 1235099

Add new bug or patch it here too?

FRG
Apparently only

#define NS_APP_BOOKMARKS_50_FILE                "BMarks"

is needed. Will check

FRG
Compiled fine with the define. Adding a new profile and trying to use it results in a crash every time you start Seamonkey.

FRG
(In reply to Frank-Rainer Grahl from comment #8)
> Compiled fine with the define. Adding a new profile and trying to use it
> results in a crash every time you start Seamonkey.
We are probably missing something. But what?
Flags: needinfo?(neil)
(In reply to Philip Chee from comment #9)
> (In reply to Frank-Rainer Grahl from comment #8)
> > Compiled fine with the define. Adding a new profile and trying to use it
> > results in a crash every time you start Seamonkey.
> We are probably missing something. But what?

Hm. My SM-Trunk Linux x86_64 built successfully using the hint in Comment 7. Runs without crashes in my normal profile. Not usable, though, with symptoms pointing to Bug 1237602.

Now I have added suite, v3 from Bug 1237602. The resulting build works fine. No problems, so far. ;)
Hartmut,

try a new profile. The code is used during migration. It add default files in your new profile and this seems to crash.

I applied the suite patch in 1237602 which hasen't been checked in yet. Not sure if it fixes everything but seems to be ok to get things started. 

FRG
Upps too early:) Should have read the whole message and saw that you already applied the suite fix. So disregard this comment.

FRG
This one seems to do the trick. Please check. I think the process dirs were the wrong ones.
Flags: needinfo?(philip.chee)
Just built a Windows installer and this one works too. Old profile still works and creation of a new one also succeeds.
1238428-NS_APP_PROFILE_DEFAULTS-V2.patch works for SM-Trunk Linux x86_64.
Comment on attachment 8706292 [details] [diff] [review]
Patch v1.1 Restore keys and fix compile errors

>+  nsCOMPtr<nsIFile> appDir;
>+  rv = mAppDir->Clone(getter_AddRefs(appDir));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIFile> defaultsDir;
>+  rv = appDir->AppendNative(NS_LITERAL_CSTRING("defaults"));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = defaultsDir->AppendNative(NS_LITERAL_CSTRING("profile"));
>+  NS_ENSURE_SUCCESS(rv, rv);
You've got too many dirs here. Clone mAppDir into defaultsDir, then append "defaults" and "profile", and it won't crash.
Flags: needinfo?(neil)
Attachment #8706292 - Flags: review?(neil) → review-
Comment on attachment 8708450 [details] [diff] [review]
1238428-NS_APP_PROFILE_DEFAULTS-V2.patch

>+  if (!mAppDefaultsDir) {
>+    nsCOMPtr<nsIProperties> dirSvc =
>+      do_GetService(NS_DIRECTORY_SERVICE_CONTRACTID, &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    rv = dirSvc->Get(NS_APP_DEFAULTS_50_DIR, NS_GET_IID(nsIFile),
>+                     getter_AddRefs(mAppDefaultsDir));
I'm not convinced we need to save this if it's always the same property.

>+  nsCOMPtr<nsIFile> appDir;
Please call this defaultsDir to reduce confusion.
Attachment #8708450 - Flags: feedback+
This patch:
1. removes references to NS_APP_PROFILE_DEFAULTS_{NLOC_,}50_DIR directory keys removed in Bug 1234012 because nothing cares about these.
2. Uses the NS_APP_DEFAULTS_50_DIR but does not cache the directory.
3. Names the var profileDir (maybe we should call it defaultsProfileDir?)
4. Re-instates NS_APP_BOOKMARKS_50_FILE for the nonce.
Attachment #8706292 - Attachment is obsolete: true
Attachment #8708450 - Attachment is obsolete: true
Flags: needinfo?(philip.chee)
Attachment #8709112 - Flags: review?(neil)
Summary: Reinstate NS_APP_PROFILE_DEFAULTS_{NLOC_,}50_DIR directory keys removed in Bug 1234012 → Workaround the NS_APP_PROFILE_DEFAULTS_{NLOC_,}50_DIR directory keys removed in Bug 1234012
Depends on: 1240798
Comment on attachment 8709112 [details] [diff] [review]
Patch v3 minimal patch plus improvements by frg

>-  NS_GetSpecialDirectory(NS_APP_PROFILE_DEFAULTS_50_DIR,
>-                         getter_AddRefs(defaults));
...
>+  nsCOMPtr<nsIProperties> dirSvc =
>+    do_GetService(NS_DIRECTORY_SERVICE_CONTRACTID, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIFile> defaultsDir;
>+  rv = dirSvc->Get(NS_APP_DEFAULTS_50_DIR,
>+                   NS_GET_IID(nsIFile),
>+                   getter_AddRefs(defaultsDir));
Actually, now that I see it, does it work to use NS_GetSpecialDirectory(NS_APP_DEFAULTS_50_DIR, here?

>+  nsCOMPtr<nsIFile> profileDir;
>+  rv = defaultsDir->Clone(getter_AddRefs(profileDir));
Now that we're not caching the dir we don't need to clone it.

>+  rv = profileDir->AppendNative(NS_LITERAL_CSTRING("profile"));
>+  NS_ENSURE_SUCCESS(rv, rv);
This is now so simple that I don't think it needs to be its own function.
Ratty, I put the suggestions from Neil in and tested it. Seems to work ok and a lot shorter.
Attachment #8719331 - Flags: feedback?(philip.chee)
Comment on attachment 8719331 [details] [diff] [review]
Patch v4 Rattys minimal patch including suggestions from Neil

> +  nsresult rv;
I would move this closer to where it is used.
Attachment #8719331 - Flags: feedback?(philip.chee) → feedback+
Attachment #8709112 - Attachment is obsolete: true
Attachment #8709112 - Flags: review?(neil)
Assignee: philip.chee → frgrahl
Attachment #8719331 - Flags: review?(neil)
Comment on attachment 8719331 [details] [diff] [review]
Patch v4 Rattys minimal patch including suggestions from Neil


>+  nsresult rv;

>-  defaults->AppendNative(aLeafName);
>+  rv = defaultsDir->AppendNative(NS_LITERAL_CSTRING("profile"));
Can this just be:
nsresult rv = defaultsDir->AppendNative(NS_LITERAL_CSTRING("profile"));

rs=me, if Neil needs further changes then they can land later.

a=me to land on c-a assuming no bustage on c-c
Attachment #8719331 - Flags: review+
Attachment #8719331 - Flags: approval-comm-aurora+
What's the next step?
Flags: needinfo?(frgrahl)
works for me, FWTW.
>> What's the next step?

I want to check out you suggestion tomorrow and see if the nsresult can be put on the same line. I think yes but didn't have time to check this week. If we want it in fast I can also set checkin needed. Patch works as is. 

FRG
Flags: needinfo?(frgrahl)
http://hg.mozilla.org/comm-central/rev/afa056cc4fd9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.44
You need to log in before you can comment on or make changes to this bug.