We really shouldn't be creating principals based on the spec for data: URIs.

necko doesn't know about principals -> caps

So I was actually going to expose a contract for this which Necko _would_ know about. But biesi convinced me to put this in getCodebasePrincipal...

Implementing a URI is really painful.... :(

Comment on attachment 219240 [details] [diff] [review] Proposed patch dveditz, I'd love your eye on the docshell and script security manager changes.

So, is there a reason why the viewsource URI does not just inherit from nsSimpleURI and just adds the new interface? Also, why does it need a contract ID?

It doesn't just inherit because I wasn't sure that was ok. If it did, I'd need to override nsIClassInfo, nsISerializable, Clone(), and all setter implementations, but I guess that would be less work, yeah. > Also, why does it need a contract ID? It doesn't. And it doesn't have one.

oh I see. I meant "why does it the module entry", but the answer is "because it's serializable". inheriting from other classes in the same module seems fine to me.

Comment on attachment 219240 [details] [diff] [review] Proposed patch netwerk/base/public/nsINestedURI.idl + // XXXbz should there be a method for "innermost" URI? Hard to tell + // whether making callers or callees handle that is a bigger + // implementation burden. well, the code would look identical for either case, right? in which case I'd rather keep the interface minimal. netwerk/base/public/nsNetUtil.h + nsresult rv; + nsCOMPtr<nsIIOService> grip; + rv = net_EnsureIOService(&ioService, grip); why not declare the rv on the same line where it is used? (I prefer not having uninitialized variables lying around when it can easily be avoided) Using __builtin_popcount for the "more than one flag" assertion is probably not worth it? gcc-only anyway. + * Helper function for testing whether the given uri, or any of its uppercase URI? hrm, the IDL list in netwerk/base/public/Makefile.in has inconsistent indentation :( but the majority seems to use tabs, so can you do that too? didn't review nsViewSourceURI, but if you want to go with this instead of inheriting from nsSimpleURI, let me know nsViewSourceHandler.cpp + // Inner URI must be serializable + nsCOMPtr<nsISerializable> serial = do_QueryInterface(innerURI); why? caps/src/nsScriptSecurityManager.cpp + // If either uri is a nested URI, get the base URI please fix the casing inconsistency here :-) (uri/URI) r=biesi, but I want to look at whatever URI impl you decide to go with

> well, the code would look identical for either case, right? It's a matter of whether we have a while loop in the URI impl or have while loops in all consumers. At the moment we have something like 2-3 consumers and 2 URI impls, so it's a wash.... > why not declare the rv on the same line where it is used? Copy-paste from other methods in the file.... Will fix. >>+ // Inner URI must be serializable > why? Because otherwise we can't be serializable. I'll do the inheriting from nsSimpleURI thing and post an updated patch.

(In reply to comment #9) > >>+ // Inner URI must be serializable > > why? > > Because otherwise we can't be serializable. Yeah, but should having a serializable URI be a requirement for viewing the source of pages? Why not fail in Write instead?

Hmm... OK, that makes sense. Will do that.

Comment on attachment 219560 [details] [diff] [review] Fix a view-source issue in the previous patch netwerk/base/public/nsINestedURI.idl + */ + +[scriptable,uuid(6de2c874-796c-46bf-b57f-0d7bd7d6cab0)] should probably remove the empty line and add a comma before "uuid" netwerk/protocol/viewsource/src/nsViewSourceHandler.h What's mSimpleURI for? netwerk/protocol/viewsource/src/nsViewSourceHandler.cpp +nsViewSourceURI::GetClassID(nsCID * *aClassID) why overwrite this one? SimpleURI already throws for most nsIURI methods. are you overwriting them because you think that might change in the future?

> What's mSimpleURI for? Removed. ;) > why overwrite this one? I didn't want to depend on the nsSimpleURI impl. I could change that, I suppose. > are you overwriting them because you think that might change in the future? Yes. I'd rather people could change nsSimpleURI without worrying about this class too much.

Comment on attachment 219560 [details] [diff] [review] Fix a view-source issue in the previous patch >Index: netwerk/base/public/nsIProtocolHandler.idl >+ /** >+ * The URIs for this protocol have no inherent security context. That is, >+ * it's not possible to decide what a document loaded from one of these >+ * URIs should be allowed to do. >+ */ >+ const unsigned long URI_HAS_NO_SECURITY_CONTEXT = (1<<4); I think it would be good to move this up above the ALLOW_ constants, next to the other URI_ constants. That way it is grouped with other flags that constrain the URI. >Index: netwerk/base/public/nsINestedURI.idl >+/** >+ * nsINestedURI is an interface that URIs that have some sort of >+ * "inner" URI that they get data from must implement. This sentence is really hard to read. Maybe "that" appears too many times or maybe some commas are needed. >+ */ >+ >+[scriptable,uuid(6de2c874-796c-46bf-b57f-0d7bd7d6cab0)] Please kill the newline. >+interface nsINestedURI : nsISupports >+{ >+ /** >+ * The inner URI for this nested URI. >+ */ >+ readonly attribute nsIURI innerURI; >+ >+ // XXXbz should there be a method for "innermost" URI? Hard to tell >+ // whether making callers or callees handle that is a bigger >+ // implementation burden. >+}; I'd be okay with an innermostURI attribute. You could add a function to nsNetUtil.h that gets the innermostURI and use that to implement GetInnermostURI. >Index: netwerk/base/public/nsNetUtil.h >+inline nsresult >+NS_GetHandlerForURI(nsIProtocolHandler **handler, >+ nsIURI *uri, >+ nsIIOService *ioService = nsnull) NS_GetProtocolHandler{ForURI} seems better to me. Not sure if the "ForURI" part is really needed. >+/** >+ * Helper function for testing whether the given URI's handler has the >+ * given protocol flag. >+ */ >+inline nsresult >+NS_ProtocolHasFlag(nsIURI *uri, migth make sense to put this on nsINetUtil since it is a significant amount of code. >+ PRUint32 flag, >+ PRBool *result) >+{ >+ NS_PRECONDITION(flag != 0, "No flag?"); >+#ifdef DEBUG >+ { >+ PRUint32 temp = flag; >+ while (temp) { >+ NS_ASSERTION(temp == 1 || temp % 2 == 0, "More than one flag?"); >+ temp >>= 1; >+ } >+ } >+#endif I'd recommend making this debug bit a helper function. [I had to stop here ... will resume asap]

Comment on attachment 219560 [details] [diff] [review] Fix a view-source issue in the previous patch >Index: netwerk/base/public/nsNetUtil.h >+/** >+ * Helper function for testing whether the given URI, or any of its >+ * inner URIs, has the given protocol flag. >+ */ >+inline nsresult >+NS_URIChainHasFlag(nsIURI *uri, >+ PRUint32 flag, >+ PRBool *result) >+{ >+ nsresult rv = NS_ProtocolHasFlag(uri, flag, result); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ if (*result) { >+ return rv; >+ } >+ >+ // Dig deeper into the chain >+ nsCOMPtr<nsINestedURI> nestedURI = do_QueryInterface(uri); >+ while (nestedURI) { Maybe this should be moved to nsINetUtil as well. >Index: netwerk/protocol/viewsource/src/nsViewSourceHandler.h >+ // We need a different nsISerializable impl from nsSimpleURI >+ NS_DECL_NSISERIALIZABLE Why do we need to serialize both the view-source: URI and the inner URI? Why not just serialize one of those and reconstruct the other? >+ // Various nsIURI overrides. We want to be immutable; Maybe nsSimpleURI should have a constructor parameter to indicate that it is immutable. I'd prefer that to having to special case all of these setters. Do something like nsStandardURL does to support immutability. >+ NS_IMETHOD Equals(nsIURI* other, PRBool *result); >+ NS_IMETHOD Clone(nsIURI* *result); nsStandardURL has a virtual method called StartClone that is called by nsStandardURL::Clone. The advantage of that is that it enables you to be able to leverage the code in nsSimpleURI::Clone. I think we should do the same here. >+ // Override the nsIClassInfo methods needed to make sure our >+ // nsISerializable impl works right. >+ NS_IMETHOD GetContractID(char * *aContractID); >+ NS_IMETHOD GetClassID(nsCID * * aClassID); >+ NS_IMETHOD GetClassIDNoAlloc(nsCID *aClassIDNoAlloc); Why do you need to override GetClassID if the superclass just calls GetClassIDNoAlloc? I also don't see a reason to override GetContractID. >+protected: >+ nsCOMPtr<nsIURI> mSimpleURI; >+ nsCOMPtr<nsIURI> mInnerURI; >+}; mSimpleURI is never used. You can remove it right? >Index: netwerk/protocol/viewsource/src/nsViewSourceHandler.cpp >+ // We can's swap() from an nsRefPtr<nsViewSourceURI> to an nsIURI**, sadly. "can't" >+ nsViewSourceURI* ourURI = new nsViewSourceURI(innerURI); >+ nsCOMPtr<nsIURI> uri = ourURI; >+ if (!uri) >+ return NS_ERROR_OUT_OF_MEMORY; >+ >+ // Call superclass SetSpec explicitly, since ours always fails. >+ rv = ourURI->nsSimpleURI::SetSpec(asciiSpec); > if (NS_FAILED(rv)) > return rv; >+ >+ uri.swap(*aResult); > return rv; > } You could set the immutable flag after calling SetSpec I suppose. Hmm, perhaps an immutable flag is undesirable because it adds to the allocation size of nsSimpleURI? That's probably insignificant and there may also be benefit to making nsSimpleURI store mSpec to optimize GetSpec. That's just a tad outside the scope of this bug ;-) >+nsViewSourceURI::Read(nsIObjectInputStream* aStream) >+nsViewSourceURI::Write(nsIObjectOutputStream* aStream) Come to think of it, do we actually ever fastload view-source URLs? >+NS_IMETHODIMP >+nsViewSourceURI::Equals(nsIURI* other, PRBool *result) >+{ >+ if (other) { >+ nsCAutoString scheme; >+ nsresult rv = other->GetScheme(scheme); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ if (scheme.EqualsLiteral(VIEW_SOURCE)) { Use nsIURI::SchemeIs ;-) >+ nsCOMPtr<nsINestedURI> nest = do_QueryInterface(other); Once the schemes match, I suppose you could just cast this nsIURI to nsViewsourceURI and access its member variables directly.

> Why not just serialize one of those and reconstruct the other? Mostly because doing this is actually less code, as far as I can tell. I'm lazy, and this isn't going to happen much, so... > Why do you need to override GetClassID if the superclass just > calls GetClassIDNoAlloc? > I also don't see a reason to override GetContractID. I wanted to depend on nsSimpleURI not changing as little as possible. > Come to think of it, do we actually ever fastload view-source URLs? There's no reason we wouldn't. > Once the schemes match, I suppose you could just cast this nsIURI > to nsViewsourceURI That will crash, generally speaking, if some bozo created it with createInstance instead of newURI(). So I'd rather not. ;) I'll fix up the nits and try to do the immutability and cloning stuff...

Note that the XUL fastload version changed because with this patch we'd mis-read any serialized simple URIs, unless I changed the CID or something. Which maybe I should do...

Comment on attachment 220229 [details] [diff] [review] With darin's comments addressed More comments... >Index: netwerk/base/public/nsINestedURI.idl >+[scriptable, uuid(6de2c874-796c-46bf-b57f-0d7bd7d6cab0)] >+interface nsINestedURI : nsISupports >+{ >+ /** >+ * The inner URI for this nested URI. This must not return null if >+ * the getter succeeds. >+ */ >+ readonly attribute nsIURI innerURI; Sorry for not noticing this earlier, but I think this getter should be allowed to return null. Otherwise, this API becomes painful to use from Javascript. IMO, it is better to return null and NS_OK when there is no inner URI. If the function is going to throw an exception it should be because something truly exceptional happened. >+ >+ /** >+ * The innermost URI for this nested URI. >+ */ >+ readonly attribute nsIURI innermostURI; And, this one I assume never returns null, right? Does it return |this| in some cases? Is the result a clone or not? I think it should be stated that it will not be a clone. That's important for determining whether or not the caller should have to clone the nsIURI in order to modify it. >Index: netwerk/base/public/nsINetUtil.idl >+ /** >+ * Test whether the protocol handler for this URI or that for any of >+ * its given URIs has the given protocol flag. This will QI aURI to Did you mean "its _inner_ URIs" instead? I don't understand "given" URIs. >+ * @param aFlag the flag we're testing for. This must be a single bit. I actually don't understand why this is important. Why can't I use this API to test if multiple flags are set? Why do we need to constrain the input? >Index: netwerk/base/public/nsNetUtil.h >+/** >+ * Helper functions for getting the innermost URI for a given nsINestedURI >+ */ >+inline nsresult >+NS_DoGetInnermostURI(nsINestedURI* nestedURI, nsIURI** result); >+ >+inline nsresult >+NS_GetInnermostURI(nsINestedURI* nestedURI, nsIURI** result) >+{ >+ // Make it safe to use swap() >+ *result = nsnull; >+ >+ return NS_DoGetInnermostURI(nestedURI, result); >+} >+ >+inline nsresult >+NS_DoGetInnermostURI(nsINestedURI* nestedURI, nsIURI** result) >+{ >+ NS_PRECONDITION(nestedURI, "Must have a nested URI!"); >+ NS_PRECONDITION(!*result, "Must have null *result"); >+ >+ nsCOMPtr<nsIURI> inner; >+ nsresult rv = nestedURI->GetInnerURI(getter_AddRefs(inner)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ nsCOMPtr<nsINestedURI> nestedInner(do_QueryInterface(inner)); >+ if (!nestedInner) { >+ // Found the innermost one >+ inner.swap(*result); >+ return NS_OK; >+ } >+ >+ return NS_DoGetInnermostURI(nestedInner, result); >+} Why bother forward declaring NS_DoGetInnermostURI? These do not appear to be interdependent. >+/** >+ * Helper function for testing whether the given URI, or any of its >+ * inner URIs, has the given protocol flag. >+ */ >+inline nsresult >+NS_URIChainHasFlag(nsIURI *uri, >+ PRUint32 flag, >+ PRBool *result) >+{ >+ nsresult rv; >+ nsCOMPtr<nsINetUtil> util = do_GetIOService(&rv); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ return util->UriChainHasFlag(uri, flag, result); nit: I think the nsINetUtil method should be called URIChainHasFlag instead of uriChainHasFlag (or UriChainHasFlag). We are pretty consistent about using "URI" in method names (in necko at least). >Index: netwerk/base/src/nsIOService.cpp >+nsIOService::ProtocolHasFlag(nsIURI *uri, >+ PRUint32 flag, >+ PRBool *result) indentation nit >+nsIOService::UriChainHasFlag(nsIURI *uri, >+ PRUint32 flag, >+ PRBool *result) ditto >+ // the extra addref/release on |uri| in the common (non-nested) case. Maybe >+ // we shouldn't worry about that? Seems like a reasonable optimization :) >+ nsCOMPtr<nsINestedURI> nestedURI = do_QueryInterface(uri); >+ while (nestedURI) { >+ nsCOMPtr<nsIURI> innerURI; indentation is 4 whitespace characters in this file. >Index: netwerk/base/src/nsSimpleURI.h >+ PRBool mImmutable; nsStandardURL has mMutable as the member var FWIW. Might be nice to be consistent. >Index: netwerk/base/src/nsSimpleURI.cpp >+ // Should cloning clone mImmutable? I think it should... >+ url->mImmutable = mImmutable; I think you should make the clone mutable. That's what nsStandardURL does, and for good reason... what's the point of having a clone method if the result is immutable? Why not just AddRef? ;-) >Index: netwerk/protocol/viewsource/src/nsViewSourceHandler.cpp >+ // Make the URI immutable so mInnerURI will stay in sync >+ ourURI->SetImmutable(); nsStandardURL has a SetMutable(PRBool) method, but that's defined on nsIStandardURL... hmm... I can see why you wouldn't want to grant people the ability to mutate the URI _ever_. >+NS_IMETHODIMP >+nsViewSourceURI::Write(nsIObjectOutputStream* aStream) >+{ >+ // Just serializing the whole thing is easier than having to recover parts >+ // of the state from just mInnerURI or from our nsSimpleURI data. >+ nsCOMPtr<nsISerializable> serializable = do_QueryInterface(mInnerURI); >+ if (!serializable) { >+ // We can't serialize ourselves >+ return NS_ERROR_NOT_AVAILABLE; >+ } >+ >+ nsresult rv = nsSimpleURI::Write(aStream); >+ if (NS_FAILED(rv)) return rv; >+ >+ rv = aStream->WriteCompoundObject(mInnerURI, NS_GET_IID(nsIURI), >+ PR_TRUE); >+ if (NS_FAILED(rv)) return rv; Seems like this might cause problems where in the past we just happily serialized view-source:foobar:, now we are requiring the foobar URI implementation to implement nsISerializable. Maybe that's an edge case worth ignoring? >+NS_IMETHODIMP >+nsViewSourceURI::GetInnerURI(nsIURI** uri) >+{ >+ // XXXbz should this clone, so that mInnerURI will stay in sync with our >+ // nsSimpleURI? Ah.. you and I think alike. We need to decide and document it in the IDL. I think no clone is best since we have the mutable flags and the caller can always clone if needed.

> IMO, it is better to return null and NS_OK when there is no inner URI. And IMO there is never no inner URI... That's certainly the case for all the impls we have now; I suppose you're thinking about cases where there might or might not be an inner URI depending on the details of the URI in question? The thing is that whether we have an inner URI or not needs to not mutate during the lifetime of a URI, imo; otherwise the security context of the URI also mutates and we're not well-equipped to deal with that. More on this below. :( > And, this one I assume never returns null, right? That's correct, no matter what we decide above. If we allow the innerURI to be null in some cases, then this could in fact return |this|. I'm torn about the clone issue. I don't trust callers to not modify URIs, especially if we expose nsIURIs for our documents, etc to JS more (because I suspect we won't be able to keep those URIs out of the hands of content script). I really wish we could mark URIs immutable, but we don't have that capability. :( So perhaps both GetInnerURI and GetInnermostURI _should_ in fact clone... > Why can't I use this API to test if multiple flags are set? Because then I have to somehow indicate in the function name whether we'll test for "all flags set" or "any of the flags set"... I suppose I could document that it'll be "all flags set" and remove this assert, but I'd prefer to also name the function accordingly and I couldn't think of anything. > Why bother forward declaring NS_DoGetInnermostURI? I wanted to declare the "public" method first, I guess. I'll change the order and add some comments. > nsStandardURL has mMutable as the member var FWIW. Will make nsSimpleURI do the same. > I think you should make the clone mutable. OK. Makes sense. > I can see why you wouldn't want to grant people the ability to mutate the URI > _ever_ Yeah. So the problem is that I want the inner URI that comes back from this to either be immutable or for mutations to it to not affect the view-source: URI. Unless we have a way to force immutability for general URIs, I think I'm going to have to make GetInnerURI clone here.... Might want to do the same in nsJarURI too. In general, though, I'd want to make things like the currentURI of a webnavigation immutable. Otherwise I'm pretty worried about security issues. At the moment the only thing that prevents content from messing with that URI is the lack of DOM classinfo for docshell and the URI classes (and possibly only the former, since the latter _does_ sorta exist). > Seems like this might cause problems where in the past we just > happily serialized view-source:foobar:, now we are requiring > the foobar URI implementation to implement nsISerializable. Hmm.... true. I guess I'll try to reconstruct the URI from the deserialized nsSimpleURI, then. > I think no clone is best since we have the mutable flags Unfortunately, we don't have those for mInnerURI, since that's some random nsIURI that we have no control over. :(

> And IMO there is never no inner URI... That's certainly the case for all the > impls we have now; I suppose you're thinking about cases where there might or > might not be an inner URI depending on the details of the URI in question? > > The thing is that whether we have an inner URI or not needs to not mutate > during the lifetime of a URI, imo; otherwise the security context of the URI > also mutates and we're not well-equipped to deal with that. More on this > below. :( I see. That makes sense. > I'm torn about the clone issue. I don't trust callers to not modify URIs, > especially if we expose nsIURIs for our documents, etc to JS more (because I > suspect we won't be able to keep those URIs out of the hands of content > script). I really wish we could mark URIs immutable, but we don't have that > capability. :( So perhaps both GetInnerURI and GetInnermostURI _should_ in > fact clone... I'm not understanding. If you make the nsIURIs exposed by our datastructures immutable, then all is well. So what if callers can grab the nsIURI, clone it, and modify the clone? If you don't allow them to modify the clone, then they will just call GetSpec + NewURI. What am I missing? > > Why can't I use this API to test if multiple flags are set? > > Because then I have to somehow indicate in the function name whether we'll > test for "all flags set" or "any of the flags set"... I suppose I could > document that it'll be "all flags set" and remove this assert, but I'd prefer > to also name the function accordingly and I couldn't think of anything. Hmm... the name seems fine to me. Just document that all of the given bits must match. > > Why bother forward declaring NS_DoGetInnermostURI? > > I wanted to declare the "public" method first, I guess. I'll change the > order and add some comments. OK > > I can see why you wouldn't want to grant people the ability to mutate the > > URI _ever_ > > Yeah. So the problem is that I want the inner URI that comes back from this > to either be immutable or for mutations to it to not affect the view-source: > URI. Unless we have a way to force immutability for general URIs, I think > I'm going to have to make GetInnerURI clone here.... Might want to do the > same in nsJarURI too. If the inner URI QIs to nsIStandardURL, then you can mark it immutable. Perhaps nsISimpleURI should expose a mutable attribute. Then you would be able to optimize for those two cases. In other cases, you could return a clone. > In general, though, I'd want to make things like the currentURI of a > webnavigation immutable. Otherwise I'm pretty worried about security issues. > At the moment the only thing that prevents content from messing with that URI > is the lack of DOM classinfo for docshell and the URI classes (and possibly > only the former, since the latter _does_ sorta exist). Yes, I agree that we should make the webnavigation's URI immutable. Actually, I think that nsIChannel::URI and ::OriginalURI should always be immutable. However, that means that when we construct a channel, we will probably need to clone the given URI. We could optimize for nsIIOService::NewChannel calls, but there is sadly no equivalent method on nsIProtocolHandler. > > I think no clone is best since we have the mutable flags > > Unfortunately, we don't have those for mInnerURI, since that's some random > nsIURI that we have no control over. :( Right... see above.

Thing is, nsIStandardURL _does_ allow setting mutable to true. I'm trying to protect against malicious callers here, unfortunately, not just negligent ones, so that won't do. Perhaps we should make it possible to set mMutable to false, but not change it back to true? Consumers who really care can clone or something.

> I'm trying to protect against malicious callers here What does that mean? I can cast to nsStandardURL and set mMutable to true as well if I really wanted to. Why do we care about malicious callers?

> Perhaps we should make it possible to set mMutable to false, but not change it > back to true? Consumers who really care can clone or something. That sounds like a good plan to me. I don't think anyone is really using nsIStandardURL::mutable yet, so we can redefine it a bit.

> I can cast to nsStandardURL and set mMutable to true as well if I really wanted > to. Not if you're JS. What you _can_ do if you're JS and you get your hands on a URI object is QI to nsIStandardURL. We have no way of preventing that, even for untrusted JS. So either we need to prevent untrusted JS ever getting URI objects (hard), or we need to make sure that all URIs that come from layout/docshell out to JS through getters are either clones or immutable.

Darin, what do you think? Note the XXX comment about having a common API for the "mutable" member, btw.

nsIMutable?

Yeah, an interface like nsIMutable in xpcom would be nice because then we could make nsLocalFile implement it. nsIFile has much the same issue as nsIURI concerning mutability and cloning.

Comment on attachment 220367 [details] [diff] [review] Now with nsIMutable >Index: xpcom/base/nsIMutable.idl >+ * it cannot be reset back to true -- attempts to do so silently do >+ * nothing. Why not throw NS_ERROR_INVALID_ARG? >Index: netwerk/base/public/nsNetUtil.h >+/** >+ * Helper function that ensures that |result| is a URI that's safe to >+ * mutate. If |uri| is immutable, just returns it, otherwise returns >+ * a clone. |uri| must not be null. >+ */ >+inline nsresult >+NS_EnsureSafeToMutate(nsIURI* uri, nsIURI** result) Wait, how can something that is immutable be safe to mutate? Because it will not allow you to mutate it? Seems a bit awkward to me. I'm not sure if I can think of anything better though ;-) Maybe something with a synonym of "disconnected" >+NS_TryToSetImmutable(nsIURI* uri) ... >+ mutableObj->SetMutable(PR_FALSE); >+ return; >+ } >+} No need for "return" statement. >Index: netwerk/base/src/nsStandardURL.cpp > NS_IMETHODIMP > nsStandardURL::SetMutable(PRBool value) > { >+ if (mMutable) { >+ mMutable = value; >+ } This code tends to drop brackets when they are unnecessary. Same goes for nsSimpleURI.cpp. sr=darin

jst, could you check out the script security manager change? > Maybe something with a synonym of "disconnected" I went with NS_EnsureSafeToReturn, but let me know if you don't like it? I made the other changes, including the NS_ERROR_INVALID_ARG thing.

Comment on attachment 220480 [details] [diff] [review] Address those comments >+ * nsIMutable defines an interface to be implemented by objects which >+ * can be made immutable. nsIMutable might be a useful name to save for a broader purpose, although I suppose nsIImmutable llooks llike I've got a sticky keyboard. >+NS_EnsureSafeToReturn(nsIURI* uri, nsIURI** result) >+{ >+ NS_PRECONDITION(uri, "Musth have a URI"); nit: typo "Musth" r=dveditz on the nsScriptSecurityManager changes

> r=dveditz on the nsScriptSecurityManager changes You probably also want to replace the ugly manual de-nesting of jar: in nsScriptSecurityManager::LookupPolicy with GetInnermostURI.

> You probably also want to replace the ugly manual de-nesting of jar: I'd need to construct a URI object from the string to do that; that'd be expensive. Now possibly nsPrincipal::GetOrigin should do that... I'll file a followup bug to investigate that.

Fixed. Filed bug 336303 on GetOrigin.