The B2G browser needs to be able to create an iframe and look inside it, violating the cross-origin policy. There's a lot of work here, but I'd like this bug to track a simple case, letting the parent frame read its child's window.location. We might get other properties for free, but I'm not going to go out of my way for them.

Wow, I really don't want to add a new docshell type if I don't have to. That looks painful.

Some terminology to keep us sane: The HTMLBrowser is the webpage which acts like a browser. The HTMLBrowserContent is the webpage loaded in an iframe by the HTMLBrowser. The HTMLBrowser needs to peer into the HTMLBrowserContent.

What all properties are needed from HTMLBrowserContent? Could there be some (possibly async) postMessage -like API to query certain properties. The web page in HTMLBrowserContent wouldn't need to implement it, but it would be builtin. HTMLBrowser: iframe.onbrowserstate = function(evt) { alert(evt.state + "=" + evt.data); } iframe.queryBrowserState("location");

> What all properties are needed from HTMLBrowserContent? Lots of things. Read window.location and document.title, make bitmap screen captures of the rendered page (for thumbnails), receive events when the iframe navigates (so it can update the location bar)... HTMLBrowser needs to be a Browser with a capital B. So whatever Firefox chrome does, it's likely that HTMLBrowser needs to do. Ben pointed out, for example, that we need to be able to peer into the DOM to implement sessionrestore.

See https://wiki.mozilla.org/WebAPI/EmbeddedBrowserAPI for a (surely partial) list of things needed.

> Could there be some (possibly async) postMessage -like API to query certain properties. This would certainly be a lot easier than hacking wrappers, and it might get us 90% of the way there. We'd still need something for registering events.

