Based on Anant and Kyle's initial version but with features disabled on the docshell and stylesheets disabled. Since we are exposing the IDP DOM APIs to all webpages, I think we don't need the Cu.Sandbox code which is commented out, correct? Also, I looked at bug 745415 but don't see where the problem was addressed. Is there anything else which should be done?

This is intended to be the iframe we load the provisioning site in, correct?

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #2) > This is intended to be the iframe we load the provisioning site in, correct? correct!

Comment on attachment 632087 [details] [diff] [review] WIP Sandbox.jsm for Identity Review of attachment 632087 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me! Just a few nits: ::: toolkit/identity/Sandbox.jsm @@ +41,5 @@ > + */ > + get id() { > + return this._frame.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDOMWindowUtils).outerWindowID; > + }, I'm not sure what the convention for indentation is generally, but the .getInterface on the following line looks a little off (occurs later as well). @@ +46,5 @@ > + > + /** > + * Load or reload the url > + */ > + load: function Sandbox_load() { This function isn't used anywhere. The caller creating the sandbox provides the URL in the constructor which is then loaded by _createSandbox, also called by the constructor. What is the purpose of this function? @@ +116,5 @@ > + this._sandbox = new Cu.Sandbox(workerWindow, { > + wantXrays: false, > + sandboxPrototype: workerWindow > + }); > + */ Probably best to simply remove the entire block. We might need to provide the sandbox to the caller at a later point, but we don't need it for the provisioning case. @@ +133,5 @@ > + ); > + > + }, > + > + _log: function Sandbox__log(msg) { Consider using log4moz at some point.

Comment on attachment 632087 [details] [diff] [review] WIP Sandbox.jsm for Identity Review of attachment 632087 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine. Just one nit. ::: toolkit/identity/Sandbox.jsm @@ +18,5 @@ > + * callback provided to the constructor will be invoked when the sandbox is > + * ready to be used. The callback will receive this object as its only argument. > + * > + * Please call free() when you are finished with the sandbox to explicitely free > + * up all associated resources. Change this from "Please" to "You must". Also, there's a typo in 'explicitly'.

