I don't see how Write writes out enough information to restore the principal, nor how Read() actually restores it. It _could_ work if the caller then happened to do the InitFromPersistent stuff given the pref name that the principal reads out, but that's not happening anywhere that I can tell. Even then you'd need minor things like the codebase and the cert to actually function. It so happens that fastload only writes out and reads in system principals for now, so things don't break badly. Bug I was about to suggest to someone that we use the serializing stuff on principals to store info along with a bookmark... and that would just have been bad. I think we either need to fix this (not sure how unless we make certs implement nsISerializable) or not have nsPrincipal QI to nsISerializable, or at least have Read and Write throw.

Seems like the easy thing to do is to only make the system principal QI to nsISerializeable. Not marking as blocking since this doesn't appear to actually break anything right now. But it'd be nice to fix and should be relatively easy to do so.

Actually, this does break stuff. I'm given to understand that places and feeds are doing security checks with URIs as the origin, and they can't do them with principals because they need to persist the object representing the originator of the action. They can persist a URI. But they can't persist a principal. Sorry I forgot to set the deps when I filed...

This doesn't do certificates yet; I'm waiting on feedback from the crypto guys before implementing serialization of those. But it should do everything else. This does fix bug 387446 over here.

This also does certificates, if they're serializable. They're not yet, but I filed bug 388128 on it.

Comment on attachment 272288 [details] [diff] [review] Somewhat better Brendan, is changing XUL_FASTLOAD_FILE_VERSION enough? Note that changing nsPrincipal serialization can change how JSScript serializes. Do I need to bump JSXDR_BYTECODE_VERSION as well?

Comment on attachment 272288 [details] [diff] [review] Somewhat better >+ nsTArray<nsAutoPtr<nsHashtable> > mAnnotations; Nit: space after outer < to match mandatory > > > NS_IMETHODIMP > nsPrincipal::Read(nsIObjectInputStream* aStream) > { >- PRUint32 annotationCount; >- nsresult rv = aStream->Read32(&annotationCount); >+ mInitialized = PR_TRUE; Should this be set true only at the end, false at the front in case of early return? Or see below for a more compact style of error handling used by XUL fastload code. >+ >+ PRBool hasCapabilities; >+ nsresult rv = aStream->ReadBoolean(&hasCapabilities); >+ if (NS_SUCCEEDED(rv) && hasCapabilities) { >+ // We want to use one of the nsHashtable constructors, but don't want to >+ // generally have mCapabilities be a pointer... and nsHashtable has no >+ // reasonable copy-constructor. Placement-new to the rescue! >+ mCapabilities.~nsHashtable(); >+ new (&mCapabilities) nsHashtable(aStream, ReadAnnotationEntry, >+ FreeAnnotationEntry, &rv); >+ } >+ > if (NS_FAILED(rv)) { > return rv; > } Assuming the dtor will clean up everything, you could code 'rv |= ...' in series, and test for failure in rv at the end only. Same for the Write method. /be

> Should this be set true only at the end I can nix that line completely, since I call Init(). It's a relic from an early patch version. I'll remove it. I did consider the rv |= thing. It requires care as to which things are or are not safe to press through on (e.g. if reading haveCert errors, we don't want to readObject the cert), and I didn't want to deal with either having to think about it myself or impose that mind-print on others.

(In reply to comment #7) > > Should this be set true only at the end > > I can nix that line completely, since I call Init(). It's a relic from an > early patch version. I'll remove it. Ok. Do you want review with that in mind, or are you gonna put up a new patch? > I did consider the rv |= thing. It requires care as to which things are or are > not safe to press through on (e.g. if reading haveCert errors, we don't want to > readObject the cert), and I didn't want to deal with either having to think > about it myself or impose that mind-print on others. I hear ya. I still find it acceptable for more "straight-line outermost logic level" control flow, which you have a bunch of in the patch. /be

Review with that line removed in mind if you can?

Comment on attachment 272288 [details] [diff] [review] Somewhat better r+me with < ... < ... > > angle-bracket spacing symmetry nit picked. /be

Comment on attachment 272288 [details] [diff] [review] Somewhat better r=dveditz

This makes the principal serialization/deserialization code actually work. We need this for session restore and to prevent bogus principal objects appearing when non-system things are fastloaded.

Fix checked in.