Bug 706590 - API for implementing custom protocol handlers #290

Closed
wants to merge 35 commits into
from

Projects

None yet

3 participants

@Gozala
Member
Gozala commented Dec 7, 2011

This change implements low level API for defining custom about & URI protocol handlers. This API will be used to implement custom addon: protocol handler required for Bug 706590.

Please note: This work branched out from #305 that provides API for implementing complex XPCOM interfaces. This is also a reason why this change looks huge. It will look smaller once dependencies will land. Meanwhile please ignore those changes.

@ochameau ochameau and 1 other commented on an outdated diff Dec 27, 2011
packages/api-utils/lib/protocol.js
@@ -0,0 +1,145 @@
+/* vim:set ts=2 sw=2 sts=2 expandtab */
+/*jshint asi: true undef: true es5: true node: true devel: true browser: true
+ forin: true latedef: false globalstrict: true */
+/*global define: true */
ochameau
ochameau Dec 27, 2011 Member

Is this something we really want into production code?

Gozala
Gozala Jan 16, 2012 Member

Well it's useful when used in combination with jshint / jslint that many editors integrate with, but since it does not fully supports moz-js I'll remove it.

@ochameau ochameau commented on an outdated diff Dec 27, 2011
packages/api-utils/lib/protocol.js
@@ -0,0 +1,145 @@
+/* vim:set ts=2 sw=2 sts=2 expandtab */
+/*jshint asi: true undef: true es5: true node: true devel: true browser: true
+ forin: true latedef: false globalstrict: true */
+/*global define: true */
+
+'use strict';
ochameau
ochameau Dec 27, 2011 Member

Small note about consistency, we almost everywhere use "use strict"; (instead of 'use strict';)

@ochameau ochameau and 1 other commented on an outdated diff Dec 27, 2011
packages/api-utils/lib/protocol.js
@@ -0,0 +1,145 @@
+/* vim:set ts=2 sw=2 sts=2 expandtab */
+/*jshint asi: true undef: true es5: true node: true devel: true browser: true
+ forin: true latedef: false globalstrict: true */
+/*global define: true */
+
+'use strict';
+
+const { Cc, Ci, components: { Constructor: CC } } = require('chrome')
ochameau
ochameau Dec 27, 2011 Member

You should be able to do the following shorter code instead:
const { Cc, Ci, Constructor: CC } = require('chrome')
(you already used 'CC' like this in a previous patch)
And do not forget the ; at end of line ;)

ochameau
ochameau Dec 27, 2011 Member

Oops I meant:
const { Cc, Ci, CC } = require('chrome');

Gozala
Gozala Jan 16, 2012 Member

Yeah I just copied bunch of lines here from my protocol lib which was implemented back in time where CC was not exported. I'll update this.

