Default profile files are not copied to new profiles in omni-jar builds (the shipped builds)

RESOLVED WONTFIX

Status

()

defect
RESOLVED WONTFIX
7 years ago
2 years ago

People

(Reporter: mano, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Please see bug 738263 comment 15 and below.

Unlike my homemade (non-omni.ja) builds, it seems that for quite a while now the files in resource:///defaults/profile/ are not copied to new profiles. There's not much there these days: (*) the default bookmarks.html file (which is used by the resource uri, making this copy unnecessary); (*) an empty localstore file; (*) an empty mimetypes file; (*) chrome/ folder with those userChrome/Content-example.css files in there.

So, whilst this is very likely a regression (a long-standing one), it reveals that unnecessary work was done in the past and that the builds developers use for testing can hide some bugs (which is what happened in bug 738263).

So:
1) What's causing this?
2) Can userChrome/Content-css go away?
my supposition is that we just try to copy from the usual system path, instead of considering that the files may be in the jar, and swallow the error.
Blocks: 631271
OS: Mac OS X → All
Hardware: x86 → All

Comment 2

7 years ago
I can't find the bug I filed about this in the past, but I want to just get rid of defaults/profile entirely. At the time we tried to do that it broke bookmarks, but if that's working we should just rip out that code altogether.
If it's low-priority, I can start working on it while waiting for reviews.

So, if I understand correctly, this is the task list:
1/ list the files in defaults/profile;
2/ make sure that we do not need the files;
3/ get rid of the code that copies them on first start-up;
4/ get rid of the build-time code that puts them there in the first place.

With the exception of bookmarks.html that needs to be in defaults/profile and is not copied.

Comment 4

