36.18 KB, text/plain
36.59 KB, text/plain
3.18 KB, patch
Robert Kaiser: approval-seamonkey1.1.3+
|Details | Diff | Splinter Review|
Starting the SeaMonkey profile manager as a user that doesn't own brings up a XUL error dialog: XML Parsing Error: undefined entity Location: chrome://communicator/content/profile/profileSelection.xul Line Number 53, Column 1: If I run SeaMonkey once as the user, it works after that, even if I change the directory/file ownership back to root. Rolling back nsRDFXMLDataSource.cpp to v 1.154 fixes things (v 1.156 didn't help -- it's not the actual permissions). chrome.rdf seems to be the culprit. I'll attach working and not-working versions.
Created attachment 260286 [details] chrome.rdf generated by seamonkey-bin (happy) The difference here is that there are "selectedLocale" lines like: <c:selectedLocale RDF:resource="urn:mozilla:locale:en-US:communicator"/> Old builds (before bug 375102) did not have such lines either, but worked OK.
The selectedLocale stuff should usually be only in the chrome.rdf of the profile...
OK, so the basic problem here is that the API changed a bit. rdfXMLFlush (which is just an internal method) now returns failure if it can't write (chrome.rdf in the app directory is what it's trying to write to). This failure gets propogated by RDFXMLDataSourceImpl::Flush (a public API) and things down the line think something is wrong. The previous v 1.154 implementation ignored failure to open the file. In SeaMonkey, we seem to be calling into ::Flush from nsChromeRegistry::SetProviderForPackage, and we might update the method to be more selective of what it thinks is an real failure.
We actually call ::Flush from six places, one of which is only called for global chrome, one for profile chrome, and the other four may be either.
we need to figure this out ASAP so bug 375102 can land on branch without breaking us.
andrew / neil: any update on the seamonkey side?
Created attachment 262086 [details] [diff] [review] explicitly ignore permissions failure biesi suggested ignoring this specific failure and that does seem to work. I don't know if it will help with bug 377529, which I suspect is related. I also noticed that ::rdfXMLFlush actually claims to succeed if !mIsWritable. It seems reasonable for unwritable files to have !mIsWritable, but the code thinks any file is writable. http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/rdf/base/src/nsRDFXMLDataSource.cpp&rev=1.157&mark=601-606#588 fixing that might be a better fix and/or a way to help with bug 377529.
Comment on attachment 262086 [details] [diff] [review] explicitly ignore permissions failure This didn't seem to work for me. You probably need to at least look at WriteInfoToDataSource, which is also called during startup. I also think that you should always check for errors when writing to the profile.
Any progress here? People could be very sad when we land bug 375102 and then you can't ship 1.0.9 and 1.1.2
bug 375102 has been pushed out to 22.214.171.124/126.96.36.199, so we can let this slip the 1.0.9/1.1.2 releases, but we still need to look into fixing that problem. The branch patch for the original bug is virtually identical to the trunk patch, so it will probably cause the same problems with old chromereg, and we need to find a way to keep it working on branch (trunk will be fixed by switching to suiterunner eventually, but branch won't without a working patch).
andrew / neil / robert: heads up, bug #375102 is plussed for 188.8.131.52 and 184.108.40.206. I'd like to land it ASAP, so that we can test those builds as long as possible before we ship. But there is no patch for this bug, so that will break you. What can we do here?
Someone needs to come up with a patch. I don't understand the code, but I hope someone understands it can come up with a patch, else there will be no 1.8-based releases of SeaMonkey any more.
What about using "#ifdef MOZ_SUITE" in nsRDFXMLDataSource.cpp until there is a fix?
That surely would make us not block other apps, which is nice, but I fear this will mean that this bug will stay unfixed for the rest of the 1.8.x lifetime. In any case, please use "#ifndef MOZ_XUL_APP" there if we go that way, as it's really a problem of xpfe vs. toolkit and not suite vs. the rest of the world.
Comment on attachment 262086 [details] [diff] [review] explicitly ignore permissions failure > rv = remote->Flush(); >+ // Explicitly ignore permissions failure since we sometimes try to write to >+ // global chrome when we shouldn't. >+ if (rv == NS_ERROR_FILE_ACCESS_DENIED) >+ rv = NS_OK; I'll give r=me on a patch that does it on all calls to remote->Flush() (which you would of course factor out into a separate (inline?) method).
(In reply to comment #16) >I'll give r=me on a patch that does it on all calls to remote->Flush() (and installRemote->Flush() too of course)
Created attachment 270690 [details] [diff] [review] ignore more failures
Comment on attachment 270690 [details] [diff] [review] ignore more failures r+sr=me by code inspection.
fyi, on 7/4/07, the patch for bug #375102 landed on the MOZILLA_1_8_BRANCH (in order to be a part of firefox / tbird 220.127.116.11)
Comment on attachment 270690 [details] [diff] [review] ignore more failures this code is dead on trunk, so going straight for branch approval
Comment on attachment 270690 [details] [diff] [review] ignore more failures As long as it fixes profile manager on branch, this looks acceptable ;-) a=me for 1.1.3
Since bug 375102 and this landed (I could track it down to having happened on 2007-07-04), SeaMonkey can not be launched directly from the Mac disk image any more, showing XML errors in chrome. This should be relnoted.
(In reply to comment #23) >Since bug 375102 and this landed, SeaMonkey can not be launched directly from >the Mac disk image any more, showing XML errors in chrome. What happens if you expand and write protect the disk image (or try to run the expanded image as a user without write permission to it)?
https://www.mozdev.org/bugs/show_bug.cgi?id=17464 I've had this bug recently in connection with MultiZilla since 1.1.3 My workaround was to change the permissions on chrome.rdf to 666
FYI. When Win version of Seamonkey, following files are found in write-permitted program directory only(all other files have same timestamp and same file size.) A. Seamonkey 1.1.4 (ZIP build) chrome\chrome.rdf (also in chrome in profile) components\compreg.dat (not found in profile) components\xpti.dat (not found in profile) B. Seamonkey trunk 2007/9/17 build chrome\app-chrome.manifest (not found in profile) components\compreg.dat (found in profile) components\xpti.dat (found in profile) Trunk still writes compreg.dat & xpti.dat in program directory if write-permitted, and trunk writes them in profile too.
WADA: This comment is useless, as first this is a branch-only probably that is not present in trunk, and second, we know perfectly that .rdf files, probably chrome.rdf, are the problem here. We just don't know where in the code we could ignore or fix an error that is causing this misbehavior.
This bug has keyword of "fixed-seamonkey1.1.3" and OS is set to Linux. But 389136 reports same problem of Seamonkey 1.1.4 on Mac OS & MS Windows. Really "fixed-seamonkey1.1.3"? Does "fixed-seamonkey1.1.3 & OS=Linux" mean following? Problem on Linux was resolved with 1.1.3 by this bug. But problem on Mac&MS Win was not resolved by this bug, then still remains.
The main case, for an all-writeable directory (on all platforms) or a directory from which SeaMonkey never was launched (on Linux and Windows as it seems) was fixed for 1.1.3, but apparently there are still edge cases that see such a problem, like launching directly from a mac .dmg or launching from a non-writable directory that we could already write to in a different run.
(In reply to comment #29) > The main case, (snip) was fixed for 1.1.3, > but apparently there are still edge cases that see such a problem Thanks, I've understood situation and difference between this bug and Bug 389136. I think this bug is better to be closed as FIXED(fixed-seamonkey1.1.3 for main case, changing to OS=All) with pointing Bug 389136 as follow-up/successor for remaining edge case, in order to avoid confusion.
yes, this might be a good idea. I changed the summary and other fields on that bug accordingly, and am marking this fixed instead.