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

Status: RESOLVED WONTFIX

(Toolkit :: Startup and Profile System, defect)

Description

[Asaf Romano (gone)]

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?

Comment 1

[Marco Bonardo [:mak]]

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.

Comment 2

[Benjamin Smedberg]

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.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

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

[Benjamin Smedberg]

Yes precisely!

Comment 5

[Asaf Romano (gone)]

I'm somewhat confused. Would resource:///defaults/profile/bookmarks.html still work with this change?

Comment 6

[David Teller [:Yoric] - still alive but not very active]

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

Comment 7

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 8

[Asaf Romano (gone)]

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

Comment 9

[Asaf Romano (gone)]

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

Comment 10

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Attachment: Step 3: get rid of the code that copies them on first start-up

Here we go for step 3.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Attachment: Step 4: get rid of the files and their build-time copy

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

Comment 14

[Nick Thomas [:nthomas] (UTC+12)]

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.

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Sounds like I just misunderstood removed-files.in.

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Attachment: Step 4: get rid of the files and their build-time copy

Comment 17

[Nick Thomas [:nthomas] (UTC+12)]

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.

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Attachment: Step 4: get rid of the files and their build-time copy

Thanks for the feedback. This should address your suggestion.

Comment 19

[Benjamin Smedberg]

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

Comment 20

[David Teller [:Yoric] - still alive but not very active]

It seems to pass all tests, at least.
I will take a look at the profile creation code, too.

Comment 21

[Benjamin Smedberg]

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 22

[Benjamin Smedberg]

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.

Comment 23

[David Teller [:Yoric] - still alive but not very active]

Attachment: Step 3, v2: get rid of the code that copies them on first start-up

Is this what you had in mind?
(Untested yet)

Comment 24

[Benjamin Smedberg]

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!

Comment 25

[Marco Bonardo [:mak]]

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.

Comment 26

[Asaf Romano (gone)]

I can review this on Saturday (along with the rest of my queue)  if that works for you.

Comment 27

[Marco Bonardo [:mak]]

Looks like David is away till first week of Aug, and this is not high priority so there should be no hurry.

Comment 28

[Marco Bonardo [:mak]]

flipped the requests, thanks

Comment 29

[Asaf Romano (gone)]

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.

Comment 30

[Asaf Romano (gone)]

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.

Comment 31

[David Teller [:Yoric] - still alive but not very active]

No time to work on this.

Comment 32

[Benjamin Smedberg]

We are intentionally not supporting this any more. Woot.