48155 - chrome RDF files should not contain absolute paths

Status: VERIFIED FIXED

(SeaMonkey :: Build Config, defect, P1)

Description

[Andrew Thompson]

M17 release build (2000080712), also Netscape PR2 but I reported that through the 
Netscape form.

Install M17 on a Mac using the installer. It creates a folder called Mozilla 
Folder.

(If you like, delete Mozilla Registry and your user profile just to be sure, but 
this isn't really needed)

Launch Mozilla.
Quit it.

Rename "Mozilla Folder" to "Mozilla M17"

Launch Mozilla. 

A big white fullscreen window appears with no content and no menubar.
Mozilla freezes and must be force-quit, often taking the whole system down with 
it.

Deleting your Moz Registry, Component Registry and profile won't help.
Rebooting won't help
Only renaming the folder back to "Mozilla Folder" works.

**The same problem occurs if you move the folder elsewhere on the same disk**

This may be acceptable (though extremely poor) on Windows and Linux, but its 
utterly innapproriate on MacOS where users can and do rename applications and the 
folders that they are in whenever they need/want to with no ill effects.

This is going to be so non-obvious to an average Mac user (I only realised what 
was up in a flash of inspiration while out for a walk!) that even if they 
reinstall Mozilla/Netscape 6 they're likely to rename or move the folder again, 
and still not make the connection.

At that point Moz/Netscape will be put in the Trash...

Comment 1

[Joel Reed]

I confirmed this bug using the installer build M17 (2000080712) on MacOS 9.04.
The installer version does infact lauch a screen size blank browser window, if
"Mozilla Folder" has been renamed or moved. I also duplicated this same behavior
in nightly M18 build (2000080808) installer version. Trashing profiles or the
Moz Registry does not correct the issue for either build. Renaming the folder to
its original name was the only solution. I also tested the M17 realease build
(2000080712) *sea.bin version and moving or renaming its folder did not produce
the same behavior. So not only is it confirmed I am going to reassign it to the
Installer component. Thanks.

Comment 2

[Asa Dotzler [:asa]]

setting default owner. Updating summary.  Old summary was
M17 crashes if you rename or move its folder.

Comment 3

[Blake Ross]

Adding crash keyword to all open crash bugs that don't already have it...

Comment 4

[Andrew Thompson]

Nominating for beta3 fix. Could bounce to RTM but no further.
Actually I think this is nsmac1 too!

Comment 5

[Sean Su]

reassigning to Samir.

Comment 6

[Samir Gehani]

This is a packaging issue, not a bug in the installer itself.  Reassigning to 
Build - Config component.

Comment 7

[Jon Granrose]

I doubt there's anything we can do in build config, but reassigning to jj for 
his comments.  This is most likely a core browser problem.

Comment 8

[J.J. Enser]

glad to be added to this bug, as I was discussing the exact same issue with 
sfraser and danm a couple of days ago.
I experienced that very same behavior with PR2 on my own machine, and did some 
more research.

interesting facts:
- happens with bits resulting from the installer, NOT with bits resulting from 
the self-extracting app (mozilla-M17.sea nor Netscape6-mac.sea)
- happens even after deleting all profiles + various registry files.
- could reproduce with both mozilla & netscape builds from today (08-16-12)  
- full access paths to all COOL components are hard-coded in ":Essential Files 
:xpcs Registry.dat" on first run. However, deleting this file doesn't make the 
problem go away. Also, this file is a "netscape only" file.

Renaming _any_ of the directories in the access path to mozilla/netscape causes 
the symptom to appear.

It appears clear to me that the access path to the app must be stored somewhere, 
most likely by the installer since the problem doesn't occur with the sea.

Who wants to take ownership?

Comment 9

[J.J. Enser]

more data:
various absolute access paths can actually also be found in:
	. chrome/all-locales.rdf
	. chrome/all-oackages.rdf
	. chrome/all-skins.rdf

none of these files are part of the packaging process.
Removing these files after they got created causes the app to feeze on startup.

builds extracted from .sea file don't have these 3 rdf files under chrome and 
live happy without them. These are the builds that can be moved or renamed.

We're getting close!

Comment 10

[J.J. Enser]

Bug triage. this is not a release issue, as stated above. Absolute access paths 
in .rdf files must be avoided.
This bug needs a new owner. danm, sfraser, who should take this one up?

Comment 11

[Simon Fraser [no longer active]]

This is a bug for hyatt, I think.

Comment 12

[Peter Trudelle]

->dveditz to ensure that installer writes resource URLs rather than file paths
into installed-chrome.txt.

Comment 13

[J.J. Enser]

It appears to me that files like all-locales.rdf, all-packages.rdf, and all-
skins.rdf are generated on the first run of the app rather than by the installer. 
However, I'm not 100% sure since the installer fires up the app at the end of 
installation.

just another 2¢ to the pot.

Comment 14

[Daniel Veditz [:dveditz]]

Yes, the rdf files *are* created on first run of the app, but using data from 
installed-chrome.txt which is created by the installer. Mea Culpa, I never 
switched from paths to URLs when hyatt added that capability.

Comment 15

[Daniel Veditz [:dveditz]]

Oh, and this most definitely needs to be nsbeta3+ (he says, further dooming 
himself)

Comment 16

[Daniel Veditz [:dveditz]]

Setting p1 for critical nsbeta3+ bugs

Comment 17

[leger]

PDT agrees P1

Comment 18

[leger]

Putting [pdtp1] in whiteboard

Comment 19

[hamerly]

Would not hold PR3 for this...

Comment 20

[Simon Fraser [no longer active]]

Blocker. This hoses the installers, since installed-chrome.txt files contain full 
paths:

content,install,url,jar:file:///Development/Installers/Netscape Folder/chrome/
toolkit.jar!/content/global/
content,install,url,jar:file:///Development/Installers/Netscape Folder/chrome/
comm.jar!/content/communicator/
content,install,url,jar:file:///Development/Installers/Netscape Folder/chrome/
comm.jar!/content/editor/
content,install,url,jar:file:///Development/Installers/Netscape Folder/chrome/
comm.jar!/content/navigator/
skin,install,url,jar:file:///Development/Installers/Netscape Folder/chrome/
modern.jar!/skin/modern/communicator/
skin,install,url,jar:file:///Development/Installers/Netscape Folder/chrome/
modern.jar!/skin/modern/editor/
skin,install,url,jar:file:///Development/Installers/Netscape Folder/chrome/
modern.jar!/skin/modern/global/
skin,install,url,jar:file:///Development/Installers/Netscape Folder/chrome/
modern.jar!/skin/modern/messenger/
skin,install,url,jar:file:///Development/Installers/Netscape Folder/chrome/
modern.jar!/skin/modern/navigator/
skin,install,url,jar:file:///Development/Installers/Netscape Folder/chrome/
modern.jar!/skin/modern/aim/
skin,install,url,jar:file:///Development/Installers/Netscape Folder/chrome/
classic.jar!/skin/classic/communicator/
skin,install,url,jar:file:///Development/Installers/Netscape Folder/chrome/
classic.jar!/skin/classic/editor/
skin,install,url,jar:file:///Development/Installers/Netscape Folder/chrome/
classic.jar!/skin/classic/global/
skin,install,url,jar:file:///Development/Installers/Netscape Folder/chrome/
classic.jar!/skin/classic/messenger/
skin,install,url,jar:file:///Development/Installers/Netscape Folder/chrome/
classic.jar!/skin/classic/navigator/
skin,install,url,jar:file:///Development/Installers/Netscape Folder/chrome/
classic.jar!/skin/classic/aim/
content,install,url,jar:file:///Development/Installers/Netscape Folder/chrome/
messenger.jar!/content/messenger/
locale,install,url,jar:file:///Development/Installers/Netscape Folder/chrome/en-
US.jar!/locale/en-US/global/
locale,install,url,jar:file:///Development/Installers/Netscape Folder/chrome/en-
US.jar!/locale/en-US/communicator/
locale,install,url,jar:file:///Development/Installers/Netscape Folder/chrome/en-
US.jar!/locale/en-US/messenger/
locale,install,url,jar:file:///Development/Installers/Netscape Folder/chrome/en-
US.jar!/locale/en-US/editor/
locale,install,url,jar:file:///Development/Installers/Netscape Folder/chrome/en-
US.jar!/locale/en-US/navigator/
locale,install,url,jar:file:///Development/Installers/Netscape Folder/chrome/en-
US.jar!/locale/en-US/aim/

Comment 21

[Jon Granrose]

adding twalker and myself to cc.  removing smoketest keyword since it doesn't
appear this has impacted this morning's smoketesting.

Comment 22

[Mike Pinkerton (not reading bugmail)]

I just ran into this, very very annoying.

we should hold pr3 for this. most mac users will try to rename this folder after
installs (heck, i did) and the build will magically fail. I disagree with hammerly.

Comment 23

[Simon Fraser [no longer active]]

The full paths in installed-chrome.txt are actually covered by bug 40626. But the 
more serious problem is covered by this bug.

Comment 24

[Andrew Thompson]

Yeah, I agree with Pinkerton. It was blind luck that I spotted what this is, its 
so subtle.

Everyone's going to change the name because the name is "Netscape Folder" or 
"Mozilla Folder" (hmmm, maybe I'll file a bug on this!) which is so blindingly 
undescriptive...

Comment 25

[Daniel Veditz [:dveditz]]

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

Comment 26

[Daniel Veditz [:dveditz]]

If we're going to let people crash in beta 3 we should at least fix for rtm

Comment 27

[Phil Peterson]

I wouldn't *hold* PR3 for this bug, but if you came to PDT with a super-safe fix
on Wed or Thu morning, we might be able to get it into PR3. Marking nsbeta3-

Comment 28

[Mike Pinkerton (not reading bugmail)]

people _are_ going to rename the folder the app came in, then magically the app
won't start, and they won't know why. I wasted days trying to figure out why my
builds wouldn't work since this is such a normal thing to do.

if nothing else, we MUST relnote this, but i strongly suggest we take a fix no
matter how dangerous. requesting re-triage.

Comment 29

[Mike Pinkerton (not reading bugmail)]

I don't understand how we're willing to hold PR3 for a bug like "Net2Phone shows
up in toolbar on mac" but we're not willing to hold PR3 for a bug that renders
the app totally useless.

Dan? Hyatt says this is pretty easy to fix. I'll buy you a 6pack of your
favorite beer if you can get this in by Thursday ;)

Comment 30

[Mike Pinkerton (not reading bugmail)]

note that this also happens if you move the folder the app is in to another
location on your drive -- something Mac users will do for sure and are used to
doing.

Comment 31

[Simon Fraser [no longer active]]

I'm sure this is a pretty trivial fix. dveditz, please comment.

Comment 32

[Dan M]

The installer builds its list of installed chrome using absolute paths like
  jar:file://d|/builds/m18/chrome/toolkit.jar!/content/global/
the build system builds the same file using relative paths like
  jar:resource:/chrome/toolkit.jar!/content/global/
It's a matter of teaching the installer to use the same format as the build 
system. I can't speak for how difficult this would be to code, but it would 
certainly be a safe change, since anyone who's ever built Mozilla has 
already been testing it the relative way.

Comment 33

[Daniel Veditz [:dveditz]]

Attachment: patch to fix bug

Comment 34

[Daniel Veditz [:dveditz]]

Attachment: revised patch

Comment 35

[Daniel Veditz [:dveditz]]

Ignore the first patch, it was contaminated with part of a different fix

danm writes: "I can't speak for how difficult this would be to code, but it 
would certainly be a safe change, since anyone who's ever built Mozilla has 
already been testing it the relative way."

The risk is not in the well-tested installed-chrome.txt format, but in the code 
changes required to get it that way. It sounds trivial in principle but there 
are at least four cases to test, and since we use platform-specific nsLocalFile 
routines that's multiplied by our three platforms.

The four cases to test come about because resource: URLs only work for chrome 
installed in the chrome directory. Chrome installed elsewhere (e.g. in the 
profile) must still use file: URLs. Then there are some fundamental differences 
when this code is run in the browser environment vs. the limited initial 
install environment.

I have tested three of the cases only on windows: chrome in the chrome 
directory (i.e. resource: URLs) in both a standard XPInstall and an initial 
install, and profile chrome in XPInstall.

I need platform buddies to try this patch out on Mac and Linux.

Comment 36

[Daniel Veditz [:dveditz]]

Since no one's tried this patch on Mac and it's after the daily PDT meeting I'm 
assuming there's no chance in hell this is going into tomorrow's build. 
nsbeta3-

Comment 37

[Phil Peterson]

PDT marking [rtm need info] until code reviews are available.

Comment 38

[Mike Pinkerton (not reading bugmail)]

how does the installer work when you build it? does it just pull down the xpi
files from sweetlou? how are they tagged so we're sure to get the right ones?

just asking about how i'd verify that this worked with my build...

...btw, dan, what kind of beer do you like? I owe you a 6-pack. ;)