Comment on attachment 580335 [details] [diff] [review] Part 0: Add nsContentUtils::URIInPrefList Review of attachment 580335 [details] [diff] [review]: ----------------------------------------------------------------- You must have done too much C in your life to use |type *param| :) r=me with the comments addressed ::: content/base/public/nsContentUtils.h @@ +1862,5 @@ > + * > + * Comparisons are case-insensitive, and whitespace between elements of the > + * comma-separated list is ignored. > + */ > + static bool URIInPrefList(nsIURI *aURI, const char *aPref); nit: I would have prefer "IsURIInPrefList", it is more readable. ::: content/base/src/nsContentUtils.cpp @@ +696,5 @@ > + > + if (whitelistItem.Length() == prePath.Length() && > + CaseInsensitiveCompare(whitelistItem.Data(), > + prePath.Data(), > + whitelistItem.Length()) == 0) { You can use nsAString::Equals(const PRUnichar*, const nsStringComparator&).

Comment on attachment 580337 [details] [diff] [review] Part 1: Allow window.location to be queried cross-origin. Review of attachment 580337 [details] [diff] [review]: ----------------------------------------------------------------- Couldn't you create a new interface that would apply to nsHTMLIFrameElement only if "dom.mozQueryInnerStateEnabled" is true? Seems cleaner to me.

Comment on attachment 580337 [details] [diff] [review] Part 1: Allow window.location to be queried cross-origin. >+ nsIPrincipal *principal = NodePrincipal(); >+ nsCOMPtr<nsIURI> principalURI; >+ principal->GetURI(getter_AddRefs(principalURI)); >+ if (!nsContentUtils::URIInPrefList(principalURI, "dom.mozQueryInnerStateWhitelist")) { >+ return NS_ERROR_FAILURE; >+ } Should chrome be able to query always? And, throw security error, not generic NS_ERROR_FAILURE

How do you feel about Mounir's suggestion to move this code into a separate interface? > Should chrome be able to query always? I'm tempted to say YAGNI. But I'd also be happy to put a chrome check into URIInPrefList. What do you think?

Ah, yes, a separate interface would be good. Oh, also, make sure also <frame> element supports the interface. So, could you push that implementation to nsGenericHTMLFrameElement

It would be strange to add chrome check to URIInPrefList, since that has nothing to do with chrome. There are other helpers which you could use.

Maybe we should change the name, but URIInPrefList is going to be used all over the place until we get the much promised Permission Manager, so I'd rather have both the pref and chrome check in one place.

Comment on attachment 583043 [details] [diff] [review] Part 3 - Renaming queryInnerState to getInnerState. I spent a while trying to come up with a better name for this. I came up with <iframe inspectable> getInnerState() setInnerState() addInnerStateListener(topic, callback) removeInnerStateListener(topic, callback) I feel like this is better than "queryInnerState", since it's not clear how the setter would be named. But I'm not entirely satisfied with "inspectable".

How about... <iframe browser> getContentProperty(name, listener) setContentProperty(name) addContentEventListener(type, listener) removeContentEventListener(type, listener) Unless there's some reason it's inaccurate, "*ContentProperty" seems more in-keeping with the current lexicon than "*InnerState" (e.g. window vs. contentWindow). I'm not so sure about "browser" vs. "inspectable" but bare in mind that as well as getters, setters and event listeners there may need to be methods such as "reload" and "stop" so the iFrame isn't just inspectable, it can be controlled too. I can't think of a more accurate term to describe this succinctly than "browser".

I'm OK with that naming. Smaug? I don't really like <iframe browser>, because that's sort of proscriptive rather than descriptive. By which I mean, it's saying "you can make a browser out of this", rather than "you can peer into this iframe and modify it." But we can always change it again if we come up with something better.

Comment on attachment 582069 [details] [diff] [review] Part 1, v2: Add queryInnerState() >+ nsCxPusher pusher; >+ nsCOMPtr<nsIDOMEventTarget> eventTarget = >+ do_QueryInterface(nsContentUtils::GetWindowFromCaller()); >+ NS_ENSURE_TRUE(eventTarget, NS_ERROR_FAILURE); >+ pusher.Push(eventTarget); One must always check the return value of Push

Check the return value of Push().

Comment on attachment 583198 [details] [diff] [review] Part 2: Renaming to <iframe browser> and getContentState(). (will be folded into part 2) (Sorry to churn the attachment descriptions like this -- I'm trying out a git workflow and still working out the kinks.)

inspectable sounds strange. Hard for at least one non-native-English speaker. What would add/RemoveContentEventListener do? Will the iframe run always in the same process? If not, perhaps getContentProperty should be requestContentProperty and the callback would be called async.

I don't find it so confusing that "getContentState" is async, myself. Since we have something called "setContentState" (which is also async), maybe it makes sense to have a corresponding "get" function?

Comment on attachment 583198 [details] [diff] [review] Part 2: Renaming to <iframe browser> and getContentState(). (will be folded into part 2) >-[scriptable, function, uuid(077660D6-43A1-4A83-B7D8-F49AC0B539B0)] >-interface nsIDOMQueryInnerStateCallback : nsISupports >+[scriptable, function, uuid(37687881-1801-489f-ad03-7af651a93448)] >+interface nsIDOMInnerStateCallback : nsISupports nsIDOMMoz... >-[scriptable, uuid(655d6101-6b60-4a55-ba1c-cff5e17ec3f4)] >-interface nsIDOMQueryableFrameElement : nsISupports >+[scriptable, uuid(2ff0f421-64e4-4186-b0dd-619629f46048)] >+interface nsIDOMBrowserFrameElement : nsISupports nsIDOMMoz... >- * If the iframe is not mozQueryable, or if the calling window is not >+ * If the iframe's mozBrowser is false, or if the calling window is not > * privileged, this function fails. > */ >- void mozQueryInnerState(in DOMString property, >- in nsIDOMQueryInnerStateCallback callback); >+ void mozGetContentState(in DOMString property, >+ in nsIDOMInnerStateCallback callback); Why do we need callback here. The implementation seems to work synchronously. If the idea is that in the future iframe would be using a separate process, then async API is needed. But please implement even the current API in an async way. (use runnables)

> If the idea is that in the future iframe would be using a separate process, then > async API is needed. Yes, this is the idea. > But please implement even the current API in an async way. (use runnables) Since this API is exposed only to B2G at the moment and for the foreseeable future, will you let me get away with responding synchronously in this patch? It'll be wasted work to rewrite all this, since the IPC code will be totally different.

Smaug, ping re comment 31?

(In reply to Justin Lebar [:jlebar] from comment #31) > Since this API is exposed only to B2G at the moment and for the foreseeable > future, will you let me get away with responding synchronously in this > patch? No. Since if the API is first sync, all the code written using it will assume sync behavior. It would be painful to change from sync to async later. > It'll be wasted work to rewrite all this, since the IPC code will be > totally different. Handling the callback in a runnable for now isn't difficult. That is enough to force the user of the API to handle things async.

I can do a roll-up patch too, if you'd prefer. But this seems simple enough to me.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc753e605073 https://hg.mozilla.org/integration/mozilla-inbound/rev/1bdcd84ae727

And a followup to fix a silly test: https://hg.mozilla.org/integration/mozilla-inbound/rev/14385cbb4131

So we're going to have two kinds of privileged pages, each one with its own set of APIs for writing a browser?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #38) > So we're going to have two kinds of privileged pages, each one with its own > set of APIs for writing a browser? Yes? Note that a lot of the existing Chrome API (e.g. nsIWebProgress) is hardly a web API.

https://hg.mozilla.org/mozilla-central/rev/cc753e605073 https://hg.mozilla.org/mozilla-central/rev/1bdcd84ae727

Comment on attachment 589970 [details] [diff] [review] Patch v1: Back out Oops; wrong bug.

Note that this bug was backed out as part of bug 710231. The functionality there subsumes the functionality added here, so I'm leaving this bug as FIXED.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a262cdf67c82

Marking as [qa-] since this bug has been backed out and the bug where it has been backed out is [qa-].