Anant, I looked at bug 745415 but don't see where the problem was addressed; could you point me to it? Here's a bunch of tests. test_redirect is not working at the moment. Thanks. (In reply to Anant Narayanan [:anant] from comment #4) > Review of attachment 632087 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good to me! Just a few nits: > > ::: toolkit/identity/Sandbox.jsm > @@ +41,5 @@ > > + */ > > + get id() { > > + return this._frame.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor) > > + .getInterface(Ci.nsIDOMWindowUtils).outerWindowID; > > + }, > > I'm not sure what the convention for indentation is generally, but the > .getInterface on the following line looks a little off (occurs later as > well). I aligned the periods now which is what I've seen in other modules. > @@ +46,5 @@ > > + > > + /** > > + * Load or reload the url > > + */ > > + load: function Sandbox_load() { > > This function isn't used anywhere. The caller creating the sandbox provides > the URL in the constructor which is then loaded by _createSandbox, also > called by the constructor. What is the purpose of this function? I've renamed it to "reload" to make it clearer. It is used by the identity code when we re-use a Sandbox after the first provisioning flow fails ( PROV => AUTH => PROV ). This is to maintain the same outer window ID for the 2 provisioning flows since that's what we key the prov. flow off of. It would probably be better to attach some "provId" attribute or something else(?) on the frame/window in a way that nsDOMIdentity can also look it up and use it instead of the outer window ID for provisioning. It was easier for now to just re-use the Sandbox for the 2 prov. flows and then nsDOMIdentity can always use the outer window ID. > @@ +116,5 @@ > > + this._sandbox = new Cu.Sandbox(workerWindow, { > > + wantXrays: false, > > + sandboxPrototype: workerWindow > > + }); > > + */ > > Probably best to simply remove the entire block. We might need to provide > the sandbox to the caller at a later point, but we don't need it for the > provisioning case. Thanks for clarifying. I wasn't sure if it was something I was supposed to be using. > Consider using log4moz at some point. If only bug 451283 would get fixed.

Comment on attachment 634815 [details] [diff] [review] v.1 Address feedback and added tests >diff --git a/toolkit/identity/Sandbox.jsm b/toolkit/identity/Sandbox.jsm >+ _createFrame: function Sandbox__createFrame() { >+ // TODO: What if there is no most recent browser window? (bug 745415). >+ // Or use hiddenWindow This seems like an important issue to solve, particularly if this is to be useful as a general purpose module (we might like to use this for social code as well - see bug 766607). Just using the hidden window should work fine (though you'll need to use createElementNS(XHTML_NS, "iframe"), since the hidden window document can be either xhtml (Win/Linux) or xul (Mac)).

Comment on attachment 634815 [details] [diff] [review] v.1 Address feedback and added tests Review of attachment 634815 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Matthew N. [:MattN] from comment #6) > Anant, I looked at bug 745415 but don't see where the problem was addressed; > could you point me to it? This fix is in this version of the patch: https://bug745345.bugzilla.mozilla.org/attachment.cgi?id=628588 - nothing fancy, just: let aSvc = Cc["@mozilla.org/appshell/appShellService;1"].getService(Ci.nsIAppShellService); let doc = aSvc.hiddenDOMWindow.document; It didn't make it to the final version of the patch, because (IIRC) of the reason Gavin mentions below. > > > + /** > > > + * Load or reload the url > > > + */ > > > + load: function Sandbox_load() { > > > > This function isn't used anywhere. The caller creating the sandbox provides > > the URL in the constructor which is then loaded by _createSandbox, also > > called by the constructor. What is the purpose of this function? > > I've renamed it to "reload" to make it clearer. It is used by the identity > code when we re-use a Sandbox after the first provisioning flow fails ( PROV > => AUTH => PROV ). This is to maintain the same outer window ID for the 2 > provisioning flows since that's what we key the prov. flow off of. It would > probably be better to attach some "provId" attribute or something else(?) on > the frame/window in a way that nsDOMIdentity can also look it up and use it > instead of the outer window ID for provisioning. It was easier for now to > just re-use the Sandbox for the 2 prov. flows and then nsDOMIdentity can > always use the outer window ID. Sounds good! Might help to add some comments to the function to the effect of "this makes it easier to reuse sandboxes". > > Consider using log4moz at some point. > > If only bug 451283 would get fixed. Is adding log4moz to toolkit a prerequisite? All the services code already uses log4moz, so you could, in theory do a Cu.import("resource://services-common/log4moz.js"); no? (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #7) > >+ _createFrame: function Sandbox__createFrame() { > >+ // TODO: What if there is no most recent browser window? (bug 745415). > >+ // Or use hiddenWindow > > This seems like an important issue to solve, particularly if this is to be > useful as a general purpose module (we might like to use this for social > code as well - see bug 766607). Just using the hidden window should work > fine (though you'll need to use createElementNS(XHTML_NS, "iframe"), since > the hidden window document can be either xhtml (Win/Linux) or xul (Mac)). Ah, indeed, I think this was the issue we tripped up on. If we create a XHTML iframe, is it still possible to retrieve the docShell? I ran into some issues with permissions, but maybe I wasn't doing it right. ::: toolkit/identity/tests/chrome/Makefile.in @@ +19,5 @@ > + sandbox_content_popup.html \ > + sandbox_content_redirect.html \ > + sandbox_content_redirect.html^headers^ \ > + sandbox_content.sjs \ > + $(NULL) Nice tests! \o/ ::: toolkit/identity/tests/chrome/test_sandbox.xul @@ +57,5 @@ > + * Free the sandbox and make sure all properties that are not booleans, > + * functions or numbers were freed. > + */ > +function free_and_check_sandbox(aSandbox) { > + SimpleTest.executeSoon(function() { I think it might make more sense to put this (and the following test_creation, test_reload tests too) into an xpcshell-test. That way, when you implement the hidden DOM window that gets tested implicitly too.

Yes, you should still be able to access the docShell regardless of what kind of iframe it is, though you might need to do a slightly different funny dance?

I tried a couple different ways, but this one is the most promising: frame.QueryInterface(Ci.nsIInterfaceRequestor) .getInterface(Ci.nsIWebNavigation) .QueryInterface(Ci.nsIDocShell); (instead of doing frame.docShell directly, which will only work if it's a XUL iframe). Matt, can you try this and see if it works? Though we only need to do this on Linux & Windows, so you might want to add a check to find out the type of the hiddenDOMWindow first.

iframe.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDocShell) might be more reliable. I'm not sure how frame.QueryInterface(Ci.nsIInterfaceRequestor) works.

Thanks for the info guys. * 2 tests TODOs fixed: Redirects are now handled in the sandbox and dialogs from the sandbox no longer work (now that we're using the hiddenDOMWindow). * Made test_popup cleanup better * Switched to DOMContentLoaded instead of DOMWindowCreated so that consumers of the API (and tests) don't start probing the Sandbox private members before it's ready. This also aligns with the function name "_makeSandboxContentLoaded". Gavin, let me know what directory to put this JSM in so it can be shared better. (In reply to Anant Narayanan [:anant] from comment #8) > Comment on attachment 634815 [details] [diff] [review] > > > Consider using log4moz at some point. > > > > If only bug 451283 would get fixed. > > Is adding log4moz to toolkit a prerequisite? All the services code already > uses log4moz, so you could, in theory do a > Cu.import("resource://services-common/log4moz.js"); no? My understanding is that toolkit shouldn't depend on Services as not all toolkit apps build services. I've switched to using our centralized Identity logging function. > ::: toolkit/identity/tests/chrome/test_sandbox.xul > @@ +57,5 @@ > > + * Free the sandbox and make sure all properties that are not booleans, > > + * functions or numbers were freed. > > + */ > > +function free_and_check_sandbox(aSandbox) { > > + SimpleTest.executeSoon(function() { > > I think it might make more sense to put this (and the following > test_creation, test_reload tests too) into an xpcshell-test. That way, when > you implement the hidden DOM window that gets tested implicitly too. It seems like the hiddenDOMWindow doesn't exist in the xpcshell environment so unless I'm doing something wrong, they'll have to use the mochitest framework. [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIAppShellService.hiddenDOMWindow]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: resource://gre/modules/identity/Sandbox.jsm :: Sandbox__createFrame...]

Comment on attachment 637411 [details] [diff] [review] v.2 hiddenDOMWindow with XHTML iframe (disables dialogs) and make redirects work The _createFrame changes look good to me. Don't worry about the file location for the moment, you can land it where it is in this patch and we can always move it later.

Comment on attachment 637411 [details] [diff] [review] v.2 hiddenDOMWindow with XHTML iframe (disables dialogs) and make redirects work Who wants to be the toolkit peer to review this?

Comment on attachment 637411 [details] [diff] [review] v.2 hiddenDOMWindow with XHTML iframe (disables dialogs) and make redirects work Or perhaps this falls under the BrowserID module[1] and that page should be updated to list toolkit/identity? Then Ben can just review this. [1] https://wiki.mozilla.org/Modules/All#BrowserID

Comment on attachment 637411 [details] [diff] [review] v.2 hiddenDOMWindow with XHTML iframe (disables dialogs) and make redirects work jst, can you make sure we're doing the docshell/webnavigation/presshell stuff correctly?

Yup, I'm planning to spend time on this (and other identity reviews ) tomorrow.

I'm doing a try run with just this patch (removing the logging dependency on IDLog and adding 2 Makefile.in files) so we could land it first as a sanity check that we have the build system right before we push the major identity patches. https://tbpl.mozilla.org/?tree=Try&rev=5cd08dee6740

Comment on attachment 637411 [details] [diff] [review] v.2 hiddenDOMWindow with XHTML iframe (disables dialogs) and make redirects work >diff --git a/toolkit/identity/Sandbox.jsm b/toolkit/identity/Sandbox.jsm >+ _createFrame: function Sandbox__createFrame() { >+ let hiddenWindow = Services.appShell.hiddenDOMWindow; >+ let doc = hiddenWindow.document; >+ >+ // Insert iframe in to create docshell. >+ let frame = doc.createElementNS(XHTML_NS, "iframe"); Actually, I discovered in bug 762569 that this won't work - you get a chrome docshell for html iframes in the hidden document, which isn't desirable, and XHTML iframes apparently can't be marked type="content".

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #19) > >+ _createFrame: function Sandbox__createFrame() { > >+ let hiddenWindow = Services.appShell.hiddenDOMWindow; > >+ let doc = hiddenWindow.document; > >+ > >+ // Insert iframe in to create docshell. > >+ let frame = doc.createElementNS(XHTML_NS, "iframe"); > > Actually, I discovered in bug 762569 that this won't work - you get a chrome > docshell for html iframes in the hidden document, which isn't desirable, and > XHTML iframes apparently can't be marked type="content". That explains the problem I was seeing. Would the right solution then be to create a new hidden top-level XUL window if the enumerator doesn't return any, and then append an iframe to it? Hopefully the docShell still gets created even though the top level window is hidden...

Comment on attachment 637411 [details] [diff] [review] v.2 hiddenDOMWindow with XHTML iframe (disables dialogs) and make redirects work Review of attachment 637411 [details] [diff] [review]: ----------------------------------------------------------------- I would prefer if Sandbox.jsm didn't depend on other identity components, e.g. IDLog, but otherwise this looks good to me from the identity point of view.

(In reply to Anant Narayanan [:anant] from comment #20) > Would the right solution then be to create a new hidden top-level XUL window > if the enumerator doesn't return any, and then append an iframe to it? > Hopefully the docShell still gets created even though the top level window > is hidden... I'm investigating other solutions for bug 762569. One of them might be to make the default hidden window (used on Windows/Linux) be an XHTML document so that you can insert a XUL document into it.

I was wrong about the cause - the key distinction is that the document is unprivileged (can't create XUL elements), not that it's HTML. I just attached a patch in bug 769771 that should allow us to use an xhtml:iframe with mozframetype="content".

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #7) > This seems like an important issue to solve, particularly if this is to be > useful as a general purpose module (we might like to use this for social > code as well - see bug 766607). Just using the hidden window should work > fine (though you'll need to use createElementNS(XHTML_NS, "iframe"), since > the hidden window document can be either xhtml (Win/Linux) or xul (Mac)). Would it be okay if we reverted this change? It's causing some cascading failures, specifically in the way it interacts with frameMessageManager. I'd rather we have something working now and improve it later.

Comment on attachment 638086 [details] [diff] [review] v.3 Use @mozframetype=content from bug 769771 Looks good, r=jst

(In reply to Ben Adida [:benadida] from comment #24) > Would it be okay if we reverted this change? It's causing some cascading > failures, specifically in the way it interacts with frameMessageManager. I'd > rather we have something working now and improve it later. I think, from reading later comments on IRC from Matt, that these have been sorted out? If you are still seeing issues, I can help you figure them out.

Comment on attachment 638086 [details] [diff] [review] v.3 Use @mozframetype=content from bug 769771 >diff --git a/toolkit/identity/Sandbox.jsm b/toolkit/identity/Sandbox.jsm >+ _createFrame: function Sandbox__createFrame() { >+ frame.setAttribute("mozbrowser", true); Is this still necessary to get the right .top behavior (or is setting .isBrowserFrame sufficient)? The .top thing doesn't seem to be covered in the tests, can we add it? >+ _createSandbox: function Sandbox__createSandbox(aCallback) { >+ let webNav = this._frame.contentWindow >+ .QueryInterface(Ci.nsIInterfaceRequestor) >+ .getInterface(Ci.nsIWebNavigation); >+ let docShell = this._frame.contentWindow >+ .QueryInterface(Ci.nsIInterfaceRequestor) >+ .getInterface(Ci.nsIWebNavigation) >+ .QueryInterface(Ci.nsIInterfaceRequestor) >+ .getInterface(Ci.nsIDocShell); >+ >+ webNav.loadURI( >+ this._url, >+ docShell.LOAD_FLAGS_BYPASS_CACHE, >+ null, // referrer >+ null, // postData >+ null // headers >+ ); You could just use Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_CACHE, and avoid doing that dance to retrieve "docShell" (which partially duplicates the dance done for webNav :).

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #28) > Comment on attachment 638086 [details] [diff] [review] > v.3 Use @mozframetype=content from bug 769771 > > >diff --git a/toolkit/identity/Sandbox.jsm b/toolkit/identity/Sandbox.jsm > > >+ _createFrame: function Sandbox__createFrame() { > > >+ frame.setAttribute("mozbrowser", true); > > Is this still necessary to get the right .top behavior (or is setting > .isBrowserFrame sufficient)? I don't think it was necessary for the .top behaviour but there may be other behaviour fixes so I think it's safer to leave it for now. > The .top thing doesn't seem to be covered in the tests, can we add it? I had the test in progress in my patch queue but didn't want to slow down the review of the code since we want to land this today. It tests: * window.top == window * Components.classes is not defined * Trying to access an attribute on Ci.nsIDOMWindowUtils throws If there are other things we should test, we can add them in a follow-up. > You could just use Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_CACHE, and avoid > doing that dance to retrieve "docShell" (which partially duplicates the > dance done for webNav :). Yes, I planned on doing that but in the rush to land this, I didn't come back to it. One of these attributes fixed it so window.open doesn't work from the sandbox content! Another todo down :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/15ac0594d478 Try results (excluding attachment 638168 [details] [diff] [review]): https://tbpl.mozilla.org/?tree=Try&rev=5e9a22124b91 https://tbpl.mozilla.org/?tree=Try&rev=bed1f91910fe (follow-up fix for macosx debug orange)

https://hg.mozilla.org/mozilla-central/rev/15ac0594d478 https://hg.mozilla.org/mozilla-central/rev/25e49f145492

Cc:ing jlebar here so he can look over how the mozbrowser stuff ended up being used here.

This seems relatively sane to me, although doing this means we have to lock down in-process mozbrowser properly (it currently is not bug-free). But I'm working on it!