This stems from the following report in an email from Andrew Zitnay, who works on LastPass: ---- Within a Chrome content script, I'm able to document.createElement('iframe') and then set its src directly to something like: chrome-extension://hdokiejnpimakedhajhdlcegeplioahd/popupfilltab.html However, within a Firefox page-mod script, if I try to do the same and then set its src directly to something like: resource://support-at-lastpass-dot-com/lastpass/data/popupfilltab.html I get a security error: Security Error: Content at https://fieldtrials.tivo.com/login.html may not load or link to resource://support-at-lastpass-dot-com/lastpass/data/popupfilltab.html. --- I talked to bz about this and the reason we restrict resource: URIs and he said that they have certain privileges: <bz> but they can sync-load XBL bindings from resource: and chrome: <bz> They may be able to read file: URIs; I can't recall I also looked at how Chrome handles this stuff. https://developer.chrome.com/extensions/manifest/web_accessible_resources They require that these resources be called out in the extension manifest. That avoids some fingerprinting issues. So I think what we want is a new URI scheme that is totally unprivileged, that allows the same sort of whitelisting that Chrome has, but is otherwise similar to resource:. Then we could use this scheme for Jetpack, hopefully.

Possibly a dupe of bug 792479 or bug 820213. I think I had a WIP patch lying around somewhere for this sort of thing too...

No idea how far I got here: http://hg.oxymoronical.com/mozilla/queues/general/file/8499c6f8133e/protocol

Bobby, this is the second bug we talked about.

I was hoping to get to this this week but didn't - too many things came up. Sorry about that.