7 years ago
Yes precisely!
I'm somewhat confused. Would resource:///defaults/profile/bookmarks.html still work with this change?
(In reply to Mano from comment #5)
> I'm somewhat confused. Would resource:///defaults/profile/bookmarks.html
> still work with this change?

Afaict, resource:///defaults/profile/bookmarks.html would be the only file untouched by this change, so yes, it would still work.
Short summary of my findings so far.

The files I find in this directory are:
- bookmarks.html (used directly);
- localStore.rdf aka NS_APP_LOCALSTORE_50_FILE aka "LclSt" (looks copied to preferences by nsXREDireProvider::GetFile and nsProfileDirServiceProvider::GetFile);
- mimeTypes.rdf aka NS_APP_USER_MIMETYPES_50_FILE aka "UMimTyp"
(looks copied to preferences by nsXREDireProvider::GetFile and nsProfileDirServiceProvider::GetFile);
- prefs.js (I haven't found any reference to that file in runtime code);
- chrome/userChrome-example.css  (I haven't found any reference to that file in runtime code);
- chrome/userContent-example.css  (I haven't found any reference to that file in runtime code).

Also, the following files do not seem to be present in the directory, but seem to be expected nevertheless:
- panels.rdf aka NS_APP_USER_PANELS_50_FILE aka 
"UPnls" (looks copied to preferences by nsProfileDirServiceProvider::GetFile);
- search.rdf aka NS_APP_SEARCH_50_FILE aka "SrchF" (looks copied to preferences by nsProfileDirServiceProvider::GetFile).

Investigating further.
> - prefs.js (I haven't found any reference to that file in runtime code);

Does this one count? 
https://mxr.mozilla.org/mozilla-central/source/browser/components/migration/content/migration.js#404

It can go away *safely and easily* (and it should, regardless of this bug).
> - chrome/userChrome-example.css  (I haven't found any reference to that file in runtime 
> code);
> - chrome/userContent-example.css  (I haven't found any reference to that file in runtime 
> code).

Sure you didn't :) These are "example" files (features-discoverability for the geek). I'm pretty sure though that no one would miss them.
Good spot, I had missed that source. If I understand correctly, it creates or overwrites defaults/profile/prefs.js - where nobody will ever read it.

If so, yes, it should go away.
Ok, I have experimented with removing the code that copies the files in nsXREDirProvider and nsProfileDirServiceProvider and this does not seem to alter the outcome of any test. I will proceed with ripping off all of this.
Here we go for step 3.
Assignee: nobody → dteller
Attachment #637842 - Flags: review?(mak77)
Attachment #637842 - Flags: review?(benjamin)
Component: Build Config → Startup and Profile System
Product: Firefox → Toolkit
QA Contact: build.config → startup
Tests are not fully complete yet, but so far, they seem to indicate that everything is fine when we remove the files from the build system:

https://tbpl.mozilla.org/?tree=Try&rev=2bc3e81b3ffe
Attachment #637907 - Flags: review?(mak77)
Could you offer a few comments on the changes to removed-files.in within the step 4 patch? Normally when we stop shipping files we add files to this list.
Sounds like I just misunderstood removed-files.in.
Attachment #637907 - Attachment is obsolete: true
Attachment #637907 - Flags: review?(mak77)
Attachment #638191 - Flags: feedback?(nrthomas)
Attachment #638191 - Flags: feedback?(mak77)
Attachment #638191 - Flags: feedback?(benjamin)
Comment on attachment 638191 [details] [diff] [review]
Step 4: get rid of the files and their build-time copy

Let me see if I have this straight:
* MOZ_OMNIJAR builds put these files in omni.ja, but we're removing that in this bug. They're already in removed-files.in within an #ifdef MOZ_ONMIJAR, so we don't need any change there
* non-MOZ_OMNIJAR builds will stop populating dist/bin with the files, but if anyone generates updates from those they won't be removed

AFAICT we build Firefox, Thunderbird, XULRunner, Seamonkey (via their confvars.sh), b2g and android (via configure.in) with MOZ_CHROME_FILE_FORMAT=omni by default, so the second case isn't a big deal for Mozilla builds but might miss third parties. For completeness you could make the lines in removed-files.in unconditional.
Attachment #638191 - Flags: feedback?(nrthomas) → feedback+
Thanks for the feedback. This should address your suggestion.
Attachment #638191 - Attachment is obsolete: true
Attachment #638191 - Flags: feedback?(mak77)
Attachment #638191 - Flags: feedback?(benjamin)
Attachment #638334 - Flags: review?(mak77)
Attachment #638334 - Flags: review?(benjamin)
Comment on attachment 637842 [details] [diff] [review]
Step 3: get rid of the code that copies them on first start-up

Hrm, this is not actually what I meant. In most cases I thought that EnsureProfileFileExists was the *correct* way of ensuring that a file was set up correctly in the profile.

The incorrect way was the profile creation code at http://hg.mozilla.org/mozilla-central/annotate/098fbc184428/toolkit/profile/nsToolkitProfileService.cpp#l794

So I don't actually know whether it's safe to do this, but it wasn't what I was expecting :-(
It seems to pass all tests, at least.
I will take a look at the profile creation code, too.
Attachment 295771 [details] [diff] on bug 381365 is where we did this originally, and there were startup regressions? In any case, I think that before I can consider these patches we need to take that one; otherwise we'll be masking regressions like the one originally dealt with in bug 381365 and 411353.
Comment on attachment 637842 [details] [diff] [review]
Step 3: get rid of the code that copies them on first start-up

Clearing review requests, we can't take this *now* without dealing with the other issue first.
Attachment #637842 - Flags: review?(benjamin)

Updated

7 years ago
Attachment #638334 - Flags: review?(benjamin)
Is this what you had in mind?
(Untested yet)
Attachment #643413 - Flags: feedback?(benjamin)
Comment on attachment 643413 [details] [diff] [review]
Step 3, v2: get rid of the code that copies them on first start-up

Yes. I think that needs to land and probably ride a train before we do the other things. It caused regressions the first time!
Attachment #643413 - Flags: feedback?(benjamin) → feedback+
I'm not going to be able to go to this review soon, I suggest finding an alternative reviewer, maybe Mano or Neil? If still here I'll look at it when I'm back.
I can review this on Saturday (along with the rest of my queue)  if that works for you.
Looks like David is away till first week of Aug, and this is not high priority so there should be no hurry.
Attachment #637842 - Flags: review?(mak77) → review?(mano)
Attachment #638334 - Flags: review?(mak77) → review?(mano)
flipped the requests, thanks
Comment on attachment 637842 [details] [diff] [review]
Step 3: get rid of the code that copies them on first start-up

So, all in all, I'm not sure we want to do this... Please talk to Benjamin and Neil Deakin after reading the comments below.

>diff --git a/profile/dirserviceprovider/src/nsProfileDirServiceProvider.cpp b/profile/dirserviceprovider/src/nsProfileDirServiceProvider.cpp
>--- a/profile/dirserviceprovider/src/nsProfileDirServiceProvider.cpp
>+++ b/profile/dirserviceprovider/src/nsProfileDirServiceProvider.cpp
>@@ -179,64 +179,53 @@ nsProfileDirServiceProvider::GetFile(con
>     rv = domainDir->Clone(getter_AddRefs(localFile));
>     if (NS_SUCCEEDED(rv))
>       rv = localFile->AppendNative(USER_CHROME_DIR_50_NAME);
>   }
>   else if (strcmp(prop, NS_APP_LOCALSTORE_50_FILE) == 0) {
>     rv = domainDir->Clone(getter_AddRefs(localFile));
>     if (NS_SUCCEEDED(rv)) {
>       rv = localFile->AppendNative(LOCAL_STORE_FILE_50_NAME);
>-      if (NS_SUCCEEDED(rv)) {
>-        // it's OK if we can't copy the file... it will be created
>-        // by client code.
>-        (void) EnsureProfileFileExists(localFile, domainDir);
>-      }

In theory, this could affect performance, depending on how slow the "interactive" creation is. Neil Deakin probably knows best how slow/fast is the rdf code.

>     }
>   }
>   else if (strcmp(prop, NS_APP_USER_PANELS_50_FILE) == 0) {
>     rv = domainDir->Clone(getter_AddRefs(localFile));
>     if (NS_SUCCEEDED(rv)) {
>       rv = localFile->AppendNative(PANELS_FILE_50_NAME);
>-      if (NS_SUCCEEDED(rv))
>-        rv = EnsureProfileFileExists(localFile, domainDir);

I talked with Neil Rushbrook. SeaMonkey relies on this file being in place:
https://mxr.mozilla.org/seamonkey/source/suite/common/sidebar/nsSidebar.js#415

I told him, that for now, I'll ask you to leave this as is, but if you insist, make sure you file a SeaMonkey bug ahead of landing this.

>     }
>   }
>   else if (strcmp(prop, NS_APP_USER_MIMETYPES_50_FILE) == 0) {
>     rv = domainDir->Clone(getter_AddRefs(localFile));
>     if (NS_SUCCEEDED(rv)) {
>       rv = localFile->AppendNative(MIME_TYPES_FILE_50_NAME);
>-      if (NS_SUCCEEDED(rv))
>-        rv = EnsureProfileFileExists(localFile, domainDir);

I think you might be breaking handlerApps.js here. It doesn't seem to check if the file exists.

By the way, Alex Limi told me a while ago that default handling for some files types should be added back at some point. In the past, we used this the default mimetypes file for implementing that. We don't have to implement it the same way again, but you should take it into account.

>     }
>   }
>   else if (strcmp(prop, NS_APP_BOOKMARKS_50_FILE) == 0) {
>     rv = domainDir->Clone(getter_AddRefs(localFile));
>     if (NS_SUCCEEDED(rv))
>       rv = localFile->AppendNative(BOOKMARKS_FILE_50_NAME);
>   }

This one actually needs to go away at some point.. Please file a follow up.

>   else if (strcmp(prop, NS_APP_SEARCH_50_FILE) == 0) {
>     rv = domainDir->Clone(getter_AddRefs(localFile));
>     if (NS_SUCCEEDED(rv)) {
>       rv = localFile->AppendNative(SEARCH_FILE_50_NAME);
>-      if (NS_SUCCEEDED(rv))
>-        rv = EnsureProfileFileExists(localFile, domainDir);
>     }
>   }

search.rdf isn't used anymore, not even by seamonkey. Let's remove this one.


>   else if (strcmp(prop, NS_APP_DOWNLOADS_50_FILE) == 0) {
>     rv = domainDir->Clone(getter_AddRefs(localFile));
>     if (NS_SUCCEEDED(rv))
>       rv = localFile->AppendNative(DOWNLOADS_FILE_50_NAME);
>   }

The only reference to this one is in Thunderbird, which attempts to remove the file if it's still there. I think that code can also away, and we can remove this definition as well.

It can be done in a follow up though.


>diff --git a/toolkit/xre/nsXREDirProvider.cpp b/toolkit/xre/nsXREDirProvider.cpp
>--- a/toolkit/xre/nsXREDirProvider.cpp
>+++ b/toolkit/xre/nsXREDirProvider.cpp
>@@ -373,23 +373,21 @@ nsXREDirProvider::GetFile(const char* aP
>     }
>     else if (!strcmp(aProperty, NS_APP_LOCALSTORE_50_FILE)) {
>       if (gSafeMode) {
>         rv = file->AppendNative(NS_LITERAL_CSTRING("localstore-safe.rdf"));
>         file->Remove(false);
>       }
>       else {
>         rv = file->AppendNative(NS_LITERAL_CSTRING("localstore.rdf"));
>-        EnsureProfileFileExists(file);
...

>         ensureFilePermissions = true;
>       }
>     }
>     else if (!strcmp(aProperty, NS_APP_USER_MIMETYPES_50_FILE)) {
>       rv = file->AppendNative(NS_LITERAL_CSTRING("mimeTypes.rdf"));
>-      EnsureProfileFileExists(file);
>       ensureFilePermissions = true;
>     }

Same as my first comment for the other implementation.
Attachment #637842 - Flags: review?(mano) → review-
Comment on attachment 638334 [details] [diff] [review]
Step 4: get rid of the files and their build-time copy

Looks ok, of course, but only feedback+ed because it can land as is.
Attachment #638334 - Flags: review?(mano) → feedback+
No time to work on this.
Assignee: dteller → nobody
We are intentionally not supporting this any more. Woot.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.