57733 - Add-on packages don't work with Netscape 6

Status: RESOLVED FIXED

(Core :: XUL, defect, P2)

Description

[David Hyatt]

There is a bug in the chrome registry that prevents add-on packages (e.g.,
chatzilla) from installing properly.  The auto-skin selection code is broken.

The patch is one-line, and of enormous benefit, since otherwise, all add-ons
produced for Netscape 6 will be broken.

Comment 1

[David Hyatt]

Nominating for rtm.

Comment 2

[David Hyatt]

Attachment: Patch to fix problem.

Comment 3

[David Hyatt]

*** Bug 51187 has been marked as a duplicate of this bug. ***

Comment 4

[Robert Ginda]

tested patch on linux.  r=rginda

Comment 5

[Peter Trudelle]

rtm need info

Comment 6

[Brendan Eich [:brendan]]

Patch wasn't quite one line (could be smaller though: why not

+        PRBool useProfile = !mProfileRoot.IsEmpty();

rather than the longer, optimizer-needs-to-avoid-double-set-in-true-case:

+        PRBool useProfile = PR_FALSE;
+        if (!mProfileRoot.IsEmpty())
+          useProfile = PR_TRUE;

?)

What was the one-line part?  Both parts look important, but could you say a few 
more words (for the PDT as well as for me)?  I'm happy to give sr= soon.

/be

Comment 7

[Brendan Eich [:brendan]]

Also, we need QA coverage here, unless this was a very recent regression.  
What's our QA story?

/be

Comment 8

[David Hyatt]

The one-liner caused us to bail early while walking through a loop of installed 
skins.  The idea is that chatzilla has no skin selected yet.  It only supplies 
a skin for modern.  The code is walking the list of skins until it finds one 
that chatzilla supports.

In a situation where classic comes first (which is the way it is for 
everybody), I end up not checking modern, since I mistakenly bail because I 
checked the wrong thing (I checked the address rather than the value).

The other code, the useProfile thing, will prevent packages from being 
installed on Linux where the user isn't running as root.  I was trying to write 
to the install dir, when if the profile root is known, I can just write to thep 
rofile dir instead.

Comment 9

[Brendan Eich [:brendan]]

sr=brendan (a=, same thing I think).  I took the liberty of setting [rtm+].

/be

Comment 10

[selmer (gone)]

Back to need info.  We still don't see a one-line patch.

Comment 11

[Brendan Eich [:brendan]]

Hey, hyatt explained why the patch was more than one line.  His "one-line" was 
wrong.  You can't expect a one-line patch for this bug, given my understanding 
of the code problems being addressed.

/be

Comment 12

[Peter Trudelle]

Really, this is very simple - even I can understand it!  It could be written in
fewer lines, but that wouldn't make it simpler, just less clear.

Comment 13

[Brendan Eich [:brendan]]

Cc'ing selmer so he can react to our frustration.

/be

Comment 14

[selmer (gone)]

So, the smallest possible fix would be the change from
  if (aSelectedProvider)
to
  if (*aSelectedProvider)
and that would avoid the bailing out too early problem.  Is this not a correct
fix all by itself?

The rest of the patch seems to address a different bug that non-root users can't
install packages if root installed to an unwritable directory.  Would not fixing
this have any impact on the bailing out problem?  Will taking this part of the
fix cause users running as root to always install packages into their own
profile directory?  (There doesn't appear to be a way of choosing.)

Comment 15

[David Hyatt]

It's two one-liners (for a total of two lines).  I have made the adjustment
brendan suggested, so now the patch is two lines.

Note that depending on how you install on Linux, this can even cause the browser
to not start.  If you - this, there will definitely have to be a release note
entry about how lame we are at installation on shared networks.

Comment 16

[David Hyatt]

The bug is that chatzilla doesn't start.  Even if you fix the first problem,
then you get bitten by the second.  I could break it out if you want, but what
I'm showing you are two one-line patches that address the same problem.

Comment 17

[selmer (gone)]

We would like to see this have some focused testing about packages that work on 
mozilla vs N6 and install as root/non-root.  We spoke with Peter and he knows 
what is required.

Comment 18

[Peter Trudelle]

Hyatt plans to land this on the trunk this evening, and jrgm will take tonight's
verification builds and start testing those permutations.

Comment 19

[selmer (gone)]

Oops, forgot to mark need info until the testing is done.

Comment 20

[David Hyatt]

Checked in on trunk.

Comment 21

[John Morrison]

Ran the Netscape6 trunk installer on Linux/win/mac with successful installs,
on systems scrubbed of any prior profiles/registry info. Started mail,
composer, browser, addressbook, AIM, and their respective overlays were in
place.

Added 'button {border:1px solid red !important}' to
<profile>/chrome/userChrome.css and had that reflected in the UI. Deleted
*.rdf and overlayinfo, and saw that they were rebuilt. Touched
installed-chrome.txt and saw that chrome was reregistered (by
timestamps). Delete ./chrome from <profile> and had it rebuilt with
user-skins.rdf and user-locale.rdf.

Ran the Mozilla trunk installer on Linux/win/mac with successful installs, on
systems scrubbed of any prior profiles/registry info.

Mozilla builds, came up with Chatzilla fully functioning (AFAICT), with all
overlays in place.

So: that all looks perfectly fine.

One caveat though: if there is only one profile on the system (i.e., don't
need to run profilemanger), then user-*.rdf will not be rewritten into the
install directory.  However, if there are 2+ profiles (i.e., profilemanager
runs to select the profile), then user-*.rdf will be rewritten again into the
install directory. hyatt needs to ensure that this information is available
in either case.

Comment 22

[David Hyatt]

Attachment: New patch.

Comment 23

[David Hyatt]

Ok, this is ready to go.

Comment 24

[selmer (gone)]

This bug is in candidate limbo.  We will reconsider this fix once we have a 
candidate in hand, but we can't take this fix before then.  Please check into 
the trunk ASAP.

Comment 25

[selmer (gone)]

r= & sr= are done per Dave's comments at the meeting.

Comment 26

[selmer (gone)]

rtm++, please checkin ASAP so we can build today.

Comment 27

[David Hyatt]

Fixed.

Comment 28

[John Morrison]

I've verified the chatzilla aspect of this patch. I'm through most of the
other stuff, but have a couple of items left. (But, right now, I am going
home ...)

Comment 29

[John Morrison]

running through the items noted 10/25, mac/linux/win32 with 20001109nn MN6
candidate commercial builds, everything checks out, including the placement
of a backup user-skins.rdf/user-locale.rdf to enable profile selection.
Marking vtrunk.

Comment 30

[tao]

Folks: 

In working on 44070 (matching browser locale with system locale), I plan to 
change the ProfileCreation code to not auto select/set profile locale when
users do not explicitly choose one via locale selector. This would save us
a 'write' to disk and subsequent reads in startup.

Unfortunately, this patch results in a 'write' to disk in finding each
skin/locale providers (except gloal and communicator) and subsequent read to 
chrome.rdf in profile directory. Is this really necessary? Could we set a flag
such as mRuntimeProvider to avoid writing to global and profile directory. 
See my patch in 44070.

Comment 31

[selmer (gone)]

Adding Conrad and Bhuvan to answer your profile question.