675372 - allow resource protocol mappings in chrome.manifest files loaded dynamically

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Garrett]

Bug 564667 added the capability to load/unload a chrome.manifest file dynamically for bootstrapped addons. This followup bug is for adding support for resource protocol mappings in chrome.manifest files loaded in this manner. While chrome mappings are often far more useful, resource mappings do have their uses and the closer chrome.manifest support in bootstrapped addons is to the full feature set the fewer issues there will be in porting complex addons to be bootstrapped.

Comment 1

[Bill Gianopoulos [:WG9s]]

Attachment: fix

This seems too simple.  Not asking for review yet.

Comment 2

[Dave Townsend [:mossop]]

Yeah, registering the resource uri was never the problem. It's unregistering it after removing chrome.manifest file dynamically which is the problem.

Comment 3

[Bill Gianopoulos [:WG9s]]

Ah I will look into that and if i can figure it out I will do that an write some tests.

Comment 4

[Dave Townsend [:mossop]]

Right now the way it works for chrome mappings is that when you unregister a manifest we wipe the chrome mappings and re-parse all the manifests. That's fine because the only chrome mappings are those in the manifests. The same isn't true for resource mappings.

Comment 5

[Bill Gianopoulos [:WG9s]]

OIC.  This is probably not only something I really don;t have time to do, because I have another job, but actually something not worth it to me to do to fix the minor issue I have in my extension with using chrome: URI's.

Comment 6

[clever]

(In reply to Dave Townsend (:Mossop) from comment #4)
> Right now the way it works for chrome mappings is that when you unregister a
> manifest we wipe the chrome mappings and re-parse all the manifests. That's
> fine because the only chrome mappings are those in the manifests. The same
> isn't true for resource mappings.

sounds like the solution is to parse the chrome.manifest for resource lines, upon shutdown (or save them from startup with the addon state)

and call nsIResProtocolHandler.setSubstitution(prefix,null); to unregister them, i'm attaching an example bootstrap.js that does that

Comment 7

[clever]

Attachment: example of how to unload a resource://

Comment 8

[clever]

hmm, this is actually a much bigger change then i was expecting

the best solution ive been told is to use https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#3688 XPI_unloadBootstrapScope() to detect when the addon has been unloaded
then use ChromeManfestParser.jsm to find all chrome resources in it

then its as simple as this
var resProt = io.getProtocolHandler("resource").QueryInterface(Ci.nsIResProtocolHandler);
resProt.setSubstitution(prefix,null);

and its un-registered!

not sure what will happen to any modules still loaded at this point, but you can find them listed under about:compartments

Comment 9

[Ben Bucksch (:BenB)]

resource:// URLs are critical for JSMs. I don't know how I'm supposed to modularize my source code, if I don't have JSMs.

mfinkle posted a workaround
http://starkravingfinkle.org/blog/2011/01/restartless-add-ons-more-resources/
but many addons will need that, so it's not useful to add that to every addon. More importantly, this code happens within startup(), but I want my import statements at the top of the file, as normal.

Comment 10

[Jonathan Protzenko [:protz]]

Ben, you may be interested in how I do that for Conversations. There's basically a piece of code that everyone is cargo-culting https://github.com/protz/GMail-Conversation-View/blob/master/bootstrap.js#L52 ; then, even though you can't write them at the top of your file, you can use Cu.import statements https://github.com/protz/GMail-Conversation-View/blob/master/bootstrap.js#L137

I think that's mostly ok. At least it's been pretty solid for me.

Comment 11

[Dave Garrett]

(In reply to Ben Bucksch (:BenB) from comment #9)
> resource:// URLs are critical for JSMs. I don't know how I'm supposed to
> modularize my source code, if I don't have JSMs.

No they're not. I'm in the process of converting Flagfox to restartless at this very moment and I've already gotten both of my resource:// mappings switched over to chrome:// mappings and working fine. JSM has loaded from chrome:// fine for a very long time now. You only really need resource:// when you absolutely have to have an nsIFile, near as I can tell. Things you were using input streams for can be converted into typed JS arrays which can be loaded via XMLHttpRequest (which you can get in a JSM too) and accessed with DataView (requires Gecko 15+). It looks like Gecko 17 is the farthest back ESR this is practical, but that's quite far back at this point. (somewhere on MDC said there were some bugs in Gecko 16 with XMLHttpRequest in JSM, but I don't know the specifics on that)

Yeah, you can file this bug under "why the hell did they never fix this?", but this part isn't really needed. If you're going for files in an extractionless addon it's not really viable either.

The docs on the general topic of writing restartless addons kinda suck. I'll write a blog post about what I've learned porting from a Firefox 3.6+ extracted addon to a Firefox 17+ restartless extractionless addon at some point.

Comment 12

[Ben Bucksch (:BenB)]

Thanks for the prompt and helpful tips.

I didn't realize that Cu.import() now works with chrome:// . That's bug 406538. I had been rooting for that seriously for a long time, then gave up. Then forgot to migrate. Great that they work now, I'll try that in both my traditional ext and in the bootstrapped one.

> everyone is cargo-culting

Yeah. And I don't like that. In cases like that, it should be fixed in the platform.

Comment 13

[Dave Garrett]

I've posted a pile of information about writing restartless extensions here:
http://flagfox.wordpress.com/2014/01/19/writing-restartless-addons/

(in part, inspired to write because you pointed out how easy it is to not keep track of what's actually supported here at this point)

Comment 14

[Geoff Lankow (:darktrojan)]

Attachment: 675372-1.diff

This sets the resource substitutions normally when the add-on is enabled (using the usual process), and remove them again on disable, when they're unloaded by the XPIProvider. As this is the only place calling removeBootstrappedManifestLocation I think it's okay to fix it in JS instead of inside the C++ function.

Also, I had trouble making the test install, did I do something wrong?

Comment 15

[Geoff Lankow (:darktrojan)]

(In reply to Geoff Lankow (:darktrojan) from comment #14)
> Also, I had trouble making the test install, did I do something wrong?

Never mind, it worked after I clobbered it.

Comment 16

[Geoff Lankow (:darktrojan)]

Attachment: 675372-test-1.diff

My changes broke test_cacheflush.js by flushing the cache twice. Also run_test_2 didn't make much sense, saying that it expects a flush then checking for no flush (and it could hide a bug in run_test_3).

Comment 17

[Dave Garrett]

I'm not really advocating heavily for one position or another, but are we sure we even want this anymore? The resource:// protocol is nowhere near as needed as it used to be and generally considered something to avoid. (it still does leak onto the web, I believe; it's a privacy hole) I was under the impression that they're effectively deprecated. This feels like it either should've been done properly at the beginning, or not at all now. At this point, chrome:// should be the way to go for almost everything. For remaining edge cases, manually registration of resource:// URLs should be viable.

Was there a specific reason this bug was resurrected? (and, is it something that chrome:// or another scheme can't do better?) It's long past the time where I would've thought it was appropriate to change the now established behavior here.

I'd like to request that whomever is in charge of whatever module owns resource:// protocol mappings (or whomever closest fits that bill) please review whether or not this is still wanted. I expected this to be WONTFIXed a year ago, not patched three and a half years after I filed this.

I have no idea who to directly ask, so needinfoing current reviewer to please investigate this topic.

Comment 18

[Jonathan Protzenko [:protz]]

The reason I'm using resource:// in my addon is that I have some outer (privileged) UI loaded from chrome:// and some inner (content) UI loaded inside an iframe using the resource:// protocol. Is there a more canonical way to do this without using resource://? (For some reason I can't recall, xul:iframe with the right content flag was not an option.) If not, that might explain why the bug is still open.

Comment 19

[Dave Garrett]

You should be able to load a content iframe from chrome:// so long as the page containing the frame is also chrome, I think. If the page cannot have chrome privileges then resource:// might be needed in this case. Based on your description, I think chrome:// might be viable for you. (setting the iframe to the appropriate privileges, of course)

I do not dispute that there may be some legitimate use cases for resource:// URIs, however I don't think they're enough to justify adding fully automatic loading of them from the manifest at this very late stage. You can manually map them at runtime instead. There might be an argument for making this simpler to do with some helper. (I don't remember what's involved off-hand) Generally though, usage of resource:// is often discouraged where possible.

Comment 20

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8549491 [details] [diff] [review]
675372-1.diff

Review of attachment 8549491 [details] [diff] [review]:
-----------------------------------------------------------------

resource:// URIs are in no way deprecated - they're just used less by restartless add-ons because of this bug. In mozilla-central, we're using them as we always have.

Add-ons can register resource:// mappings themselves... but they shouldn't have to. There's an established way to do it automatically - if we can get that working, it should be preferred over add-ons re-implementing. More implementations = more bugs. And there are a *lot* of restartless add-ons re-implementing this, which concerns me.

Comment 21

[Geoff Lankow (:darktrojan)]

Pushed both patches as one revision: https://hg.mozilla.org/integration/fx-team/rev/2ebdbe5cdeb5

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2ebdbe5cdeb5

Comment 23

[Dave Garrett]

(In reply to Blair McBride [:Unfocused] from comment #20)
> And there are a *lot* of restartless add-ons
> re-implementing this, which concerns me.

Fixing this bug is very unlikely to affect this situation at all. Not only do addon developers not always know about these changes, but in order to continue supporting older versions of Firefox they cannot use them. The amount of copy/paste work is also quite high. I doubt this will cause many developers to update things for years. :/