chrome RDF files should not contain absolute paths

VERIFIED FIXED in M18

Status

P1
blocker
VERIFIED FIXED
18 years ago
14 years ago

People

(Reporter: lordpixel, Assigned: dveditz)

Tracking

({crash})

Trunk
crash

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nsbeta3-][pdtp1][rtm++])

Attachments

(4 attachments)

(Reporter)

Description

18 years ago
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

18 years ago
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.
Status: UNCONFIRMED → NEW
Component: Browser-General → Installer
Ever confirmed: true

Comment 2

18 years ago
setting default owner. Updating summary.  Old summary was
M17 crashes if you rename or move its folder.
Assignee: asa → ssu
QA Contact: doronr → gemal
Summary: M17 crashes if you rename or move its folder → mac installer mozilla crashes if you rename or move its folder

Comment 3

18 years ago
Adding crash keyword to all open crash bugs that don't already have it...
Keywords: crash
(Reporter)

Comment 4

18 years ago
Nominating for beta3 fix. Could bounce to RTM but no further.
Actually I think this is nsmac1 too!
Keywords: nsbeta3

Comment 5

18 years ago
reassigning to Samir.
Assignee: ssu → sgehani

Comment 6

18 years ago
This is a packaging issue, not a bug in the installer itself.  Reassigning to 
Build - Config component.
Assignee: sgehani → cls
Component: Installer → Build Config
QA Contact: gemal → granrose
Summary: mac installer mozilla crashes if you rename or move its folder → mozilla crashes if you rename or move its folder

Comment 7

18 years ago
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.
Assignee: cls → jj

Comment 8

18 years ago
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

18 years ago
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

18 years ago
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

18 years ago
This is a bug for hyatt, I think.
Assignee: jj → hyatt
Keywords: nsmac1
Summary: mozilla crashes if you rename or move its folder → chrome RDF files should not contain absolute paths

Comment 12

18 years ago
->dveditz to ensure that installer writes resource URLs rather than file paths
into installed-chrome.txt.
Assignee: hyatt → dveditz

Comment 13

18 years ago
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.
(Assignee)

Comment 14

18 years ago
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.
(Assignee)

Comment 15

18 years ago
Oh, and this most definitely needs to be nsbeta3+ (he says, further dooming 
himself)
Whiteboard: [nsbeta3+]
(Assignee)

Comment 16

18 years ago
Setting p1 for critical nsbeta3+ bugs
Priority: P3 → P1

Comment 17

18 years ago
PDT agrees P1

Comment 18

18 years ago
Putting [pdtp1] in whiteboard
Whiteboard: [nsbeta3+] → [nsbeta3+][pdtp1]

Comment 19

18 years ago
Would not hold PR3 for this...

Comment 20

18 years ago
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/
Severity: critical → blocker
Keywords: smoketest
Target Milestone: --- → M18

Comment 21

18 years ago
adding twalker and myself to cc.  removing smoketest keyword since it doesn't
appear this has impacted this morning's smoketesting.
Keywords: smoketest
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

18 years ago
The full paths in installed-chrome.txt are actually covered by bug 40626. But the 
more serious problem is covered by this bug.
(Reporter)

Comment 24

18 years ago
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...
(Assignee)

Comment 25

18 years ago
*** Bug 40626 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

18 years ago
Keywords: rtm
Whiteboard: [nsbeta3+][pdtp1] → [nsbeta3+][pdtp1][rtm+]
(Assignee)

Comment 26

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

Comment 27

18 years ago
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-
Whiteboard: [nsbeta3+][pdtp1][rtm+] → [nsbeta3-][pdtp1][rtm+]
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.
Keywords: relnote
Whiteboard: [nsbeta3-][pdtp1][rtm+] → [pdtp1][rtm+]
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 ;)
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

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

Comment 32

18 years ago
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.
(Assignee)

Comment 33

18 years ago
Created attachment 15712 [details] [diff] [review]
patch to fix bug
(Assignee)

Comment 34

18 years ago
Created attachment 15713 [details] [diff] [review]
revised patch
(Assignee)

Comment 35

18 years ago
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.
(Assignee)

Updated

18 years ago
Keywords: patch
Whiteboard: [pdtp1][rtm+] → [pdtp1][rtm+] unreviewed fix in hand
(Assignee)

Comment 36

18 years ago
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-
Whiteboard: [pdtp1][rtm+] unreviewed fix in hand → [nsbeta3-][pdtp1][rtm+] unreviewed fix in hand

Comment 37

18 years ago
PDT marking [rtm need info] until code reviews are available.
Whiteboard: [nsbeta3-][pdtp1][rtm+] unreviewed fix in hand → [nsbeta3-][pdtp1][rtm need info] unreviewed fix in hand
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. ;)
(Assignee)

Updated

18 years ago
Whiteboard: [nsbeta3-][pdtp1][rtm need info] unreviewed fix in hand → [nsbeta3-][pdtp1][rtm+ need review] unreviewed fix in hand
(Assignee)

Comment 39

18 years ago
r=sgehani
Whiteboard: [nsbeta3-][pdtp1][rtm+ need review] unreviewed fix in hand → [nsbeta3-][pdtp1][rtm+ need review]

Comment 40

18 years ago
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

18 years ago
PDT marking [rtm++]
Whiteboard: [nsbeta3-][pdtp1][rtm+ need review] → [nsbeta3-][pdtp1][rtm++]
(Assignee)

Comment 42

18 years ago
Created attachment 16349 [details] [diff] [review]
Revised patch incorporating feedback from scc

Comment 43

18 years ago
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.
(Assignee)

Comment 44

18 years ago
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.
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.
Hardware: Macintosh → All
(Assignee)

Comment 46

18 years ago
Created attachment 16422 [details] [diff] [review]
completely redone patch to fix potential slash conversion problems
(Assignee)

Comment 47

18 years ago
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

18 years ago
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.
(Assignee)

Comment 49

18 years ago
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.
(Assignee)

Comment 50

18 years ago
Checked into branch and trunk
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 51

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

Comment 52

18 years ago
I don't have a mac to verify this against, but I see the patch checked into the
tree.  marking verified.
Status: RESOLVED → VERIFIED
OS: All

Comment 53

18 years ago
Unverifying, since this was not verified by the normal channels.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---

Comment 54

18 years ago
Fixed, bug unverified. jrgm, can you please do some testing here, check 
installations on machines with / in folder names, and all that jazz? Thanks.
Status: REOPENED → RESOLVED
Last Resolved: 18 years ago18 years ago
QA Contact: granrose → jrgm
Resolution: --- → FIXED

Comment 55

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

Comment 56

18 years ago
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).
Keywords: relnote → vtrunk

Comment 57

18 years ago
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.
Status: RESOLVED → VERIFIED
Keywords: vtrunk
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.