In our current application-centric scheme, window opening from JS (window.open) eventually lands in appshell and nsXULWindow. These are things that are likely or at least wishing to be omitted from an embedded situation. While window creation should be up to the embedding app, embedded Gecko still wants to be able to trigger the process. The mechanism for doing this is nearly already in. More tweaks to the general code and then support in the embedding testbed are wanted.

p1/enhancement, added tentative eta, cc'd self

On the use of the JSContextStack... In one place you are only using it for SetReferrer - so it is fine if there is not a JSContext on the stack. Though, I wonder if it is *always* appropriate to set the referrer. It also stikes me as inherently broken that the way to get the opener's uri has to rely on this whacky path through the assumptions about the JS global object of the current JSContext. But, if that's the way the system works then so be it. I don't see anything wrong with that bit of code. The stuff in nsWindowWatcher::GetJSContext is less good. Given that a comment there uses the word 'skank', I suppose you know this :) You'll have to ask jst or someone else about the general safety of the cast from GetNativeContext() to JSContext for the future. I don't know about that. I'm not clear on what conditions prevail in all cases where this might be called. Do you always need the 'right' JSContext? Or just *some* JSContext? When you are called from JS that is executing because of window events (etc) then the context stack Peek should (probably!) always work. You get into trouble if this is called directly from native code because there is no guarentee that there will be a JSContext on the top of the stack. nsIThreadJSContextStack has a GetSafeJSContext method that can be used in that case. However, if it comes to that then you'll need to Push that JSContext (so that subsequent JS code will get it from Peek) *and* be sure to Pop it later. Again, I'm not clear if this is necessary (hitting that assert you have in that code would be a good clue). If you do need to go to this depth, then you'd probably want something like - http://lxr.mozilla.org/seamonkey/ident?i=JSCLAutoContext - to automate the cleanup part. The JS_{Begin,End}Request parts would not be necessary in our system as this is always run on the main thread. So - assuming you've tested this well and know what you are doing in general - r=jband on the JSContextStack parts.

Regarding John' comments: "know what i'm doing"... That'd be nice. The SetReferrer thing is copied straight from the original location (nsGlobalWindow), no change, so odd as that code may be, it's withstood the test of time. The skank comment was about a cast that made me unhappy. But apparently that's just the way it works. Of the 296 uses of GetNativeContext in the codebase, they're all cast to a JSContext *. Your last point is more of a worry. The function can be called from native code under random circumstances including in-and-out-of-JS, so it might be right to add the GetSafeJSContext you talk about. On the other hand, that context is used (only) to ask the security manager whether an upcoming URI load is allowable. "know what I'm doing"... Heh. I've asked Mitchell to look at this, too. I'll add him to the cc: list. I'm thinking (please correct me if I'm wrong) that under those circumstances, whatever context is on the stack is probably the right one, I don't want to be making up new ones, and if there isn't one at all I should -- ahem -- not flag an error, and allow the URI load without bothering the security manager (thinking that that means there is no JS earlier up the execution chain, and native code not summoned from JS gets what it asks for.) Brendan concurs. Anybody see anything wrong with that? Thanks for looking, John and Mitchell!

Securitywise, I think your assumption about JSContexts is valid. If we have a context, it's probably the right one, and if we don't, then we're being called from native code so we know the security check will succeed.

Turns out I goofed on whether I'll need the security manager; it's used later in the process, for things like SetOwner on the DocShellLoadInfo. So I had to add the JSContext pusher thing John was talking about. I'm still a little concerned about this. Here comes another patch. I think it has John's concerns covered. Some security concerns still. I'll find someone else to do the overall review.

A couple of points about the patch... dom/src/base/nsGlobalWindow.cpp: Eeuuhh, a bunch of uninitialized variables at the top, some of which are not even used except in a very limited scope (the JSString * ones), could you move thise info the blocks where they're actually used and do the initialization of the PRUnichar * ones when you declare them? It's no big deal but I get the willies every time I see uninitialized pointers. Oh, and extraArgc could be moved into the if (wwatch)... at the end too. I'll have the pleasure of rewriting this method once the DOM uses XPConnect :-) nsWindowWatcher.cpp: Nit! JSContextAutoPusher::Context() really needs to be named ::GetContext(), or simply just ::get() to follow general naming conventions used in mozilla. Your xxxAutoPusher classes seem a bit odd to me, they're not really something that "push" something automatically, rather they "pop" automatically. If the classes were written to push on construction and pop on destruction they'd be nice and symmetrical (and I can see from the code why this is not how your classes work) and then naming them xxxAutoPusher would make more sense, but since your classes require the user of these classes to manually call Push() I think renaming them to xxxAutoPopper be clearer here, no? xpfe/bootstrap/nsAppRunner.cpp: Any reason for InitializeWindowCreator() not being static? And why not return NS_ERROR_OUT_OF_MEMORY if new nsWindowCreator returns null in stead of returning NS_ERROR_FAILURE? Also, in InitializeWindowCreator() you use dont_QueryInterface(), you don't need to, it's a nop, so unless you're trying to express something by using it here (which I don't see) then don't use it. Nit: in the end of nsWindowCreator::CreateWindow() there's this code: appShell->CreateTopLevelWindow(..., getter_AddRefs(newWindow)); if (newWindow) { nsCOMPtr<nsIInterfaceRequestor> thing(do_QueryInterface(newWindow)); if (thing) thing->GetInterface(NS_GET_IID(nsIWebBrowserChrome), (void **) _retval); } that can safely be reduced to: appShell->CreateTopLevelWindow(..., getter_AddRefs(newWindow)); nsCOMPtr<nsIInterfaceRequestor> thing(do_QueryInterface(newWindow)); if (thing) thing->GetInterface(NS_GET_IID(nsIWebBrowserChrome), (void **) _retval); since do_QueryInterface() is null ptr safe your if(newWindow) is redundant... With those issues at least conciderd, r=jst

Alright, all of those things done. Less willies for Johnny. Thanks!

Fewer willies. Dangerously close to losing my apostrophe griping privileges by making mistakes like that, I am. I was thinking of Willie the Groundskeeper. But who doesn't do that?

Patch is in. Feature is implemented. Bug is put to sleep.

Correction: Changing QA contact for the Embed API bugs to David Epstein.

reassign qa contact to Dharma.

code checked in.verified.