Comment 39

[Daniel Veditz [:dveditz]]

r=sgehani

Comment 40

[Samir Gehani]

Dan,
Don't you need to change this to [rtm+] now that it's been reviewed so that it
is detected on pdt's radar?

Comment 41

[Phil Peterson]

PDT marking [rtm++]

Comment 42

[Daniel Veditz [:dveditz]]

Attachment: Revised patch incorporating feedback from scc

Comment 43

[Simon Fraser [no longer active]]

There is some scarey : -> / conversion going on in this code. Are these changes 
safe on machines that have / in the folder path on Mac? How about non-ASCII in 
disk or folder names?

I'd like to hear that some very thorough testing has been done in such 
situations.

Comment 44

[Daniel Veditz [:dveditz]]

The only path-slash conversion I added is performed on relative path fragments
in the chrome tree. Yes, I suppose it may have problems with japanese-named
chrome jars, but developers will figure that out before they release and we can
fix that problem at leisure later (it's not an easy problem, this code is
exercised by the native install wizards without the i18n converters present).

It won't be a problem with the user-chosen install directory because that's
already stripped off, and it won't be a problem with any of the chrome .jars we
ship.

In the rare case someone installs outside the chrome directory, and does so
during the initial install (extremely unlikely -- why would *we* do that?) then
the hack conversion warren added earlier (i.e. not part of this patch, and not
fixed by this patch) may have problems with multi-byte charsets. Warren has
tasked Andreas to move the necko file URL conversions into nsIFile where they
can be accessed by everyone to get the correct stuff so this hack will be
removed in the future.