(In reply to Dave Townsend [:mossop] (PTO till July 21st) from comment #2) > No idea how far I got here: > http://hg.oxymoronical.com/mozilla/queues/general/file/8499c6f8133e/protocol Mossop, is there any reason you opted to write it from scratch in JS, rather than just generalizing the existing code for resource:// URIs? I'm inclined to do the latter, but I just want to make sure that I'm not missing any problems with that approach.

Dave's on PTO for another week and I think unreachable. I also think we should proceed with the C++-based approach.

(In reply to Bobby Holley (:bholley) from comment #5) > (In reply to Dave Townsend [:mossop] (PTO till July 21st) from comment #2) > > No idea how far I got here: > > http://hg.oxymoronical.com/mozilla/queues/general/file/8499c6f8133e/protocol > > Mossop, is there any reason you opted to write it from scratch in JS, rather > than just generalizing the existing code for resource:// URIs? I'm inclined > to do the latter, but I just want to make sure that I'm not missing any > problems with that approach. At the time I don't think the SDK was shipping with Firefox so I wanted to write something standalone that would work in multiple Firefox versions. Don't think there was any other reason.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc921d239af4 This is green, but is the base refactoring only. Pushing the whole thing shortly.

Full changes, with tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e6714639929

This allows us to have a shared superclass that implements the guts of a shared superinterface, without having the superclass actually inherit the superinterface (which leads to annoying and unnecessary diamond-inheritance). The approach certainly hacky, but I think it's better than generalizing all of the machinery that we're currently using to make these declarations. This stuff isn't mission critical, and the compiler will tell us if it breaks.

As far as I can tell, this thing isn't threadsafe at all.

The heavy lifting all happened in the previous patch, so this is easy now.

Comment on attachment 8636315 [details] [diff] [review] Part 3 - Factor the sharable bits out of nsIResProtocolHandler. v1 Sorry, meant for this to be sr.

Comment on attachment 8636316 [details] [diff] [review] Part 4 - Implement moz-extension protocol. v1 Also flagging bz on the caps piece.

Comment on attachment 8636313 [details] [diff] [review] Part 1 - Generate an extra macro to declare a non-virtual variant of an interface. v1 Review of attachment 8636313 [details] [diff] [review]: ----------------------------------------------------------------- I discovered we already have a macro for the change I mentioned: NS_METHOD. It's just a nonvirtual version of NS_IMETHOD. ::: xpcom/idl-parser/xpidl/header.py @@ +438,2 @@ > elif not member.kind in ('attribute', 'method'): > + memberDecls += ('\\') Let's instead factor this out into a function that takes either NS_IMETHOD or NS_METHOD as a param. That would get passed down to attributeAsNative or methodAsNative. @@ +440,5 @@ > + > + fd.write(iface_epilog % names) > + fd.write(memberDecls) > + fd.write(iface_nonvirtual % names) > + fd.write(memberDecls.replace("NS_IMETHOD", "nsresult") And then get rid of this.

Comment on attachment 8636314 [details] [diff] [review] Part 2 - Stop using threaddsafe ISupports for nsResProtocolHandler. v1 Review of attachment 8636314 [details] [diff] [review]: ----------------------------------------------------------------- It just uses optimistic concurrency control :-).

Comment on attachment 8636315 [details] [diff] [review] Part 3 - Factor the sharable bits out of nsIResProtocolHandler. v1 Review of attachment 8636315 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/res/SubstitutingProtocolHandler.cpp @@ +63,5 @@ > + // when the scheme isn't file. > + if (!scheme.EqualsLiteral("file")) > + return NS_ERROR_NO_INTERFACE; > + > + rv = net_GetFileFromURLSpec(spec, getter_AddRefs(mFile)); Just |return net_...|? @@ +105,5 @@ > +{ > + EnumerateSubstitutionArg(nsCString& aScheme, InfallibleTArray<SubstitutionMapping>& aMappings) > + : mScheme(aScheme), mMappings(aMappings) {} > + nsCString& mScheme; > + InfallibleTArray<SubstitutionMapping>& mMappings; InfallibleTArray -> nsTArray ::: netwerk/protocol/res/SubstitutingProtocolHandler.h @@ +11,5 @@ > +#include "nsInterfaceHashtable.h" > +#include "nsIOService.h" > +#include "nsNetUtil.h" > +#include "nsStandardURL.h" > +#include "nsWeakReference.h" Do you need all of these? nsWeakReference doesn't seem to be used. ::: netwerk/protocol/res/moz.build @@ +10,5 @@ > ] > > XPIDL_MODULE = 'necko_res' > > SOURCES += [ Can this be UNIFIED_SOURCES? ::: netwerk/protocol/res/nsISubstitutingProtocolHandler.idl @@ +1,1 @@ > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ This file uses two-space indent so please make c-basic-offset be 2.

Comment on attachment 8636316 [details] [diff] [review] Part 4 - Implement moz-extension protocol. v1 Why can't the CheckLoadURI bits be handled entirely by the protocol, via a setup similar to what about: uses (or reversed, so that the loadable-by-anyone thing is the moz-extension scheme, depending on which is going to be more common)? Alternately, can we come up with a better setup for use both here and for about:? Followup is probably ok, but hardcoding things like this, expecially when we know we've needed this for multiple protocols now, is pretty icky. I _think_ you'll want a separate dir under network/protocol for the new protocol, but I'll let Patrick make that call. r=me modulo that.

Comment on attachment 8636317 [details] [diff] [review] Part 5 - Forbid mapping to anything but file:// and jar:// URIs. v1 >+ // In general, we expect the the principal of a document loaded from a s/the the/the/ sr=me

Comment on attachment 8636319 [details] [diff] [review] Part 6 - Associate extension URIs with the appropriate addon ID. v1 sr=me

Comment on attachment 8636316 [details] [diff] [review] Part 4 - Implement moz-extension protocol. v1 Review of attachment 8636316 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/nsIAddonPolicyService.idl @@ +21,5 @@ > boolean addonMayLoadURI(in AString aAddonId, in nsIURI aURI); > + > + /** > + * Returns true if a given extension:// URI is web-accessible. > + */ Formatting.

Comment on attachment 8636320 [details] [diff] [review] Part 7 - Tests. v1 Review of attachment 8636320 [details] [diff] [review]: ----------------------------------------------------------------- One thing this doesn't test is that chrome code can load these URIs unconditionally. The code I use to do this is at https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1175770&attachment=8623994 in BrowserExtension.jsm around line 492 to 521. Assuming principal is changed to include the add-on ID, will my code work (even in the loadURI case)? That's the main thing I care about :-).

Comment on attachment 8636635 [details] [diff] [review] Part 1 - Generate an extra macro to declare a non-virtual variant of an interface. v2 Review of attachment 8636635 [details] [diff] [review]: ----------------------------------------------------------------- Just a few suggestions here. ::: xpcom/idl-parser/xpidl/header.py @@ +61,5 @@ > > return ", ".join(l) > > > +def attributeAsNative(a, getter, virtual): How about using a default value for the virtual parameter (and for methodAsNative)? Also, it might be clearer to pass in either the string 'NS_IMETHOD' or 'NS_METHOD'. That way we don't have calls with a series of boolean params. @@ +424,5 @@ > raise Exception("Unexpected interface member: %s" % member) > > fd.write(iface_epilog % names) > > + def writeDeclaration(fd, iface, virtual): Maybe do something here like: override = " override" if virtual else "" and then just use that variable below. Also, it would be nice if you could use the "X if B else Y" expression instead of "B and X or Y" throughout the patch. Conditional expressions were added in Python 2.5 (~2006) so I think we can depend on them.

(In reply to Bill McCloskey (:billm) from comment #24) > > +{ > > + EnumerateSubstitutionArg(nsCString& aScheme, InfallibleTArray<SubstitutionMapping>& aMappings) > > + : mScheme(aScheme), mMappings(aMappings) {} > > + nsCString& mScheme; > > + InfallibleTArray<SubstitutionMapping>& mMappings; > > InfallibleTArray -> nsTArray Hm, how come? The callback doesn't check for append failure, so it seems like we should use the stronger type that lets us skip it.

(In reply to Bobby Holley (:bholley) (PTO July 22 - July 26) from comment #32) > Hm, how come? The callback doesn't check for append failure, so it seems > like we should use the stronger type that lets us skip it. http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArrayForwardDeclare.h#36

(In reply to Bill McCloskey (:billm) from comment #33) > http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/ > nsTArrayForwardDeclare.h#36 Ah I see - thanks!

(In reply to Bill McCloskey (:billm) from comment #29) > Comment on attachment 8636320 [details] [diff] [review] > Part 7 - Tests. v1 > > Review of attachment 8636320 [details] [diff] [review]: > ----------------------------------------------------------------- > > One thing this doesn't test is that chrome code can load these URIs > unconditionally. The code I use to do this is at > https://bugzilla.mozilla.org/page.cgi?id=splinter. > html&bug=1175770&attachment=8623994 in BrowserExtension.jsm around line 492 > to 521. Assuming principal is changed to include the add-on ID, will my code > work (even in the loadURI case)? That's the main thing I care about :-). I've added a test for navigating from chrome with .location, and with nsIWebNavigation (which is what loadURI does).

https://hg.mozilla.org/integration/mozilla-inbound/rev/f659ec819bf6 https://hg.mozilla.org/integration/mozilla-inbound/rev/cd49cae74a81 https://hg.mozilla.org/integration/mozilla-inbound/rev/7b078b5605a1 https://hg.mozilla.org/integration/mozilla-inbound/rev/9b10ef08bfbe https://hg.mozilla.org/integration/mozilla-inbound/rev/8843374b8ac7 https://hg.mozilla.org/integration/mozilla-inbound/rev/689fd01b7b3c https://hg.mozilla.org/integration/mozilla-inbound/rev/a37164c4c1bb

Comment on attachment 8636315 [details] [diff] [review] Part 3 - Factor the sharable bits out of nsIResProtocolHandler. v1 I'm assuming that patrick sr-ed what he wanted to sr here.

https://hg.mozilla.org/mozilla-central/rev/f659ec819bf6 https://hg.mozilla.org/mozilla-central/rev/cd49cae74a81 https://hg.mozilla.org/mozilla-central/rev/7b078b5605a1 https://hg.mozilla.org/mozilla-central/rev/9b10ef08bfbe https://hg.mozilla.org/mozilla-central/rev/8843374b8ac7 https://hg.mozilla.org/mozilla-central/rev/689fd01b7b3c https://hg.mozilla.org/mozilla-central/rev/a37164c4c1bb

(In reply to (PTO July 22 - July 26) from comment #37) > Comment on attachment 8636315 [details] [diff] [review] > Part 3 - Factor the sharable bits out of nsIResProtocolHandler. v1 > > I'm assuming that patrick sr-ed what he wanted to sr here. heh :) for the record I got cut off from my hotel network before I could continue.. I'm good with it.

I see that this bug is marked as resolved fixed. Does that mean it's now live, in nightly at least? If so, is there any documentation on how to use it?

(In reply to Andrew Zitnay from comment #40) > I see that this bug is marked as resolved fixed. Does that mean it's now > live, in nightly at least? If so, is there any documentation on how to use > it? Please don't use this machinery directly. Instead, use the new Browser Extension API, see https://wiki.mozilla.org/Browser_Extensions Bill, is that API ready for LastPass et al?

Interesting. This is the first I'd heard of Mozilla considering more or less supporting Chrome extensions. I honestly have my doubts, though, that it'll be implemented fully enough for us to consider using any time in the near future. So, am I incorrect that this should be usable by the add-on SDK directly? Bill did say "Then we could use this scheme for Jetpack, hopefully." in the initial comment.

Also, I noticed "webAccessibleResources" on that page. The correct manifest.json entry is "web_accessible_resources". Not sure if it's just a typo on that page, or a mistake within the API as well.

The work I did here was specifically target towards the new extension API, and wasn't intended to be generally usable. I'll let bill comment on the rest.

(In reply to Andrew Zitnay from comment #42) > Interesting. This is the first I'd heard of Mozilla considering more or > less supporting Chrome extensions. I honestly have my doubts, though, that > it'll be implemented fully enough for us to consider using any time in the > near future. Our goal is to be able to support LastPass with this API. It's not quite ready yet, but I think in a few weeks we'll have everything you guys need. One thing I'm not sure about is how to deal with deprecated APIs. LastPass uses some deprecates messaging APIs in chrome.extension. I also see references to chrome.toolstrip, which isn't documented anywhere I can find. Is this stuff that you really need? > So, am I incorrect that this should be usable by the add-on SDK directly? > Bill did say "Then we could use this scheme for Jetpack, hopefully." in the > initial comment. Our plans have changed since I wrote that. We may end up supporting the new URI scheme in Jetpack, but it will take some time. The Jetpack code is a lot harder to work on than I realized, and we don't have many people around who understand it. We're much more enthusiastic about supporting this new API in the future. I realize that puts you guys in a bad position since you recently ported to Jetpack. However, the new API has a lot of advantages. It means you'll only be supporting one codebase. Also, if we do our jobs, the "porting" process should take only a few hours.

As for the deprecated APIs in chrome.extension, I think we leave them in there so that we don't break compatibility with old versions of Chrome. We could certainly call the various chrome.runtime APIs instead if they're available, falling back to the deprecated APIs if not. As for chrome.toolstrip, it was more or less the predecessor to chrome.browserAction, and has been deprecated since 2009 or so. Although references to it still exist in our code, we shouldn't be actually calling into it. As for the rest of the chrome APIs, the only one I see that we're using that isn't listed as being supported, now or in the future, is chrome.management. Other concerns I can think of: - It sounds like, best case, this API would only be supported starting with Firefox 42. This means it wouldn't work in previous versions of Firefox. Last I checked, our add-on SDK extension more or less worked well all the way back to Firefox 30. Obviously, we could direct users of older versions of Firefox to our old extension, but that's not ideal. - The main reason we started looking into the add-on SDK was e10s compatibility. According to the e10s schedule, it's still possible that it could be released with Firefox 42 (although I have my doubts). That doesn't leave much of a window for implementing this. - We'll most certainly need a way to call into a binary component for some more advanced features. In Chrome, we currently do this via native messaging, which Firefox doesn't support. In Firefox, we currently do this via js-ctypes. I greatly prefer js-ctypes, mostly because it allows us to bundle the binary components inside the extension. Will there be a way to access js-ctypes from this new API? - Our Chrome extension makes moderate use of web SQL database, which Firefox of course doesn't support. For the add-on SDK, I'm currently using mozIStorageService via require('chrome'). Correct me if I'm wrong, but I'm guessing this new API won't have access to Components.*. If so, I guess we might have to look into IndexedDB instead. - What's the plan for Fennec? Currently, it supports a subset of the add-on SDK, which we work around for UI purposes by pulling NativeWindow out of require('chrome'). Will this new API have access to Fennec's NativeWindow? - The last thing I can think of is support for filling into and retrieving credentials from HTTP basic authentication windows. Firefox has always offered us the best experience here, because we're able to modify the XUL directly to add a select box to choose a site to fill, capture the credentials when they're submitted, etc. I'm guessing this new API wouldn't really support that. In Chrome, we have to resort to calling into binary and using accessibility APIs, which is platform-specific (i.e. we don't have a solution for Linux at all right now), and much clunkier.

I thought of at least one more... The add-on SDK version of our extension uses sdk/preferences/service to get/set arbitrary preferences (both to import preferences from our old XUL-based extension, and to turn off Firefox's built-in password manager). I'm guessing that functionality won't be a part of the new API. I'm sure there will be more stuff I'd come across.

OK, I filed bug 1194436 for further Jetpack work.

It's worth noting that I did find a workaround for our incompatibility with uBlock, so my need for this isn't very pressing at the moment.