659802 - nsJSURI and nsFileDataURI need specializations of nsIIPCSerializable Read/Write methods taking IPC::Message*

Status: RESOLVED INVALID

(Core :: DOM: Core & HTML, defect, P5)

Description

[Daniel Holbert [:dholbert]]

Right now* (in cedar as of Bug 647570, soon to be on m-c) nsJSURI overrides one set of Read/Write methods on nsSimpleURI, but not the other set.

Specifically, nsJSURI overrides the nsISerializable versions (which take nsIObjectInputStream*), but _not_ the nsIIPCSerializable versions (which take IPC::Message*)

This triggers these build warnings:

> nsSimpleURI.h:67:60: warning: ‘virtual PRBool nsSimpleURI::Read(const IPC::Message*, void**)’ was hidden
> nsJSProtocolHandler.h:116:62: warning:   by ‘virtual nsresult nsJSURI::Read(nsIObjectInputStream*)’
> nsSimpleURI.h:67:157: warning: ‘virtual void nsSimpleURI::Write(IPC::Message*)’ was hidden
> nsJSProtocolHandler.h:117:62: warning:   by ‘virtual nsresult nsJSURI::Write(nsIObjectOutputStream*)’

...and it looks like this will also cause real bugs, if we try to send a nsJARURI via IPC.  (I have no idea how frequently that happens, if it happens at all.)

Comment 1

[Benjamin Smedberg]

I thought we agreed not to serialize URIs (and other XPCOM objects) like that, and all URIs would be passed via UTF8 string. Why are we doing the complex and dangerous thing here?

Comment 2

[Josh Matthews [:jdm]]

I don't recall there being any such decision. As far as I recall, the original goal was to only send URIs via the serialization methods, but I think that broke for javascript: ones or something, so we moved to passing as a UTF8 string as a fallback mechanism.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Note that for a javascript: URI the string representation is lossy, by the way.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #3)
> Note that for a javascript: URI the string representation is lossy, by the
> way.

The same is true for moz-filedata URIs.

Comment 5

[Daniel Holbert [:dholbert]]

(Comment 0 is also entirely true of moz-filedata URIs (nsFileDataURI), FWIW.)

Comment 6

[Benjamin Smedberg]

I see that JS URIs have a concept of a "base URI", although I don't quite understand why we encode that in the URI instead of in the load. But does it really make sense to carry that information across process boundaries?

I am very concerned about the IPC deserialization running lots of code which is going to be difficult to audit for security and safety. I really think we should prefer explicitly passing URI strings, origin charsets if absolutely necessary, and other load data explicitly as IPC arguments (perhaps in a union/struct type with standard handling routines), rather than carrying it along with URI objects.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> although I don't quite understand why we encode that in the URI instead of in
> the load.

Because that information is not available at load time, but is available at URI creation time...  Better solutions quite welcome.

> But does it really make sense to carry that information across process
> boundaries?

If you're sending JS URIs across process boundaries at all, then imo yes.  Otherwise relative URIs in the document produced by the JS URI will point to the wrong place.

Comment 8

[Benjamin Smedberg]

We send URIs in the following cases:

http://mxr.mozilla.org/mozilla-central/search?string=uri&find=\.ipdl&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Apart from URIs which are a known protocol in the networking stack, we have:

* referrers
* "doc"
* cookies (are these HTTP only?)
* content permission prompts (geolocation/etc)
* offline cache (HTTP only?)
* global history
* external helper apps

Which should all be safe, I think.

* the parent instructing the child to load a URL

This is the only one where we might require additional context: these should be coming from the frontend (URLbar/bookmarks/session store): and in these cases I think we want to be pretty explicit about context.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Given those use cases, the javascript: base URI issue should not arise.

Comment 11

[Daniel Holbert [:dholbert]]

I think this bug is now INVALID.

The interface whose methods were in question here (nsIIPCSerializable) was renamed to nsIIPCSerializableObsolete here:
 https://hg.mozilla.org/mozilla-central/rev/418b5cbc7cd9#l23.1
...and then was subsequently removed here:
 https://hg.mozilla.org/mozilla-central/rev/c813eeb62b92#l34.24

As you can see, that latter commit's removal included the read/write APIs for the "Message" type that I was talking about in comment 0:
-interface nsIIPCSerializableObsolete : nsISupports
-{
-  [notxpcom] boolean read(in ConstMessage msg, in Iterator iter);
-  [notxpcom] void write(in Message msg);
-};

So there's nothing left to be done here -- the API in question (whose specializations we needed here) has now been removed.