Comment 45

[sairuh (rarely reading bugmail)]

i believe i also see this on linux. i had stored my previous builds in
directories under /u/sairuh/seamonkey/<build_id> --since i recently ran into
disk quota problems, i decided to move the most recent builds to a local disk
(/export/builds/<build_id>). and when i tried to launch one of those moved
builds, i got the max'd out window (titlebar only, empty content and chrome).

marking all. but do let me know if i'm seeing a different bug.

Comment 46

[Daniel Veditz [:dveditz]]

Attachment: completely redone patch to fix potential slash conversion problems

Comment 47

[Daniel Veditz [:dveditz]]

Reworked patch to address potential slash conversion problems raised by sfraser
and scc. They would be real corner cases and wouldn't come up in our own
installs of Mozilla, but this is better.

The code now always uses Necko (or the temporary local hack copied from Necko)
to convert paths into file:// URLs. in the resource: case we then take a
trailing substring of the URL rather than try to convert path slashes ourselves.

Seeking new review and sr= from scc

Comment 48

[Scott Collins]

I like this patch.  A couple small details, though:

  - prefer |nsCOMPtr<nsIFoo> fooService(do_GetService(fooCID, &rv));| over
      the |NS_WITH_SERVICE| macro

  - |sizeof| a string literal is the length of the string + 1 for the zero
      terminator C/C++ has thoughtfully appended to the literal for you,
      you should remember this in your calculation for padding, and note
      in a comment that you mean to include it, or that it doesn't hurt, or
      else have code to subtract

  - I really like that you say |sizeof('!')| rather than adding 1 ... that
      makes your intention clear without a comment; nice!  Saying |'!'| says
      to me that you knew about the padding you didn't want from |"!"|.

  - prefer |nsCOMPtr<nsIFoo> foo( do_QueryInterface(bar) );| over the
      copy-initialization form (that is, using |=|), on a good compiler the
      latter form will be optimized into a direct initialization, on a bad
      compiler, a temporary is made and the copy constructor is called

