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

Status: RESOLVED FIXED

(SeaMonkey :: Startup & Profiles, defect)

Comment 1

[Philip Chee]

Attachment: Patch v1 Restore keys and fix compile errors

Fixes build failures locally.

Comment 2

[Philip Chee]

Comment on attachment 8706193 [details] [diff] [review]
Patch v1 Restore keys and fix compile errors

Whoops major braino.

Comment 3

[Philip Chee]

Attachment: Patch v1.1 Restore keys and fix compile errors

This one compiles and *seems* to work. However I'm not a C++ person at all.

Comment 4

[[e-mail]]

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.)

Comment 5

[Frank-Rainer Grahl (:frg)]

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
<<

Comment 6

[Frank-Rainer Grahl (:frg)]

Bug 1235099

Add new bug or patch it here too?

FRG

Comment 7

[Frank-Rainer Grahl (:frg)]

Apparently only

#define NS_APP_BOOKMARKS_50_FILE                "BMarks"

is needed. Will check

FRG

Comment 8

[Frank-Rainer Grahl (: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

Comment 9

[Philip Chee]

(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?

Comment 10

[Hartmut Figge]

(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. ;)

Comment 11

[Frank-Rainer Grahl (:frg)]

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

Comment 12

[Frank-Rainer Grahl (:frg)]

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

FRG

Comment 13

[Frank-Rainer Grahl (:frg)]

Attachment: 1238428-NS_APP_PROFILE_DEFAULTS-V2.patch

This one seems to do the trick. Please check. I think the process dirs were the wrong ones.

Comment 14

[Frank-Rainer Grahl (:frg)]

Just built a Windows installer and this one works too. Old profile still works and creation of a new one also succeeds.

Comment 15

[Hartmut Figge]

1238428-NS_APP_PROFILE_DEFAULTS-V2.patch works for SM-Trunk Linux x86_64.

Comment 16

[[e-mail]]

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.

Comment 17

[[e-mail]]

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.

Comment 18

[Philip Chee]

Attachment: Patch v3 minimal patch plus improvements by frg

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.

Comment 19

[[e-mail]]

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.

Comment 20

[Frank-Rainer Grahl (:frg)]

Attachment: Patch v4 Rattys minimal patch including suggestions from Neil

Ratty, I put the suggestions from Neil in and tested it. Seems to work ok and a lot shorter.

Comment 21

[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.

Comment 22

[Ian Neal]

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

Comment 23

[Ian Neal]

What's the next step?

Comment 24

[Cyrus Harmon]

works for me, FWTW.

Comment 25

[Frank-Rainer Grahl (:frg)]

>> 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

Comment 26

[Philip Chee]

http://hg.mozilla.org/comm-central/rev/afa056cc4fd9

Comment 27

[Philip Chee]

http://hg.mozilla.org/releases/comm-aurora/rev/bffa07722805