@ochameau ochameau and 1 other commented on an outdated diff Dec 27, 2011
packages/api-utils/lib/protocol.js
+ },
+ end: function end(content) {
+ if (content) this.write(content)
+ response(this).stream.close();
+ }
+});
+
+const AbstractHandler = {
+ onRequest: function onRequest() { throw Error('Not implemented') },
+ newChannel: function newChannel(uri) {
+ let channel, pipe, response, request;
+
+ // We create `nsIPipe` which where response's output will be forwarded to.
+ pipe = Pipe(true, true, 0, 0, null);
+ request = { uri: uri.spec };
+ response = Response.new(request.uri, pipe.outputStream);
ochameau
ochameau Dec 27, 2011 Member

pipe is necessary only if we do not redirect.
Shouldn't it be a private of Response ? And moreover lazily created only if the user of this API starts using start/end ?

Gozala
Gozala Jan 16, 2012 Member

I assume you meant write / end instead. Yes it could be created lazily on write, but it was making implementation significantly more complicated mainly harming readability. Unless you feel very strong about it, I'd prefer to leave it as is and optimize to laziness if that will give us any visible improvements which I don't expect to get.

ochameau
ochameau Jan 17, 2012 Member

Yes, that's fine, it is just a nifty optimisation. Here is the sort of thing I had in mind:

function pipe(object) {
  let privates = response(object);
  if (!privates.pipe)
    privates.pipe = Pipe(true,true, 0, 0, null);
  return privates.pipe;
}
pipe(this).outputStream.write()
pipe(this).outputStream.close()
pipe(response).inputStream

Speaking about namespace. Don't you think we should add a second argument that would allow to set the default value of the private object?
it would allow to do: response(this) = stream; via response(this, stream);. I know it goes against the default prototype given to ns constructor, but we have many usecase where we have only one attribute as private and it would be cool to get rid of it.

Gozala
Gozala Jan 20, 2012 Member

it would allow to do: response(this) = stream; via response(this, stream);. I know it goes against the default prototype given to ns constructor, but we have > many usecase where we have only one attribute as private and it would be cool to get rid of it.

  1. I think it makes it less obvious what the line does. response(this, stream) in my mind does not resembles response(this) = stream.
  2. Then there is a question of how to access such private ? Probably response(this) but then it becomes confusing as same function is used in both ways.

I would like to optimize single property case as well, but I don't think this is a good solution.

@ochameau ochameau commented on an outdated diff Dec 27, 2011
packages/api-utils/lib/protocol.js
+ // it.
+ if (response.uri !== request.uri) {
+ // Close a response as it's a redirect and writing to redirected socket
+ // is just wrong.
+ response.end();
+ channel = URIChannel(response.uri, null, null);
+ // Set `originalURI` pointing to a pre-redirect `uri` to make this
+ // information available.
+ channel.originalURI = uri;
+ }
+ // Otherwise create a channel from the input stream of the pipe, to which
+ // user can write asynchronously.
+ else {
+ // Optionally `channelURI` may be set in order to override both output
+ // and a URI.
+ uri = response.channelURI ? URI(response.channelURI, null, null) : uri;
ochameau
ochameau Dec 27, 2011 Member

This feature is not unit-tested, or at least, used in a test so that we can at least know it doesn't make the code fail/throw.

@ochameau ochameau commented on an outdated diff Dec 27, 2011
packages/api-utils/lib/protocol.js
+ channel.setURI(uri);
+ channel.contentStream = pipe.inputStream;
+ channel.QueryInterface(Ci.nsIChannel);
+
+ // Copy content length, type and charset from the response.
+ channel.contentLength = response.contentLength;
+ channel.contentType = response.contentType;
+ channel.contentCharset = response.contentCharset;
+ }
+
+ // If `principalURI` is set to anything other then an `request.uri` it
+ // means that channel owner must be different and it's privileges must be
+ // inherited. This feature is handy, for a custom URI implementers that
+ // proxy to a content of the different URIs.
+ if (response.principalURI !== response.uri)
+ channel.owner = Principal(URI(response.principalURI, null, null));
ochameau
ochameau Dec 27, 2011 Member

As we have seen during our peer review, it doesn't work if we want system principal. (because of bug 160042)
We will need something specific, similar to this:

if (response.principalURI === "chrome")
  channel.owner = Cc["@mozilla.org/systemprincipal;1"].createInstance(Ci.nsIPrincipal);
else if (response.principalURI !== response.uri)
  channel.owner = Principal(URI(response.principalURI, null, null));
@ochameau ochameau and 2 others commented on an outdated diff Dec 27, 2011
packages/api-utils/lib/protocol.js
+ // inherited. This feature is handy, for a custom URI implementers that
+ // proxy to a content of the different URIs.
+ if (response.principalURI !== response.uri)
+ channel.owner = Principal(URI(response.principalURI, null, null));
+
+ return channel;
+ }
+}
+exports.AbstractHandler = AbstractHandler;
+
+const AboutHandler = Factory.extend(AbstractHandler, {
+ get what() { throw Error('Property `what` is required') },
+ interfaces: [ 'nsIAboutModule' ],
+ get classDescription() 'Protocol handler for "about:' + this.what + '"',
+ get contractID() '@mozilla.org/network/protocol/about;1?what=' + this.what,
+ getURIFlags: function(uri) Ci.nsIAboutModule.URI_SAFE_FOR_UNTRUSTED_CONTENT
ochameau
ochameau Dec 27, 2011 Member

Do you remember why you used this flag?
It trigger the following platform code, but it is not clear for me what it allows/restrict:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/about/nsAboutProtocolHandler.cpp#109

You may want to add this flag too: Ci.nsIAboutModule.ALLOW_SCRIPT
Otherwise, JS seems to be disabled by default in about pages.
http://mxr.mozilla.org/mozilla-central/source/mobile/xul/components/AboutRedirector.js#92

Gozala
Gozala Jan 16, 2012 Member

Do you remember why you used this flag?
It trigger the following platform code, but it is not clear for me what it allows/restrict:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/about/nsAboutProtocolHandler.cpp#109

If understand description on MDN
correctly this will allow / restrict links from a regular web content. That being said I'm fine with omitting this flag.

Mossop
Mossop Mar 13, 2012 Member

URI_SAFE_FOR_UNTRUSTED_CONTENT means that regular web-pages can link to the new about: page. You should not use this flag if you are giving your page system privileges as I think it opens a potential security hole (certainly a spoofing risk).

@ochameau ochameau and 1 other commented on an outdated diff Dec 27, 2011
packages/api-utils/lib/protocol.js
+});
+exports.AboutHandler = AboutHandler;
+
+const ProtocolHandler = Factory.extend(AbstractHandler, {
+ onResolve: function onResolve() { throw Error('Not implemented') },
+ interfaces: [ 'nsIProtocolHandler' ],
+ get classDescription() 'Protocol handler for "' + this.scheme + ':*"',
+ get contractID() '@mozilla.org/network/protocol;1?name=' + this.scheme,
+ // Feel free to override.
+ allowPort: function(port, scheme) false,
+ defaultPort: -1,
+ // For more information on what these flags mean,
+ // see caps/src/nsScriptSecurityManager.cpp.
+ protocolFlags: Ci.nsIProtocolHandler.URI_NORELATIVE
+ | Ci.nsIProtocolHandler.URI_IS_UI_RESOURCE
+ | Ci.nsIProtocolHandler.URI_DANGEROUS_TO_LOAD,
ochameau
ochameau Dec 27, 2011 Member

Atul used the following flags set:

(Ci.nsIProtocolHandler.URI_NORELATIVE |
 Ci.nsIProtocolHandler.URI_IS_LOCAL_RESOURCE |
 Ci.nsIProtocolHandler.URI_DANGEROUS_TO_LOAD)

Do you remember why you used URI_IS_UI_RESOURCE? If yes, please add comment in source code.
It seems to be redundant with URI_DANGEROUS_TO_LOAD:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIProtocolHandler.idl#191

Gozala
Gozala Jan 16, 2012 Member

I'll remove URI_IS_UI_RESOURCE for the reason you mentioned. Also, I won't add URI_IS_LOCAL_RESOURCE as it provides async API and users may actually pipe remote data.

@ochameau ochameau commented on an outdated diff Dec 27, 2011
packages/api-utils/lib/protocol.js
+ interfaces: [ 'nsIProtocolHandler' ],
+ get classDescription() 'Protocol handler for "' + this.scheme + ':*"',
+ get contractID() '@mozilla.org/network/protocol;1?name=' + this.scheme,
+ // Feel free to override.
+ allowPort: function(port, scheme) false,
+ defaultPort: -1,
+ // For more information on what these flags mean,
+ // see caps/src/nsScriptSecurityManager.cpp.
+ protocolFlags: Ci.nsIProtocolHandler.URI_NORELATIVE
+ | Ci.nsIProtocolHandler.URI_IS_UI_RESOURCE
+ | Ci.nsIProtocolHandler.URI_DANGEROUS_TO_LOAD,
+ /**
+ * Property describe how to normalize an URL.
+ * @see https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIStandardURL#Constants
+ */
+ type: 1,
ochameau
ochameau Dec 27, 2011 Member

I think it would be easier to catch if you provide all three values and use the original name, like this:

URLTYPE_STANDARD: 1,
URLTYPE_AUTHORITY: 2,
URLTYPE_NO_AUTHORITY: 3,

And later do var url = StandardURL(this.URLTYPE_STANDARD, this.defaultPort, relative, charset, base);
Or remove type and just do:
var url = StandardURL(Ci.nsIStandardURL.URLTYPE_STANDARD, this.defaultPort, relative, charset, base);

ochameau
ochameau Dec 27, 2011 Member

Oh I think I miss the point of this attribute. You would like to offer a way to overload type by inheritance?
If that's the case, can you choose a better name, and give some hints about this value, like this:
urlType: Ci.nsIStandardURL.URLTYPE_STANDARD

@ochameau ochameau was assigned Feb 10, 2012
Member
Gozala commented Mar 13, 2012

@ochameau After updating this pr to use our refined xpcom module I'm no longer convinced that it was good idea to follow XPCOM closely, as this ended up being more xpcomy then desired and originally implemented and I'm afraid that it may be end up to be inherent problem for other APIs as well :(

This patch is still requires some additional tests and docs, but early feedback is welcome!

@Gozala Gozala closed this Nov 26, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment