I'd like to prototype this during my 10% time ;-) Allow plugins to be written in JS. When the jsplugin is installed, it will register for certain MIME types, similarly to binary plugins. When we decide a jsplugin needs to be instantiated, create an iframe and load the jsplugin into that document. The data="" stream can be accessed via a global XMLHttpRequest object. The jsplugin can use some combination of HTML/SVG/canvas to actually render something interesting. By default I think the jsplugin should start out with no special privileges beyond being same-origin with the website that loads the data. Dunno yet. This is very labs-y, and I might even try implementing it in XBL to start out.

What's a usage scenario for this?

Writing a PDF viewer, or a MusicXML viewer, or even MNG viewer if you're crazy enough.

Is there a significant advantage over just using a Web site and registerContentHandler?

I really don't want to give any website access to all my PDF files, and in some cases may be legally prevented from doing so. This seems like an easy way (much easier than XBL or a network service) for adding rendering capabilities to a browser in a way that uses existing web technologies to actually do the rendering/interaction.

Is there really a security difference between giving a Web site access to all your PDF files, and downloading someone's JS file and giving it access to all your PDF files? I see that the law might draw such a distinction, but I'm not convinced it exists in reality.

Given that I can read the JS file (or have other people read it) before installing it, and decide whether to install an updated version, and viewmypdf.com can change at any time, yes.

I can see that you might want to having things running locally in case you're disconnected or for performance reasons. So we'd want to make sure that you could use registerContentHandler with the HTML5 offline appcache, and it's entirely possible that that would require some spec and implementation work. But it seems to me that would give you the same functionality as a JS plugin, but leveraging the Web programming model more effectively.

(In reply to comment #6) > Given that I can read the JS file (or have other people read it) before > installing it, and decide whether to install an updated version, and > viewmypdf.com can change at any time, yes. Maybe we should have a way to disable automatic updates to HTML5 offline apps, then.

web content handlers work fine for when I want the data (iCal/etc) to actually be handled by a website. I don't think it makes sense to try shoehorning all rendering/display tasks into that model. In any case, this is an experiment.

This seems like a really good idea for Firefox OS support for further filetypes/viewers.

Note that this already somewhat works with the way the <object> tag works - if a media handler is registered for the mime type, it will fall back to document mode, so this will work with pdf.js, for example: > <object type="application/pdf" data="./file.pdf"></object> However, this will be superseded by all native plugins

I'm enhancing the playPreview API to allow this for bug 867626 / bug 840439, patches incoming

[WIP v1] Part 1 - Use nsIPluginTag instead of nsPluginTag for navigator.{plugins,mimetypes}

Here's a mostly functional version of this -- try run: https://tbpl.mozilla.org/?tree=Try&rev=ce31eb2b7f30 The idea here is you use pluginHost::createFakePlugin to create a nsFakePluginTag that has writable fields and a AddMimeType call to make a plugin tag that behaves as you want. FakePluginTags also have registerMode and supersedeExisting flags that let them optionally appear in navigator.plugins and supersede real plugins. (registerMode doesn't quite work in this WIP) Example chrome JS: > var host = Components.classes["@mozilla.org/plugin/host;1"].getService(Components.interfaces.nsIPluginHost); > var fake = host.createFakePlugin("chrome://browser/content/aboutRobots.xhtml"); > fake.addMimeType("application/internets", "totes flash", "swf"); > fake.niceName = "Fake nice name"; > fake.name = "Fake name"; > fake.supersedeExisting = false; > fake.enabledState = Components.interfaces.nsIPluginTag.STATE_ENABLED; Would register a plugin for "application/internets" that would be handled by aboutRobots.xhtml > <embed type="application/robots">

This version explodes the browser significantly less, and doesn't accidentally break all real plugins. Note that you need to set pdfjs.disabled to true, or it will try to use the removed API and the uncaught exception breaks the front end https://tbpl.mozilla.org/?tree=Try&rev=73598bda1c4c

Are these JS-plugins OOP? If not, can we do that?

(In reply to Florian Bender from comment #28) > Are these JS-plugins OOP? If not, can we do that? Since plugins will usually load in-content, making the frame load remote would require nested-remote-iframes to be fully working first. Even then, the plugin can easily use a stub as its handler that opens a remote iframe and sends a content script to it to run OOP, so I think it'd be better to let it be handled at that level.

Exposing an API for getting the parsed plugin-params would be helpful for jsplugins, bug 853995 exists to refactor this code, and putting the necessary functions on MozObjectLoadingContent would solve this for free

Rebased and fixed for some work that landed before it. Bug 964436 and Bug 964435 opened for the streams and plugin-params work, so this just needs some cleanup around registering types with the category manager and some tests to be ready to land.

@yury - I'm looking at fixing the remaining issues with this now, have you had a chance to play with the API in comment 19? The patchset as is repurposes some of the play preview code and removes the remainder, so the old API goes away. This means we have to do one of two things to land: - Update pdf.js and shumway to feature detect the API to use, once they support both, land this - Work on a version of the patch to support both APIs, remove Playpreview once users have migrated Do you have any thoughts on how complicated the former would be from shumway/pdfjs' point of view?

Missed comment 19 build. If I'll be rebuilding it locally do I need all patches/attachments above? I can start migrating PDF.js and Shumway extensions to verify if something is missing.

I was able to build at from the 2014-02-19 tree and patches. The SWFObject detects plugins entry and creates OBJECT with application/x-shockwave-flash mime type, which allows the stream converter to take over and not use createFakePlugin args. However EMBED tag does not work, I guess due to the addons manager disabling it and/or setting "ask to activate" attribute. > Note that you need to set pdfjs.disabled to true, or it will try to use the removed API and the uncaught exception breaks the front end The PDF.js extension has detection of playpreviewMethods before calling them (so you can install it from the mzl.la/pdf-xpi during testing of this API). We can do the same at the PdfJs.jsm. Once the fake plugin API will be finalized, I can add code to extensions (PDF.js and Shumway) to detect and use createFakePlugin to experiment with this bug implementation, if it's easier.

Yury: what do you need next from johns for the jsplugin? Is the EMBED issue in your comment 40 a problem with Shumway or the jsplugin code?

(In reply to Chris Peterson (:cpeterson) from comment #41) > Yury: what do you need next from johns for the jsplugin? Is the EMBED issue > in your comment 40 a problem with Shumway or the jsplugin code? I hope for best case to land these patches soon :) I think the EMBED issue is a problem with jsplugin since I followed API described at comment 19, and addon manager ignores any attempt to re-enable the plugin.

(In reply to Yury Delendik (:yury) from comment #42) > (In reply to Chris Peterson (:cpeterson) from comment #41) > > Yury: what do you need next from johns for the jsplugin? Is the EMBED issue > > in your comment 40 a problem with Shumway or the jsplugin code? > > I hope for best case to land these patches soon :) > > I think the EMBED issue is a problem with jsplugin since I followed API > described at comment 19, and addon manager ignores any attempt to re-enable > the plugin. Yes I can reproduce this, I will fix that and see about getting reviews for landing. Is the only reason for the stream converter to avoid double-opening channels? In that case bug 964435 should remove the need for that, correct? Ideally this API removes the need for JS-plugins to have any special hacks

Yury: comment 43? ^

(In reply to John Schoenick [:johns] from comment #43) > Is the only reason for the stream converter to avoid double-opening > channels? In that case bug 964435 should remove the need for that, correct? > Ideally this API removes the need for JS-plugins to have any special hacks Initially the stream converter was created just to replace PDF (later SWF) content with HTML viewer. It evolved into chrome-to-viewer gate, and then to avoid double-channel opening. So it looks like we can do almost the same thing: chrome privileged plugin content can host non-privileged HTML viewer and poke some holes for the extra functionality. And yes, it looks like bug 964435 will solve our requirements to avoid double channel opening for PDF or SWF content.

Here is experiment with new API https://github.com/mozilla/shumway/pull/1286 . At the moment we are missing destroyFakePlugin function for parity with createFakePlugin

John: what work is left to do before you can land jsplugins (pref'd off)? Yury would like to start porting Shumway and PDF.js to jsplugins.

The patchset as is replaces or obsoletes the playPreview API, so a pref wouldn't work. Since pdfjs/shumway need to feature-detect the API to work on older builds for some time, I think we should plan on getting basic support for the API in both before landing it, but if that is difficult supporting both in gecko for some period wouldn't be impossible. As far as what needs to be done here, I need to fix the known bugs mentioned here, write some tests, and get everything reviewed. Once we have some try builds without brokenness we can ask pdfjs/shumway folks to work on API support or whatnot. I'm busy trying to get the <picture> element landed and up to spec, so I probably can't dedicate time to finishing this up for a few weeks, but I will try to get it done sometime this quarter. If someone wants to volunteer to write a set tests for it in the meantime, that would speed things up ;)

John, I'll change PDF.js as soon as these patches will be ready to land. I can prepare something like https://github.com/mozilla/shumway/pull/1286 for PDF.js so we can test the patches. Please notice that it might break only EMBED elements embedding, neither OBJECT nor direct access to the SWF or PDF shall be afffected, right? Actually, I don't think we have PlayPreview fully working, see bug 965003. This bug shall actually fix this issue... I think

I just talked about this with smaug. We think that the right approach is to have the <object> element associate itself with a chrome docshell when a plugin is loaded. In that case, we'd return null from contentWindow and friends (or possibly hide them entirely). When .src is set, we'd want to tear down the docshell entirely and create a new one, so that any Window references held by the plugin implementations wouldn't start pointing to arbitrary content-controlled windows. Smaug is going to help johns on the implementation here.

See also bug 911444 comment 14 - we should make sure that plugins loaded in iframes and top-level documents get appropriate plugin documents (regardless of how the plugin is implemented), and that the magic in comment 57 applies to the <embed> within the plugin document.

Rebased v2.2 again due to significant non-trivial plugin changes in the past few months

Part 3 split out to bug 1061967 since it's fairly independent and useful to other work

I should've done that a while back, but here's a summary of the discussions about this we had with johns, bent, and mrbkap: The plan is to directly incorporate bug 944929 here, and have all jsplugins run in a single jsplugins process. This process should be a direct child of the main process, so a sibling of all content processes. Not only does that make reasoning about synchronous calls easier, it's also required for correctness at least for Shumway: Flash runs all embedded instances in the same process/thread (so ilooping one of them freezes all others), and content relies on the fact that instances can synchronize with each other without running into multithreading-induced races. For communication between the embedding page and the plugin content, (sync and async) message channels can be mediated via the main process. We don't expect these calls to be performance critical. Should this assumption turn out to be wrong, we can always look into direct sibling-to-sibling communication later. To make this all work, part 4.3 of the patches here has to be changed. To quote johns, this will "need some jsplugins API help to create an iframe in the same process as the rest of the instances of the JS-Plugin, which should be easy since it moves the iframe creation to a central spot in gecko, rather than the current messy XBL binding dance." The part that has to change is this: https://bugzilla.mozilla.org/attachment.cgi?id=8471111&action=diff#a/content/base/src/nsObjectLoadingContent.cpp_sec16 Boris, as discuss previously, it'd be great if we could work with someone on this, as it is critical to Shumway and, for various security reasons, to pdf.js, but nobody on those teams has the required knowledge of this area of the platform. Ping me if you want to discuss additional details.

John, my impression was that you aren't working on this any more. Is that right? Did you find somebody else who was going to pick up the torch? Thanks.

I'll pick this up.

Sorry for the delayed response, I've been pretty busy with moving cities and such! I uploaded the newest version of these from my patch queue which are rebased more recently. AFAIK the remaining work here is: - Ensure the supercedeExisting and registerMode are fully implemented, it's been some months since I wrote this now, but I think a few bits of those don't quite do what their header-documentation suggests fully. - Check remaining FIXME comments and remove or address them, these were comments to myself in the WIP versions but unless they point at a specific issue they can probably just be dropped - Write tests to make sure the API actually works Since this drops the playPreview API, we'll need to land the pdf.js/shumway patches to feature-detect which API to use before this can land -- or rework them to maintain both APIs for a time, which is much messier. Comment 19 has a chrome script that uses the API the patchset provides to create a working fake plugin, and still works properly six rebases later, so hopefully this is not too far from landable. I'm around on IRC and here to answer any questions, though I wont have too much time in the near future to work on these more :(

Also, when tweaking part 4.3 to spawn an OOP iframe instead of the current use-<object>-as-the-iframe approach, be sure to note comment 57 about potential security concerns with how the iframe is created.

> we'll need to land the pdf.js/shumway patches to feature-detect which API to use before > this can land John says on IRC that these are bug 1044767 and bug 1044769. Will need to figure out the actual dep order here...

(In reply to Boris Zbarsky [:bz] from comment #80) > > we'll need to land the pdf.js/shumway patches to feature-detect which API to use before > > this can land > > John says on IRC that these are bug 1044767 and bug 1044769. Will need to > figure out the actual dep order here... If it makes the implementation of jsplugins any easier, we should make pdf.js and shumway support both if at all possible and land that first so we can rip out the current, hacky way they're integrated at the same time.

Right, that's my understanding. So I guess the priority for me here right now should be to make sure that I can give pdf.js and shumway a clear idea of what changes they need to make, so they can go ahead and get started on those....

(In reply to Boris Zbarsky [:bz] from comment #82) > Right, that's my understanding. So I guess the priority for me here right > now should be to make sure that I can give pdf.js and shumway a clear idea > of what changes they need to make, so they can go ahead and get started on > those.... Yes, that would indeed be great.

(In reply to Boris Zbarsky [:bz] from comment #82) > Right, that's my understanding. So I guess the priority for me here right > now should be to make sure that I can give pdf.js and shumway a clear idea > of what changes they need to make, so they can go ahead and get started on > those.... That would be nice. Priority is mostly falls on pdf.js migration (bug 1044769), I can start preparing the patches as soon as prototypes of the jsplugins API will be available (or examples of changes that needs to be made). As soon as pdf.js is done, shumway (bug 1044767) will be easy to accomplish using the latter as the example.

Comment on attachment 8535420 [details] [diff] [review] Part 3 - Move checking for special-cased plugin types to a central spot This got checked in in bug 1061967.

Comment on attachment 8535419 [details] [diff] [review] Part 2 - Cleanup nsPluginHost API This got checked in in bug 1136379.

Bugs that block m3 don't need to also block m4.

This is just a patch that combines all the other patches into one. After applying this patch a plugin should be able to register itself like this: let pluginHost = Cc["@mozilla.org/plugin/host;1"].getService(Ci.nsIPluginHost); let plugin = pluginHost.createFakePlugin("chrome://..."); plugin.niceName = "Foo plugin"; plugin.registerMode = Ci.nsIFakePluginTag.MIME_REGISTER_ALL; plugin.supersedeExisting = true; plugin.addMimeType("application/x-shockwave-flash"); plugin.sandboxScript = "resource://..."; // or chrome:// plugin.enabledState = Ci.nsIPluginTag.STATE_ENABLED; The url passed to createFakePlugin will be loaded in an iframe in a separate process. The url set through .sandboxScript will be loaded in a sandbox in the content process, the sandbox' global will have a property named pluginElement set to the element that caused the plugin instance to be created. There's no way to have a process script loaded once for the plugin (as opposed to per instance), but the iframe can Cu.import a module to achieve the same effect.

Working usage (Shumway) of this API at https://github.com/mozilla/shumway/pull/2294

As a temporary replacement for whitelist, can we add/export a function on the sandbox that will cancel a loading of the handler URL and skip fake plugins? Or, will the calling skipFakePlugins from the sandbox script properly fallback to the native plugin?

I have issues with running it in e10s mode, when content page has more than one object, getting the following errors: IPDL protocol error: Handler for ScreenForBrowser returned error code ###!!! [Parent][DispatchSyncMessage] Error: (msgtype=0xB20009,name=PScreenManager::Msg_ScreenForBrowser) Processing error: message was deserialized, but the handler returned false (indicating failure) ###!!! [Child][MessageChannel] Error: (msgtype=0x20007A,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv .... ###!!! [Child][MessageChannel] Error: (msgtype=0x20007A,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv

Couple of more issues (when running Shumway from comment 97): - the new plugin instance is opened in new child process (when dom.ipc.processCount is set > 1) -- it's expected to load jsplugins into single process. URL to try http://areweflashyet.com/shumway/examples/all.html - the destroyed instances are staying in memory e.g. try to load e.g. http://www.homestarrunner.com/sbemail41.html then http://areweflashyet.com/shumway/examples/all.html (audio from previous movie is still playing)

Comment on attachment 8622993 [details] [diff] [review] Part 1 - Use nsIPluginTag instead of nsPluginTag in cases where it may be nsFakePluginTag Spun parts 1, 4.1, 4.2 off into bug 1178963.

Comment on attachment 8622999 [details] [diff] [review] Part 5 - Strip remaining front-end hooks for PlayPreview Landed in bug 1192831.

I updated to trunk and looked this over. Looks fine to me, there are some fixmes left but I4ll file bugs about those.

Comment on attachment 8844221 [details] [diff] [review] Part 7 - Keep track of the content parent that opened a tab so that ContentProcessManager::GetTopLevelTabParent can walk up to the top level tab parent for a JS plugin tab child This makes use keep track of the ContentParentId/TabId pair for the opener, so that we can walk up the tree even if the ContentParents are not in a parent/child relation. We want to group the iframe for the JS plugins in one process, even if there are multiple content processes. That means the js plugin process doesn't have one logical parent process, so we give it the main process as parent.

Comment on attachment 8844222 [details] [diff] [review] Part 8 - Load js plugins in a separate process This groups the iframes for JS plugins into one process per plugin. I'm not using the grandchild relationship anymore for this process, since there can be multiple content processes and one js plugin process.

Comment on attachment 8844221 [details] [diff] [review] Part 7 - Keep track of the content parent that opened a tab so that ContentProcessManager::GetTopLevelTabParent can walk up to the top level tab parent for a JS plugin tab child Review of attachment 8844221 [details] [diff] [review]: ----------------------------------------------------------------- I looked this over and I'm pretty confused about what Opener means. I guess it's not like window.opener? And it doesn't seem to be the same as "parent", but sometimes it does. Comments would be much appreciated about what this means. In any case, I think Kan-Ru would be a better reviewer. He understands this nested content process stuff much better than me. ::: dom/ipc/ContentProcessManager.cpp @@ +53,5 @@ > + if (!info.mCp) { > + info.mCp = aChildCp; > + } else { > + MOZ_ASSERT(info.mCp == aChildCp); > + MOZ_ASSERT_IF(!!info.mParentCpId, info.mParentCpId == aParentCpId); Why would AddContentProcess be called twice?

Comment on attachment 8844221 [details] [diff] [review] Part 7 - Keep track of the content parent that opened a tab so that ContentProcessManager::GetTopLevelTabParent can walk up to the top level tab parent for a JS plugin tab child Review of attachment 8844221 [details] [diff] [review]: ----------------------------------------------------------------- Since the opener ID is either GetID()/ChildID() when the opener is a content process or 0 when the opener is the chrome process, could we only get the ChildID() in chrome process? That way we don't need to send one extra parameter (potentially incorrect one). ::: dom/ipc/ContentParent.cpp @@ +4077,5 @@ > { > TabId tabId; > if (XRE_IsParentProcess()) { > ContentProcessManager* cpm = ContentProcessManager::GetSingleton(); > + tabId = cpm->AllocateTabId(aOpenerCpId, aOpenerTabId, aContext, aCpId); Change this to cp->AllocateTabId(ContentParentId(0), aOpenerTabId, aContext, aCpId); @@ +4114,5 @@ > const IPCTabContext& aContext, > const ContentParentId& aCpId, > TabId* aTabId) > { > + *aTabId = AllocateTabId(aOpenerCpId, aOpenerTabId, aContext, aCpId); And this to cpm->AllocateTabId(ChildID(), aOpenerTabId, aContext, aCpId); ::: dom/ipc/ContentProcessManager.cpp @@ +53,5 @@ > + if (!info.mCp) { > + info.mCp = aChildCp; > + } else { > + MOZ_ASSERT(info.mCp == aChildCp); > + MOZ_ASSERT_IF(!!info.mParentCpId, info.mParentCpId == aParentCpId); If we reuse processes then it is possible to AddContentProcess twice. I wonder if we could avoid this in the caller but this looks ok to me. I like the assertions here.

Is there/will there be something like XRE_IsContentProcess to allow code to check/assert that it is running in a JSPlugin process?

Comment on attachment 8844222 [details] [diff] [review] Part 8 - Load js plugins in a separate process Review of attachment 8844222 [details] [diff] [review]: ----------------------------------------------------------------- It would make me very happy if you would write a patch to rename "fake" plugins to "JSPlugin" or something. Overall, this looks fine, but I'd like to go over it again. I didn't really look at the tab ID/layer ID stuff. Can you explain more what's going on there? Is it related to the fact that the JS plugin process has parent ID 0? ::: dom/base/nsFrameLoader.cpp @@ +313,5 @@ > > nsCOMPtr<nsIDocument> doc = mOwnerContent->OwnerDoc(); > > + nsresult rv; > + if (!IsForJSPlugin()) { Maybe comment that JS plugins can be loaded by anyone, and they always load the same URI (I'm guessing). ::: dom/base/nsFrameLoader.h @@ +79,5 @@ > public: > static nsFrameLoader* Create(mozilla::dom::Element* aOwner, > nsPIDOMWindowOuter* aOpener, > + bool aNetworkCreated, > + int32_t aJSPluginID = -1); Could the invalid value be a const somewhere? ::: dom/base/nsObjectLoadingContent.cpp @@ +73,5 @@ > #include "nsPluginFrame.h" > #include "nsDOMClassInfo.h" > #include "nsWrapperCacheInlines.h" > #include "nsDOMJSUtils.h" > +#include "xpcprivate.h" What's this for? @@ +598,5 @@ > +} > + > +// Helper to spawn the frameloader and return a pointer to its docshell. > +already_AddRefed<nsIDocShell> > +nsObjectLoadingContent::SetupDocshell(nsIURI *aRecursionCheckURI) * should go with nsIURI. ::: dom/base/nsObjectLoadingContent.h @@ +537,5 @@ > * it to mFinalListener > */ > bool MakePluginListener(); > > + void SetupFrameLoader(uint32_t aJSPluginId=0); Spaces around the = sign. Also, I'm very confused about the values this can take on. In nsFrameLoader, 0 means "valid JS plugin" and -1 means "not a plugin". Here, it seems like 0 means "not a plugin". I think you're going to make the frameloaders for eType_Document plugins act like they're JS plugin frameloaders, which seems wrong to me. @@ +545,5 @@ > * > * @param aURI URI we intend to load for the recursive load check (does not > * actually load anything) > */ > + already_AddRefed<nsIDocShell> SetupDocshell(nsIURI *aRecursionCheckURI); Please capitalize the S in DocShell. Also, the * should go with nsIURI. ::: dom/events/EventStateManager.cpp @@ +1290,5 @@ > if (browserFrame && browserFrame->GetReallyIsBrowser()) { > return !!TabParent::GetFrom(target); > } > > + nsCOMPtr<nsIObjectLoadingContent> olc = do_QueryInterface(target); This whole function (EventStateManager::IsRemoteTarget) seems very complex for what little it does. Could we instead just have it |return !!TabParent::GetFrom(target);|? The only time I could see that failing is if we haven't had a chance to create the TabParent yet. But, in that case, we can't possibly forward events anyway. ::: dom/ipc/ContentBridgeParent.h @@ +138,5 @@ > protected: // members > RefPtr<ContentBridgeParent> mSelfRef; > ContentParentId mChildID; > bool mIsForBrowser; > + bool mIsForJSPlugin; This needs to be initialized to false somewhere. ::: dom/ipc/ContentParent.cpp @@ +745,5 @@ > + sJSPluginContentParents = > + new nsDataHashtable<nsUint32HashKey, ContentParent*>(); > + } > + > + if (!p) { Probably better to reverse this and return p.forget() if p is non-null. Then you can un-indent everything below. @@ +846,5 @@ > *aIsForBrowser = cp->IsForBrowser(); > + *aIsForJSPlugin = cp->IsForJSPlugin(); > + > + if (cp->IsForJSPlugin()) { > + *aTabId = AllocateTabId(ChildID(), I guess you're skipping putting this process in the ContentProcessManager. Reading the code, it seems like JS plugin processes don't have a parent because any content process should be allowed to talk to them. This should be commented somewhere. Maybe this is the right place. @@ +1174,5 @@ > aPriority, > aOpenerTabId, > &cpId, > &isForBrowser, > + &isForJSPlugin, This out param doesn't seem very useful. Can't we get the same information from the context? @@ +1768,5 @@ > bool > ContentParent::ShouldKeepProcessAlive() const > { > + if (IsForJSPlugin()) { > + return true; When do these processes die? ::: dom/ipc/ContentParent.h @@ +688,3 @@ > ContentParent(ContentParent* aOpener, > + const nsAString& aRemoteType) > + : ContentParent(aOpener, aRemoteType, -1) Again, using a const instead of -1 would be better. @@ +1163,5 @@ > > ContentParentId mChildID; > int32_t mGeolocationWatchID; > > + int32_t mJSPluginID; And commenting on the the valid values here would be great. ::: dom/ipc/TabContext.h @@ +127,5 @@ > */ > bool mIsPrerendered; > > + /** > + * Empty comment. ::: dom/plugins/base/nsIPluginTag.idl @@ +73,5 @@ > // plugin. Note that the original data/src value for the plugin is not loaded > // and will need to be requested by the handler via XHR or similar if desired. > readonly attribute nsIURI handlerURI; > + > + readonly attribute unsigned long id; It looks like 0 is a valid ID, so please make a comment to that effect.

(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #115) > > + tabId = cpm->AllocateTabId(aOpenerCpId, aOpenerTabId, aContext, aCpId); > > Change this to cp->AllocateTabId(ContentParentId(0), aOpenerTabId, aContext, > aCpId); How is that correct if this is called from ContentParent::RecvCreateChildProcess? aOpenerTabId is not in the main process, is it?

Is this more what you wanted?

Comment on attachment 8844222 [details] [diff] [review] Part 8 - Load js plugins in a separate process > // <iframe mozbrowser> gets to skip these checks. Please adjust this comment to mention the jsplugin case too. >+ int32_t mJSPluginID; This needs documentation. Especially why it's signed (-1 is the guard value), and what this number means, of anything. >+ if (NS_FAILED(tag->GetId(&id)) || id > PR_INT32_MAX) { Can "id" end up < 0 here? Should that be allowed? Can it have arbitrary values < 0, or should they get clamped to -1? >+ void SetupFrameLoader(uint32_t aJSPluginId=0); Why are we defaulting to 0 here? 0 means "is a JS plugin" as far as the frameloader is concerned, no? Also, this is declared as taking uint32_t, but the one callsite does: >+ uint32_t id; .. >+ SetupFrameLoader(int32_t(id)); which seems pretty odd. Finally, need spaces around '='. And this function need to be documented. >+ if (cp->IsForJSPlugin()) { >+ *aTabId = AllocateTabId(ChildID(), This should document why the special-case is needed. >+ parentId == (cp->IsForJSPlugin() ? ContentParentId(0) : ChildID())) { Needs documentation. Why is this happening? >+ parent != (contentParent->IsForJSPlugin() ? ContentParentId(0) : ChildID())) { And here... >+ static already_AddRefed<ContentParent> >+ GetNewOrUsedJSPluginProcess(uint32_t aPluginID, Needs documentation. >+ : ContentParent(nullptr, nsString(), aPluginID) EmptyString() instead of nsString()? >+ bool SetTabContextForJSPluginFrame(int32_t aJSPluginID); This should really be InitTabContextForJSPluginFrame, right? >+ readonly attribute unsigned long id; Needs docs. The changeset could also use a commit message describing the overall setup, if possible. r- because some of those bits with integer types and default-initializing to 0 don't makes sense to me....

Comment on attachment 8844220 [details] [diff] [review] Part 6 - Return fake plugins in e10s too >+ for (const auto& tag : fakePlugins) { >+ // Don't add the same plugin again. >+ for (const auto& existingTag : mFakePlugins) { This is O(N^2), but I guess we expect this list to be very short? >+ tag.name().get(), Ugh. I guess this is no worse than what we have now, but still... raw char pointers. <sigh>. >+ const char* aNiceName) Why isn't this a const nsACString? mNiceName is already nsCString, and so is the return value of GetNiceFileName(). I actually don't understand how passing tag->GetNiceFileName() to the ctor's last arg works, given that. >+ uint32_t mId; Needs documenting. >+ static uint32_t sNextId; Needs documenting. I can't tell offhand whether the URIParams are right or not... r=me with the above fixed.

Comment on attachment 8844223 [details] [diff] [review] Part 9 - Optionally load a script in a sandbox in the content process for every plugin instance >+ AutoSafeJSContext cx; Like the comments say, "This is deprecated. Please use AutoJSAPI instead". In particular, this will automatically enter the unprivileged junk scope compartment. But why do you need to do that? Seems to me like you should just AutoJSAPI and Init() it with no arguments. >+ nsCOMPtr<nsIContent> content; >+ CallQueryInterface(NS_ISUPPORTS_CAST(nsIImageLoadingContent*, this), >+ getter_AddRefs(content)); nsIContent* content = AsContent(); (It's new since you wrote this code.) >+ sandbox = js::UncheckedUnwrap(sandbox); Why do you need that? It shouldn't be wrapped to start with, I don't think. >+ AutoJSAPI jsapi; >+ if (!jsapi.Init(sandbox)) { This needs to be an AutoEntryScript, since you very much plan to run script under it. >+ rv = nsContentUtils::XPConnect()->EvalInSandboxObject(sandboxScript, nullptr, jsapi.cx(), sandbox, &rval); >+ NS_ENSURE_SUCCESS(rv, rv); This will convert exceptions in the script into exceptions from LoadObject (though with some random nsresult). Should we just return NS_OK from LoadObject here instead? Either way, please document the reasoning. >+ * Optional script to run in a sandbox when instantiating a plugin. The script Document that it's just empty string if there is no script? r=me with the above fixed.

Comment on attachment 8844224 [details] [diff] [review] Part 10 - Only allow JS plugins for Flash and PDF >+ RefPtr<nsFakePluginTag> pluginTag = >+ *mFakePlugins.AppendElement(new nsFakePluginTag(tag.id(), > mozilla::ipc::DeserializeURI(tag.handlerURI()), Weird indent here, and on the following lines, in several ways. r=me with that fixed.

These are just the changes on top of the previous version of the patch to address review comments (attachment 8868055 [details] [diff] [review] already contains these and is the one that will be checked in).

Comment on attachment 8868055 [details] [diff] [review] Part 8 - Load js plugins in a separate process Addresses most of the comments. Added nsFakePluginTag::NOT_JSPLUGIN constant with value -1 for the "is not a JS plugin" value. > I didn't really look at the tab ID/layer ID stuff. Can you explain more > what's going on there? Is it related to the fact that the JS plugin process > has parent ID 0? Yes, I added some comments in the code. JS plugin processes are parented to the main process because we have one process per plugin, but multiple content processes can contain instances of plugins. > When do these processes die? Currently on shutdown. We should probably implement some sort of a timer so that they die sooner if they're not used anymore. I'd like to do this in a followup. > >+ if (NS_FAILED(tag->GetId(&id)) || id > PR_INT32_MAX) { > > Can "id" end up < 0 here? Should that be allowed? Can it have arbitrary > values < 0, or should they get clamped to -1? id is an unsigned int32. There really shouldn't be a way to have a value > PR_INT32_MAX here, so I made nsFakePluginTag::Create return an error if we get that value there and made this a MOZ_ASSERT. > >+ bool SetTabContextForJSPluginFrame(int32_t aJSPluginID); > > This should really be InitTabContextForJSPluginFrame, right? I guess, but the existing init function are all called SetTabContext already. (Attachment 8868898 [details] [diff] contains just the changes I made to address the review comments)

Comment on attachment 8868055 [details] [diff] [review] Part 8 - Load js plugins in a separate process Review of attachment 8868055 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsFrameLoader.cpp @@ +2924,5 @@ > openerContentParent = openingTab->Manager()->AsContentParent(); > } > > // <iframe mozbrowser> gets to skip these checks. > + // iframes for JS plugins also get to skip these checks, we control the URL that gets Should be a period instead of a comma in between "checks" and "we". ::: dom/events/EventStateManager.cpp @@ +1281,5 @@ > } > } > > bool > EventStateManager::IsRemoteTarget(nsIContent* target) { Can you move the brace to its own line while you're here? ::: dom/ipc/ContentParent.cpp @@ +986,1 @@ > cpm->AddContentProcess(cp, this->ChildID()); Would you mind adding a comment to this line saying that the process has already been added, and now we're just setting the parent ID. I got confused while reading over this code. @@ +1009,2 @@ > if (cpm->GetParentProcessId(cp->ChildID(), &parentId) && > + parentId == (cp->IsForJSPlugin() ? ContentParentId(0) : ChildID())) { I think it might be good to split this logic into a helper method. Maybe something like: bool ContentParent::CanCommunicateWith(ContentParentId aOtherProcess) { // Normally a process can only communicate with its parent. // But a JS plugin process can communicate with any process. ContentProcessManager *cpm = ContentProcessManager::GetSingleton(); ContentParentId parentId; if (!cpm->GetParentProcessId(ChildID(), &parentId)) { return false; } if (IsForJSPlugin()) { return true; } return parentId == aOtherProcess; } From here you could do |cp->CanCommunicateWith(ChildID())|. From RecvAllocateLayerTreeId you could do |contentParent->CanCommunicateWith(ChildID())|. @@ +1729,5 @@ > GPUProcessManager* gpu = GPUProcessManager::Get(); > > + ContentProcessManager* cpm = ContentProcessManager::GetSingleton(); > + RefPtr<ContentParent> contentParent; > + if (ChildID() == aCpId) { I don't think this branch will ever be true. We only send DeallocateLayerTreeId in the nested content process case. We send it from the content process that owns the frame to the UI process. And aCpId will be the process that renders the frame, which will not be the one that owns it. I suspect that this code (before your patch) has never been tested with nested content processes and is incorrect. As best as I can tell, this->OtherPid() was never the right value to pass to IsLayerTreeIdMapped. @@ +1738,5 @@ > + return IPC_FAIL(this, "Spoofed DeallocateLayerTreeId call"); > + } > + > + ContentParentId parent; > + if (!cpm->GetParentProcessId(contentParent->ChildID(), &parent) || Could we do another CanCommunicateWith check here instead of what you have? I think that would fix this for the normal nested content process case as well as for JS plugins.

https://hg.mozilla.org/integration/mozilla-inbound/rev/675c893a028dfe609ee6812cba0d1e86b2853d23 Bug 558184 - Part 4.3 - Load fake plugin handlers as eType_FakePlugin in nsObjectLoadingContent. r=peterv. https://hg.mozilla.org/integration/mozilla-inbound/rev/0aa594d8fb39d096c685a04ca69e1b68e081952d Bug 558184 - Part 6 - Return fake plugins in e10s too. r=bz. https://hg.mozilla.org/integration/mozilla-inbound/rev/e69bf3dbf73c58fb3e85ab95d8cb18a70aad6914 Bug 558184 - Part 7 - Keep track of the content parent that opened a tab so that ContentProcessManager::GetTopLevelTabParent can walk up to the top level tab parent for a JS plugin tab child. r=kanru. https://hg.mozilla.org/integration/mozilla-inbound/rev/b31942dbee848f90041776eb0da2867b8ae9f59e Bug 558184 - Part 8 - Load js plugins in a separate process. r=billm. https://hg.mozilla.org/integration/mozilla-inbound/rev/d04876d5e3c6859b0224f36b8f7e1090cba3d56e Bug 558184 - Part 9 - Optionally load a script in a sandbox in the content process for every plugin instance. r=bz. https://hg.mozilla.org/integration/mozilla-inbound/rev/afc28100c299bca3d7ee88121c815d9dccf9ae52 Bug 558184 - Part 10 - Only allow JS plugins for Flash and PDF. r=bz.

https://hg.mozilla.org/mozilla-central/rev/675c893a028d https://hg.mozilla.org/mozilla-central/rev/0aa594d8fb39 https://hg.mozilla.org/mozilla-central/rev/e69bf3dbf73c https://hg.mozilla.org/mozilla-central/rev/b31942dbee84 https://hg.mozilla.org/mozilla-central/rev/d04876d5e3c6 https://hg.mozilla.org/mozilla-central/rev/afc28100c299

So this patch broke `window.print()` in Firefox for PDF documents? Currently, Firefox is the **only** browser that does not support `print()` for `<iframe type="application/pdf" src="some.pdf"></iframe>` and `<object type="application/pdf" src="some.pdf"></object>` https://github.com/mozilla/pdf.js/issues/10290 https://bugzilla.mozilla.org/show_bug.cgi?id=911444

> So this patch broke `window.print()` in Firefox for PDF documents? This patch landed in June 2017. Bug 911444 was filed in 2013. So no, this did not cause bug 911444, unless time travel was involved. More to the point, the PDF renderer doesn't use this infrastructure at all. If it started doing that, that might _fix_ bug 911444, which is why the dependency is there.

>More to the point, the PDF renderer doesn't use this infrastructure at all. If it started doing that, that might _fix_ bug 911444, which is why the dependency is there. That is great news! Who do I need to nag to get some Firefox dev eyes in 911444? I suspect that the people working on https://github.com/mozilla/pdf.js/ can not do anything about it? PS. I do wish time travel was an option here

> Who do I need to nag to get some Firefox dev eyes in 911444? That's part of the problem. The pdf viewer is basically unowned and has been for a while... I'll see if I can find someone who knows something about it, but chances are slim.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #138) > I'll see if I can find someone who knows something about it, but chances are > slim. Yury and Brendan both would know things about pdf.js.

Clearing the needinfo here and carrying the conversation over in bug 911444.