Good work, and thanks for going to the extra trouble to handle the naming edge
cases.  I'd be happiest if you fixed the three minor negative points I mentioned
above, but if this code has been reasonably tested, I have no problem with it
going in as is.

sr=scc.

Comment 49

[Daniel Veditz [:dveditz]]

Thanks Scott, I fixed the three things you mentioned and will check it in. I did
know that sizeof("") counts the null, but thought the code was more readable if
I didn't try to compensate.

Comment 50

[Daniel Veditz [:dveditz]]

Checked into branch and trunk

Comment 51

[Jon Granrose]

jj, can you verify this since I don't have a mac?

Comment 52

[Jon Granrose]

I don't have a mac to verify this against, but I see the patch checked into the
tree.  marking verified.

Comment 53

[Simon Fraser [no longer active]]

Unverifying, since this was not verified by the normal channels.

Comment 54

[Simon Fraser [no longer active]]

Fixed, bug unverified. jrgm, can you please do some testing here, check 
installations on machines with / in folder names, and all that jazz? Thanks.

Comment 55

[J.J. Enser]

fyi, using yesterday's full installer, 'Netscape Folder' could be moved or 
renamed without affecting the product's behavior.

Comment 56

[John Morrison]

Okey-dokey, verified in the branch 2000103008 mac os9.0. Marking vtrunk. 

I installed into 'foo:bar/test:ÒMy §Œ¶ÄÓ' (just for extra fun), and could 
install, start and restart. I variously moved the folder around, or renamed, 
and on each restart I could browse pages, send/receive mail, ftp, prefs, 
view-source, create/select profiles ... and, of course, the 
installed-chrome.txt contained only jar:resource: urls.

(removing relnote keyword).

Comment 57

[Asa Dotzler [:asa]]

Verified with 110606 mozilla trunk build on Mac OS 9.  I did a completely clean
install of mozilla.  After an initial launch and profile creation I rennamed the
Mozilla install directory and moved it's location from HD to Desktop.  I had no
problems using Mozilla, Mozilla Mail-News or Composer.  Setting bug status to
Verified and removing vtrunk keyword.