need ability to open windows independently of appshell & XUL

VERIFIED FIXED in mozilla0.9

Status

enhancement
P1
normal
VERIFIED FIXED
19 years ago
4 months ago

People

(Reporter: danm.moz, Assigned: danm.moz)

Tracking

(Blocks 1 bug, {embed})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

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.
Keywords: embed
Whiteboard: ETA 2 Feb
Target Milestone: --- → mozilla0.8
Blocks: 64846
Whiteboard: ETA 2 Feb → ha! i spent my 0.8 milestone dogpile day elsewise. didn't make it.
p1/enhancement, added tentative eta, cc'd self
Severity: normal → enhancement
Priority: -- → P1
Whiteboard: ha! i spent my 0.8 milestone dogpile day elsewise. didn't make it. → ETA: 2/8 (?)
Status: NEW → ASSIGNED
Whiteboard: ETA: 2/8 (?)
Target Milestone: mozilla0.8 → mozilla0.9
Blocks: 64833
No longer blocks: 64833
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.
Keywords: patch
Blocks: 69923
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?
Blocks: 70229
Patch is in. Feature is implemented. Bug is put to sleep.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Keywords: patch
Resolution: --- → FIXED
Blocks: 71895
Correction: Changing QA contact for the Embed API bugs to David Epstein.
QA Contact: mdunn → depstein
reassign qa contact to Dharma.
QA Contact: depstein → dsirnapalli
code checked in.verified.
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.