Closed Bug 742944 Opened 13 years ago Closed 12 years ago

Handle window.open in <iframe mozbrowser>

Categories

(Firefox OS Graveyard :: General, defect)

Product:
Firefox OS Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-kilimanjaro:+, blocking-basecamp:+)

Status:
RESOLVED FIXED
Project Flags:
blocking-kilimanjaro +
blocking-basecamp +

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(6 files, 3 obsolete files)

Part 1: Always set nsDocShell::isBrowserFrame to true, for in- and out-of-process iframes.
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
Bug 742944 - Part 2: Allow getInterface'ing an nsGlobalWindow to an nsIDocShell.
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
Bug 742944 - Part 5: <iframe mozbrowser> window.open tests.
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
Part 3: Move nsHTMLIFrameElement to a header file.
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
Part 4: Handle window.open in <iframe mozbrowser>.
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
Small fix: missing interface in b2g package
(deleted), patch
vingtetun
: review+
Details | Diff | Splinter Review 
My thought is that window.open should call a method in the window which contains the <iframe mozbrowser>.

That method will return a window object, which is what we'll return down to the caller of window.open.

This means that the browser app and the browsed page must be in the same process.  We can worry about whether or not this is good after we have e10s on b2g.
Blocks: browser-api
Bobby, do I need to do anything special when I synchronously call a content method from chrome?

I'm going to do something like

  XPCNativeWrapper(win.frameElement).onnewwindow(url)
>  XPCNativeWrapper(win.frameElement).onnewwindow(url)

Do you mean XPCNativeWrapper.unwrap?

If so, it should be fine. Waivers automatically have their principals clamped during cross-compartment function calls.

Note that I still think we need to sit down sometime and come up with a real security model for all this stuff.
> Do you mean XPCNativeWrapper.unwrap?

Yes, sorry. :)
We already support window.open for content processes.  See PBrowser.ipdl.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> We already support window.open for content processes.  See PBrowser.ipdl.

Yes, but all the hydrogen and oxygen atoms here are happily bonded, at least for the time being...
Also, note that this is a separate problem.  The issue here is, the browser frame needs a chance to control where the popup goes -- presumably, it's going to create an iframe somewhere, and we need to return that window down to the browsee.

The browser frame also needs to be able to deny the request for a popup -- if an app does window.open, I expect we want to return null (as though there's a permanent popup blocker).
(In reply to Justin Lebar [:jlebar] from comment #6)
> Also, note that this is a separate problem.  The issue here is, the browser
> frame needs a chance to control where the popup goes -- presumably, it's
> going to create an iframe somewhere, and we need to return that window down
> to the browsee.
> 
I was expecting to use nsBrowserAccess from the b2g/ chrome code to fire an Intent (like 'open-in-browser' or something like that. This way the browser application that handle the intent could open a window (in a tab) an provide it as a result to the intent.



> The browser frame also needs to be able to deny the request for a popup --
> if an app does window.open, I expect we want to return null (as though
> there's a permanent popup blocker).

I was expecting the same nsBrowserAccess behavior.
Well, first of all, intents don't exist at the moment, right?

Second of all, there might be more than one browser app.  The running browser app needs a chance to handle the popup -- it shouldn't go globally to the default browser, nor should the user be prompted for which browser to open.

Third, remember that we have to return the opened window in window.open.  So this whole thing needs to block the content script.  Are intents intended to be blocking and return DOM objects like this?

From an app, if window.open is supposed to "launch the browser", we don't want to return the window to the app -- that would preclude running the app and the browser in separate  processes.  But if we return null, the app will think the popup was blocked.  So I'm not sure how to do this well, except by always returning null from window.open and allowing the app to use intents to open new browser windows.

<a target=_blank> should Just Work from within an app.
I think we're well within our rights to block window.open from <iframe mozapp>.  It's not clear what that should mean.  Until we have use cases we might as well just block it.

Agreed about target=_blank.
Doesn't blocking window.open completely break the web? I would imagine there are lots of existing web sites which would not expect this to break if they start hosting an app manifest.
(In reply to Ben Francis from comment #10)
> Doesn't blocking window.open completely break the web? I would imagine there
> are lots of existing web sites which would not expect this to break if they
> start hosting an app manifest.

People already have popup blockers, so you can't rely on window.open working, in general.

But do you have a better suggestion?
There's a real tension here between the needs of the desktop and the needs of mobile. On desktop it's not unusual for apps to have multiple windows, on mobile it's less common.

timdream mentioned third party authentication as a common use case for window.open, but I think he will post some of his thoughts himself.
Josh, Ben and I have been talking about the specific use case of the 3rd party log-in (e.g. Facebook connect). Not having window.open will complete break that, with no useable alternative solution (beside forcing web apps to use BrowserID).

We do have some idea to workaround the popup window on the UX front (basically that involves alternate the behavior of window.open to user; making the popup as the modal dialog of the original app instead of throwing it to the browser, etc.), but at minimal the platform need to be responsive to window.open calls for Gaia window manager to handle.

(In reply to Justin Lebar [:jlebar] from comment #11)
> People already have popup blockers, so you can't rely on window.open
> working, in general.

That is true. But it's also true for the specific use case above that this is one popup user expected to see. The current behavior of popup blockers is that it would only block window.open calls from a non-user action routine (e.g. window.onload); for a user trigger event (e.g. click), popup always work. Web developers and author of Facebook Javascript SDK expect that to work.
There's no reason FB connect couldn't run in an iframe, right?

Anyway, this is a policy decision more than a technical one.  I know cjones likes to say that mozapp is in no way a subset of mozbrowser, but here is an exception.  We can expose to Gaia the same window.open handling behavior that we expose on the browser.  Then you can deny the popup or handle it somehow, as you wish!
It is generally considered best practice to use window.open instead of an iframe when presenting a form that will solicit authentication credentials.  Once a session is established, an iframe is generally considered enough for confirmation/authorization steps.  PayPal X, for example, uses this pattern explicitly.

This is a well-understood pattern on the desktop web - but I don't think there's anything like a broad consensus on how mobile should handle it.

Digging into the question-behind-the-question - the threat that authentication services are mitigating is of phishing.  The location bar is the only clue that users might have that they are talking to a fake site.  Historically, this was to mitigate phishing threats from impersonators and homonym domain names (spelling paypal with a "one" glyph, that sort of thing).  I think we are all frustrated, as a community of practice, with how weak this protection is -- users don't know how to interpret the location bar, the SSL identity block, or the color of the block, and certainly don't understand the threat of a valid-SSL-connection-to-a-bogus-site.

So, again, to unpack the issue: the reason why some services can't run in an iframe, today, is that 3rd party services will refuse to offer authenticated services through an iframe, because doing so would create the conditions for a nefarious app or a man-in-the-middle attack to impersonate or proxy them, in order to capture credentials, personal details, or payment data.

So, the issue is, do we provide enough other protection that would make these services comfortable running in an iframe?  Or, alternatively, do we believe that the show-a-location-bar technique of threat mitigation is sufficiently weak, especially on mobile, that we don't mind removing it from the toolchest?  Or do we invent a way of interpreting window.open that works for the 3rd-party-authentication use case without turning into a huge pile of extra work?

I suggest that this needs to get on the WebAPI security review queue quickly - that conversation is happening on dev-webapps right now.

Background reading on threats to iframe-based authentication protocols:
http://www.eecs.berkeley.edu/~sch/w2sp2010ena.pdf
What I was trying to get at in comment 14 is: This discussion about how window.open works in apps is interesting, but orthogonal to this bug.  This bug is the wrong place to have that discussion.

We need to figure out window.open in the browser, and I'd like not to get muddled in the details of apps.

If you want to file a "handle window.open in <iframe mozapp>" bug, or start a thread dedicated to this on a mailing list, that sounds good to me.
Excellent point.  Created bug #744451, app-specific conversation should continue over there.
[Posted this also to 744451]...

Here's some Gaia UX team thoughts on this. Grain of salt: the technical details are largely unfamiliar to me. I'm speaking from general mobile UX best practices standpoint.

"There's no reason FB connect couldn't run in an iframe, right?" 

My suggestion as well. Every mobile OS provides web views of some sort. Open Flipboard on an iPad, and select "Login to Facebook". The FB Connect UI opens in a webview, within the app. 

The alternative—opening the window in the default browser, and then returning the user to the parent app once the login was complete—would be terrible UX. Suspicious as well, to the average user. "Why am I being asked into a random website> Must be a scam..." 

Bottom line, meta conversation: the rules for mobile are different than the rules for desktop. Spawning windows Just Isn't Done. It's one window per app. The only exception is throwing hyperlinks to default browser if parent app doesn't handle them inline (eg: click on URL in an email).
(In reply to Justin Lebar [:jlebar] from comment #8)
> Well, first of all, intents don't exist at the moment, right?
>
> Second of all, there might be more than one browser app.  The running
> browser app needs a chance to handle the popup -- it shouldn't go globally
> to the default browser, nor should the user be prompted for which browser to
> open.
> 
> Third, remember that we have to return the opened window in window.open.  So
> this whole thing needs to block the content script.  Are intents intended to
> be blocking and return DOM objects like this?
> 
> From an app, if window.open is supposed to "launch the browser", we don't
> want to return the window to the app -- that would preclude running the app
> and the browser in separate  processes.  But if we return null, the app will
> think the popup was blocked.  So I'm not sure how to do this well, except by
> always returning null from window.open and allowing the app to use intents
> to open new browser windows.
> 
> <a target=_blank> should Just Work from within an app.

I think I messed up window.open and <a target=_blank> in my mind. Your solution of calling a method in the container of the <iframe mozbrowser> as a reaction to window.open seems good to me.
Depends on: 758297
Hmm...this is becoming tricky.

No matter what I do, I'm eventually going to be in a situation where I have an <iframe mozbrowser> in the parent process, and I want to pass a handle to that iframe's window down to the BrowserElementChild.

How do I do this, exactly?  I presume I need to make a sync call to the mm, but I'm not sure what object to return so the mm marshals it properly.
can you be more precise about what you mean by "handle" and "iframe window".

phone
(In reply to Chris Jones [:cjones] [:warhammer] from comment #21)
> can you be more precise about what you mean by "handle" and "iframe window".

I have an <iframe mozbrowser> in the parent process.  Let's suppose it has a window, which lives in the child process.  (That it has a window at all is not a given, but I understand that part a little better.)

So I'm holding the <iframe mozbrowser> in JS, parent process.  I want to somehow send a message to the child process so that the child gets the window corresponding to this iframe.

Or alternatively, the child could send a sync message to the parent, and the parent could return some object which the mm marshals such that the thing the child receives it can translate into the correct window.

If that's not possible, I now think I can hack around it.  I have listeners which observe when a remote browser frame is Show()'n -- I can figure out, at that point, whether this frame is for my window.open, and send messages to the right BrowserElementChild.  That may or may not make sense, but the point is, it's a lot uglier than simply sending a message through the mm.
we have code for this in TabChild/Parent.
TabChild::ProvideWindow
But is any of this is exposed to the mm, is it?  Working in JS is sometimes an all-or-nothing affair...
It is not in MM, and I don't see reason to expose it via MM. Something else is needed.
You probably want to make TabParent::AnswerCreateWindow to handle also mozbrowser case.
smaug is explaining to me over IRC that the whole window.open call has to happen synchronously (otherwise the page could observe an event occurring *during* the open() call).  That means that the hack in the bottom of comment 22 won't work, because that requires asynchronicity.

I'm worried about relying on sync calls handled by the "embedder", because sync calls block the child process, and we don't necessarily control the embedder's behavior.  These sync calls could become our next "main-thread I/O".  But perhaps this worry is premature.

Anyway, I'll look into what smaug suggested.
Well, s/sync/rpc/
I wonder how hard it would be to make returning a remote <iframe> from a sync message give the iframe's window to the caller.

To be concrete, if I were to do

child:
  let win = sendSyncMessage("get-window");

parent, handler for "get-window" message:
  let frame = (create an <iframe> element, force it to have a window in the child process);
  return frame;
   
then |win| would logically be frame.contentWindow.
Actually, this will not work -- you can't send any other messages to the child while it's waiting on a sync message.  So in particular the parent can't tell the child to create a new window.  :-/
So the most basic constraint here seems to be that, unless we set up an RPC protocol, we can't make any calls from parent to child while the child is waiting for the parent to respond to a sync call.  And everything here has to be sync because window.open has to be sync.

This means that we have to create the window in the child process (basically, TabChild::ProvideWindow) and then send its PBrowser up to the parent process.

If we did it the other way around -- if we had the iframe, in the parent process, initiate the creation of the remote frame -- then the parent would have to send a message to the child asking it to create that frame.  And the parent can't send messages to the child, because the child's message to the parent to start this process was sync.

But that means we have to install a PBrowser into an existing iframe/frameloader.  And the frameloader code is not set up to do this; it assumes that *it's* creating the remote browser object.  We can change this, but I'm not yet convinced that's the path of least resistance.
Whiteboard: [b2g:blocking+]
Interactions with paypal (purchasing without preauth, registration, etc.) are all done via a popup on the marketplace which is paypal code, not something we can change.  Flagging for basecamp blocking triage
blocking-basecamp: --- → ?
Wil, do you mean to nom bug 744451 instead of this one?

The difference is, we're going to allow arbitrary window.open calls in mozbrowser, but in mozapp, the plan is, for B2G, to allow only one popup at a time.

If paypal won't work without more than one popup, we need to talk.

FWIW both this bug and bug 744451 are likely to be resolved by just one checkin.  The restriction to one popup will be done in Gaia, the B2G front-end.  I just want to get the use-cases straight.
(In reply to Justin Lebar [:jlebar] from comment #33)
> Wil, do you mean to nom bug 744451 instead of this one?
> 
> The difference is, we're going to allow arbitrary window.open calls in
> mozbrowser, but in mozapp, the plan is, for B2G, to allow only one popup at
> a time.
> 
> If paypal won't work without more than one popup, we need to talk.
> 
> FWIW both this bug and bug 744451 are likely to be resolved by just one
> checkin.  The restriction to one popup will be done in Gaia, the B2G
> front-end.  I just want to get the use-cases straight.

IMO, This should be nom'd for basecamp.  if bug 744451 gets fixed for both, we can resolve this one.  But there's no harm in tracking an important issue like window.open().  In fact, i just tried loading a popup site on browser, and it completely locks up the phone.  (same symptom if done within an App)
blocking bug 744451, per comment 33.
Blocks: 744451
Thanks for pointing that bug out.  I'll nominate it too. :)

If we can't buy stuff on the platforms it's a pretty big deal, I think both are worth getting triaged/tracked.
Given a docshell, how do I synchronously force it to have a window (say, to about:blank)?
nsCOMPtr<nsIDOMWindow> win = do_GetInterface(docshell); should create the window.
Depends on: 760132
blocking-basecamp: ? → +
blocking-kilimanjaro: --- → +
Whiteboard: [b2g:blocking+]
Assignee: nobody → justin.lebar+bug
Attached patch WIP v1 (obsolete) (deleted) — — Splinter Review 
This is still very much a work in progress (crashes, assertions, security errors), but the relevant window is in fact created synchronously and returned to child-side JS.
Attached patch WIP v2 (obsolete) (deleted) — — Splinter Review 
My lame tests pass.  Getting there...
Attachment #630263 - Attachment is obsolete: true
Depends on: 762049
Depends on: 741587

  

  



        Attachment #631525 -
        Flags: review?(bzbarsky)

    
    

    
  


  

  

  



        Attachment #631526 -
        Flags: review?(bzbarsky)

    
    

    
  


  
I didn't split the IPC code out into a separate patch because it's tightly intergrated with the rest of this patch.

  

  



        Attachment #631527 -
        Flags: review?(jones.chris.g)

        Attachment #631527 -
        Flags: review?(bzbarsky)

    
    

    
  


  

  

  



        Attachment #631528 -
        Flags: review?(bzbarsky)

    
    

    
  
The test changes rely on bug 762049, which I hope to land soon.

  

  



    
  

        Attachment #630570 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 631525 [details] [diff] [review]
Part 1: Always set nsDocShell::isBrowserFrame to true, for in- and out-of-process iframes.

r=me

  

  



        Attachment #631525 -
        Flags: review?(bzbarsky) → review+

    
    

    
  
Comment on attachment 631526 [details] [diff] [review]
Bug 742944 - Part 2: Allow getInterface'ing an nsGlobalWindow to an nsIDocShell.

> +    nsCOMPtr<nsIDocShell> docShell = do_QueryInterface(mDocShell);

You don't need the QI.  mDocShell is already an nsIDocShell.

You also don't need the null-check on the next line, I think.

r=me with that

  

  



        Attachment #631526 -
        Flags: review?(bzbarsky) → review+

    
    

    
  
Comment on attachment 631527 [details] [diff] [review]
Bug 742944 - Part 3: Handle window.open in <iframe mozbrowser>.

>+++ b/content/base/src/nsFrameLoader.h
>+  void SetRemoteBrowser(nsITabParent* aTabParent);

Please document what this is meant to be used for, since it seems like the set of cases when it's OK to call this is pretty limited.

> +++ b/dom/base/nsDOMClassInfo.cpp
>+  NS_DEFINE_CLASSINFO_DATA(OpenWindowEventDetail, nsDOMGenericSH,
>+                           DOM_DEFAULT_SCRIPTABLE_FLAGS)

Hrm.  I was going to ask to just use WebIDL for this, but the everything-must-have-a-parent requirement makes that a pain.  :(

>+  DOM_CLASSINFO_MAP_BEGIN(OpenWindowEventDetail, nsIOpenWindowEventDetail)
>+    DOM_CLASSINFO_MAP_ENTRY(nsIOpenWindowEventDetail)
>+    DOM_CLASSINFO_EVENT_MAP_ENTRIES

Why the EVENT_MAP_ENTRIES?  That looks wrong to me.

>+++ b/dom/browser-element/BrowserElementParent.cpp
>+already_AddRefed<nsIDOMElement>
>+CreateIframe(nsIDOMElement* aOpenerFrameElement)

Hmm.  Why nsIDOMElement and not Element?  I guess because surrounding code is doing that already? :(

>+  nsCOMPtr<nsIContent> popupFrameElementContent =
>+    do_QueryInterface(popupFrameElement);

You don't need a QI here.  nsGenericHTMLElement inherits from nsIContent.

>+  NS_ENSURE_TRUE(popupFrameElement, NULL);

And this null-check makes no sense.  You already know it's non-null here.

You can just call SetAttr on popupFrameElement directly.  So I don't think you even need popupFrameElementContent.

>+DispatchOpenWindowEvent(nsIDOMElement* aOpenerFrameElement,

Please document the meaning of the return value.

>+  nsCOMPtr<nsIWritableVariant> detailVariant =
>+    do_CreateInstance(NS_VARIANT_CONTRACTID);

Just |new nsVariant|, please.

>+  // preventDetault() being called on the event indicates that we're proceeding
>+  // with the window.open call.  If preventDefault() wasn't called, the
>+  // window.open call should be aborted.
>+  return status == nsEventStatus_eConsumeNoDefault;

That's somewhat backwards.  I sort of see why you did it this way, but would it make more sense to do it via a method call on the detail?

>+BrowserElementParent::OpenWindowOOP(mozilla::dom::TabParent* aOpenerTabParent,
>+  static_cast<nsGenericHTMLFrameElement*>(popupBrowserFrame.get())->

I would slightly prefer that CreateIframe just return an already_AddRefed<nsGenericHTMLFrameElement>.

>+++ b/dom/browser-element/nsOpenWindowEventDetail.cpp
>+NS_IMPL_CYCLE_COLLECTION_CLASS(nsOpenWindowEventDetail)
>+
>+NS_IMPL_CYCLE_COLLECTING_ADDREF(nsOpenWindowEventDetail)
>+NS_IMPL_CYCLE_COLLECTING_RELEASE(nsOpenWindowEventDetail)
>+
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsOpenWindowEventDetail)
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mFrameElement)
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_END
>+
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsOpenWindowEventDetail)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mFrameElement)
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END

You can replace all that with:

  NS_IMPL_CYCLE_COLLECTION_1(nsOpenWindowEventDetail, mFrameElement)

>+++ b/dom/ipc/TabChild.cpp
>+TabChild::BrowserFrameProvideWindow(nsIDOMWindow* aOpener,
>+  printf("[TabChild] BrowserFrameProvideWindow\n");

Please nix.

>+  newChild->SendBrowserFrameOpenWindow(this, url, name,
>+                                         features, aWindowIsNew);

Fix indent.

> TabChild::RecvShow(const nsIntSize& size)
>+        printf("[TabChild] ignoring SHOW because we already faked one.\n");

Nix.

>+++ b/dom/ipc/TabParent.cpp
>+TabParent::RecvBrowserFrameOpenWindow(PBrowserParent* aOpener,
>+  printf("[TabParent] RecvBrowserFrameOpenWindow(%p)\n", aOpener);

And this.

I don't understand the "throw ABORT if the window is not new" code in the IPC bits.  How is this handling window.open into existing windows?

  

  



        Attachment #631527 -
        Flags: review?(bzbarsky) → review-

    
    

    
  
Comment on attachment 631528 [details] [diff] [review]
Bug 742944 - Part 5: <iframe mozbrowser> window.open tests.

Why does file_browserElement_Open1.html have alert() calls?  Same for various other tests... how does that work in test harnesses?

  

  



    
    

    
  
(In reply to Boris Zbarsky (:bz) from comment #50)
> Comment on attachment 631528 [details] [diff] [review]
> Bug 742944 - Part 4: <iframe mozbrowser> window.open tests.
> 
> Why does file_browserElement_Open1.html have alert() calls?  Same for
> various other tests... how does that work in test harnesses?

When content inside <iframe mozbrowser> calls alert(), it triggers a mozbrowsershowmodalprompt event in the embedder.  So that's how the child communicates success/failure to the parent.

We could do it directly through the message manager (and indeed that's how I did it before I had alert), but this is just as easy and doesn't rely on any special powers.

  

  



    
    

    
  
Comment on attachment 631528 [details] [diff] [review]
Bug 742944 - Part 5: <iframe mozbrowser> window.open tests.

Ah, ok.  Worth documenting that!

  

  



        Attachment #631528 -
        Flags: review?(bzbarsky) → review+

    
    

    
  
Thanks a lot for the fast review.

> I was going to ask to just use WebIDL for this, but the everything-must-have-a-parent requirement 
> makes that a pain.  :(

OOC, what do you mean?

> >+  // preventDetault() being called on the event indicates that we're proceeding
> >+  // with the window.open call.  If preventDefault() wasn't called, the
> >+  // window.open call should be aborted.
> >+  return status == nsEventStatus_eConsumeNoDefault;
> 
> That's somewhat backwards.  I sort of see why you did it this way, but would it make more sense to 
> do it via a method call on the detail?

Yes, I agree it's backwards.  I did the same thing for alert, but I can change that.

Making the event not cancelable and adding a method on the detail sounds good to me.  The default behavior, if you didn't call the method, would still be to block the popup (or not show the alert), right?

> I don't understand the "throw ABORT if the window is not new" code in the IPC bits.  How is this 
> handling window.open into existing windows?

I don't understand the question, but the purpose of the ABORT business is because if nsIWindowProvider returns NULL, the WindowWatcher goes ahead and creates a new window on its own!  (I don't understand why it does that...)

  

  



    
    

    
  
> I would slightly prefer that CreateIframe just return an already_AddRefed<nsGenericHTMLFrameElement>.

Turns out the ergonomics of dealing with nsRefPtr<nsGenericHTMLFrameElement> are just awful; you can't QI it (nsISupports is an ambiguous base), you can't AddRef it (same reason).  Maybe I'm doing something wrong.

  

  



    
    

    
  
> Hmm.  Why nsIDOMElement and not Element?

I really have no idea when to use which class, and reading through the code isn't enlightening me.  Do you have any guidance?

  

  



    
    

    
  
(In reply to Justin Lebar [:jlebar] from comment #55)
> > Hmm.  Why nsIDOMElement and not Element?
> 
> I really have no idea when to use which class, and reading through the code
> isn't enlightening me.  Do you have any guidance?

FWIW I did this, and it was a net addition of almost exactly 0 lines of code.  Lots of things changed, but nothing was simpler.  But I may be missing something particularly cool about using mozilla::dom::Element (other than not needing to QI to nsIContent).

  

  



    
    

    
  
> OOC, what do you mean?

The WebIDL bindings assume all objects are nsWrapperCache and hence that all objects can chain up to a unique Window (because you want to make sure you only create one wrapper per object).  Which is a huge annoyance....  We're still thinking about how to relax those restrictions.

> Making the event not cancelable and adding a method on the detail sounds good to me.

Excellent.

> The default behavior, if you didn't call the method, would still be to block the popup
> (or not show the alert), right?

Yes.  That behavior totally makes sense.

> I don't understand the question

In TabChild::BrowserFrameProvideWindow we have:

>+  if (!*aWindowIsNew) {
>+    PBrowserChild::Send__delete__(newChild);
>+    return NS_ERROR_ABORT;
>+  }

why is that the right thing to test?  In particular, could an existing window not have been found based on aName?  I just don't know the ipc setup there well enough to tell...

> you can't QI it

do_QueryObject()

> you can't AddRef it

NS_ADDREF should work.  As should various nsRefPtr things like forget() and whatnot.  I'd love to see what your code looked like when the addref didn't work.

> Do you have any guidance

My usual guidance would be to avoid nsIDOM* like the plague.  ;)  The code may have cruft left over from when we didn't have nsINode and Element classes, but it's just that: cruft.

  

  



    
    

    
  
> But I may be missing something particularly cool about using mozilla::dom::Element

Well, better performance and the hope that we can get rid of nsIDOMElement some day.

  

  



    
    

    
  
Comment on attachment 631527 [details] [diff] [review]
Bug 742944 - Part 3: Handle window.open in <iframe mozbrowser>.

In general, keep in mind that the "mozbrowser" embedding is going to
be *the* cross-process Gecko going forward.  Treat the old XUL stuff
here as legacy.

>diff --git a/content/base/src/nsFrameLoader.h b/content/base/src/nsFrameLoader.h

>+  void SetRemoteBrowser(nsITabParent* aTabParent);
>+

Document this.


>diff --git a/dom/ipc/PBrowser.ipdl b/dom/ipc/PBrowser.ipdl

>+    /**
>+     * window.open from inside <iframe mozbrowser> is special.  When the child
>+     * process calls window.open, it creates a new PBrowser (in its own
>+     * process), then calls BrowserFrameOpenWindow on it.
>+     *
>+     * The parent process gets a chance to accept or reject the window.open
>+     * call, and windowOpened is set to true if we ended up going through with
>+     * the window.open.
>+     *
>+     * @param opener the PBrowser whose content called window.open.
>+     */
>+    sync BrowserFrameOpenWindow(PBrowser opener, nsString aURL,
>+                                nsString aName, nsString aFeatures)
>+      returns (bool windowOpened);

I would just call this |OpenWindow()|.  This is the implementation we
really want for |CreateWindow()|, which is going to die soon.
Similarly, I would drop the qualification about <iframe mozbrowser>,
since what you describe is pretty much how window.open is supposed to
work anyway.

>diff --git a/dom/ipc/PContent.ipdl b/dom/ipc/PContent.ipdl

>+both:
>+    async PBrowser(PRUint32 chromeFlags);

Please document why this is |both|.

>diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp

>+        !(aChromeFlags & (nsIWebBrowserChrome::CHROME_MODAL |
>+                          nsIWebBrowserChrome::CHROME_OPENAS_DIALOG |
>+                          nsIWebBrowserChrome::CHROME_OPENAS_CHROME))) {

Note to self: we need to block
nsIWebBrowserChrome::CHROME_OPENAS_CHROME from content processes! o_O
Jeez.

>+nsresult
>+TabChild::BrowserFrameProvideWindow(nsIDOMWindow* aOpener,
>+                                    nsIURI* aURI,
>+                                    const nsAString& aName,
>+                                    const nsACString& aFeatures,
>+                                    bool* aWindowIsNew,
>+                                    nsIDOMWindow** aReturn)
>+{

>+  nsRefPtr<TabChild> newChild = static_cast<TabChild*>(
>+    ContentChild::GetSingleton()->SendPBrowserConstructor(0));
>+

  Manager()->SendPBrowserConstructor(0);

is simpler and allows you to drop the ContentChild.h include above.

>+  newChild->DoFakeShow();
>+

Per IRC chat, this exists to force creation of the opened window's
actual DOMWindow, which is a little counterintuitive.  Please
document.

The problem here is that Show() also initializes some graphics state
that will fail if the TabParent's nsFrameLoader is disconnected from
the frame tree (can't find a widget).  That's the reason that we only
initialize all that stuff when the parent side asks.

In this case though, you /might/ be ok, because you're creating an
iframe nested in something that should have a frame tree.  Hopefully
it'll be able to find a widget.  We'll see.  If this is an issue, the
content process will die, so we'll know.

We really need to decouple creating the DOMWindow from initializing
gfx.  We can totally rework this code when xul-fennec goes away
without risking regression, so putting that off until then is OK.  I
would be happy if you filed a followup and "FIXME/bug XXX"'d this
hack.

>diff --git a/dom/ipc/TabChild.h b/dom/ipc/TabChild.h

>+    // Call RecvShow(nsIntSize(0, 0)) and block future calls to RecvShow().
>+    void DoFakeShow();
>+
>+    nsresult
>+    BrowserFrameProvideWindow(nsIDOMWindow* aOpener,
>+                              nsIURI* aURI,
>+                              const nsAString& aName,
>+                              const nsACString& aFeatures,
>+                              bool* aWindowIsNew,
>+                              nsIDOMWindow** aReturn);
>+

Same thing here: s/BrowserFrame//.

This looks quite nice to me, from my narrow view of things.  r=me with
above.

  

  



        Attachment #631527 -
        Flags: review?(jones.chris.g) → review+

    
    

    
  
> >+    sync BrowserFrameOpenWindow(PBrowser opener, nsString aURL,
> >+                                nsString aName, nsString aFeatures)
> >+      returns (bool windowOpened);
>
> I would just call this |OpenWindow()|.  This is the implementation we
> really want for |CreateWindow()|,

This does the whole dance of creating an <iframe mozbrowser> in the parent, triggering a mozbrowseropenwindow event on the opener iframe, giving the embedder a chance to cancel that event... I don't see how this is what you'd want for a general-purpose CreateWindow call.

  

  



    
    

    
  
That is fully general.  What else would we want?

  

  



    
    

    
  
(In reply to Chris Jones [:cjones] [:warhammer] from comment #61)
> That is fully general.  What else would we want?

In your view, every window we ever open in a multi-process world should go through <iframe mozbrowser>?  We should never open a new top-level chrome window?  What about desktop e10s?

The current solution is "fully general" only if the whole world writes browsers using the Browser API.

  

  



    
    

    
  
Content processes have no reason to open chrome windows.  I will officially not comment on the rest of what you say ;).

  

  



    
    

    
  
> Content processes have no reason to open chrome windows.

So suppose we had desktop e10s.  Content running inside an OOP tab calls window.open.  We are not supposed to open a chrome window?  What are we supposed to do?

I'd missed this originally:

> In general, keep in mind that the "mozbrowser" embedding is going to
> be *the* cross-process Gecko going forward.  Treat the old XUL stuff
> here as legacy.

I think this is more than a bit of a stretch.  I don't think the powers that be have signed off on the idea of an "HTML browser" with no chrome JS, outside B2G.  Indeed, I haven't bought into this idea entirely, and I'm doing the work.

It sounds like you think that "throwing away XUL Fennec" also means "throwing away multiprocess XUL Firefox".  I'd prefer not to use this patch to push that agenda, if that's OK with you.

  

  



    
    

    
  
Let's land these patches for b2g and not spit into the wind.  Sorry for opening a can of worms.  It's closed again.

  

  



    
    

    
  
>> you can't AddRef it
>
> NS_ADDREF should work.  As should various nsRefPtr things like forget() and whatnot.  I'd love to 
> see what your code looked like when the addref didn't work.

  nsRefPtr<nsGenericHTMLFrameElement> popupFrameElement =
    static_cast<nsGenericHTMLFrameElement*>(
      NS_NewHTMLIFrameElement(nodeInfo.forget(), mozilla::dom::NOT_FROM_PARSER));

> dist/include/nsAutoPtr.h:898:13: error: request for member ‘AddRef’ is ambiguous
> dist/include/nsISupportsBase.h:63:60: error: candidates are: virtual nsrefcnt nsISupports::AddRef()
> dist/include/nsISupportsBase.h:63:60: error:                 virtual nsrefcnt nsISupports::AddRef()
> ../src/content/base/src/nsGenericElement.h:217:181: error:                 virtual nsrefcnt nsGenericElement::AddRef()
> dist/include/nsAutoPtr.h: In destructor ‘nsRefPtr<T>::~nsRefPtr() [with T = nsGenericHTMLFrameElement]’:

Note that nsGenericHTMLFrameElement is not a concrete class; I think that's why I'm seeing this.

  

  



    
    

    
  
> Note that nsGenericHTMLFrameElement is not a concrete class; I think that's why I'm seeing this.

...but I can't use nsHTMLIFrameElement because that's not declared in a header file.  (I could move it to one...)

  

  



    
    

    
  
Oh, right.  That makes sense.

Just move nsHTMLIFrameElement to a header and use that, please.

  

  



    
    

    
  
>> The default behavior, if you didn't call the method, would still be to block the popup
>> (or not show the alert), right?
> 
> Yes.  That behavior totally makes sense.

On second thought, how about we make this completely implicit?  If you add the iframe to the DOM somewhere, you haven't blocked the popup.  Otherwise, you have.

(It's already the case that you have to add the iframe to some document's DOM in order to allow the popup.)

  

  



    
    

    
  
> On second thought, how about we make this completely implicit? 

If we can do that, sure.

  

  



    
    

    
  
> In TabChild::BrowserFrameProvideWindow we have:
> 
> >+  if (!*aWindowIsNew) {
> >+    PBrowserChild::Send__delete__(newChild);
> >+    return NS_ERROR_ABORT;
> >+  }
> 
> why is that the right thing to test?  In particular, could an existing window not have been found 
> based on aName?  I just don't know the ipc setup there well enough to tell...

Per TabParent::RecvBrowserFrameOpenWindow, aWindowIsNew is the return value of BrowserElementParent::OpenWindowOOP(), which returns true iff we create a new <iframe mozbrowser>.  So I *think* this is is right -- the handling of named windows has to happen elsewhere.

(I added a small test for named windows, but I suspect there are edge cases which behave incorrectly.  In particular, I think if you have two mozapp iframes for different apps, and app A opens a new window with name N, and app B tries to open a new window with name N, we may try to re-use A's window for B, which would of course be wrong.  But this is kind of tricky, and enough people are waiting on this work that I'd like to investigate this as a follow-up.)

  

  



    
  
Blocks: 763602

    
    

    
  
> So I *think* this is is right -- the handling of named windows has to happen elsewhere.

Ok, good. 

Agreed on fixing named windows elsewhere.  Even the single-process handling in Gecko needs fixing, imo.

  

  



    
    

    
  


  

  

  



        Attachment #631980 -
        Flags: review?(bzbarsky)

    
    

    
  


  

  

  



        Attachment #631982 -
        Flags: review?(bzbarsky)

    
  

        Attachment #631527 -
        Attachment is obsolete: true

    
  

        Attachment #631980 -
        Attachment description: Move nsHTMLIFrameElement to a header file. → Part 3: Move nsHTMLIFrameElement to a header file.

    
  

        Attachment #631528 -
        Attachment description: Bug 742944 - Part 4: <iframe mozbrowser> window.open tests. → Bug 742944 - Part 5: <iframe mozbrowser> window.open tests.

    
    

    
  
Comment on attachment 631980 [details] [diff] [review]
Part 3: Move nsHTMLIFrameElement to a header file.

r=me

  

  



        Attachment #631980 -
        Flags: review?(bzbarsky) → review+

    
    

    
  
Comment on attachment 631982 [details] [diff] [review]
Part 4: Handle window.open in <iframe mozbrowser>.

> +++ b/content/html/content/src/Makefile.in.orig

Please don't.

>+++ b/dom/browser-element/BrowserElementParent.cpp
>+DispatchOpenWindowEvent(Element* aOpenerFrameElement,

>+  nsCOMPtr<nsIDOMElement> aPopupFrameDOMElement =
>+    do_QueryInterface(aPopupFrameElement);

  nsIDOMNode* popuFrameDOMNode = aPopupFrameElement->AsDOMNode().

>+BrowserElementParent::OpenWindowOOP(mozilla::dom::TabParent* aOpenerTabParent,

>+  nsRefPtr<Element> openerFrameElement = openerFrameNode->AsElement();

This could just be an Element*, I think.

Note that you can QI to Element directly, by the way, if that's all you're trying to do here.

>+++ b/dom/browser-element/BrowserElementParent.h

The comments still talk about calling preventDefault, which now does absolutely nothing.  Should just remove that part of the comments.

r=me with those nits.

  

  



        Attachment #631982 -
        Flags: review?(bzbarsky) → review+

    
    

    
  


  
I was getting a 
|[Exception... "Failure arg 0 [nsIDOMCustomEvent.detail]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: http://system.segonzac.info/js/attention_screen.js :: as_open :: line 46"
  data: no]|
on the device only.

Thanks again to :vingtetun for debugging this!

  

  



    
    

    
  
> Small fix: missing interface in b2g package

I expect I'll have to make this change elsewhere as well; I'll check it out before I push.

  

  



    
    

    
  
(With the package-manifest fixes): https://tbpl.mozilla.org/?tree=Try&rev=1d9600438d97

  

  



    
    

    
  
Hm, Windows link red:

/e/builds/moz2_slave/try-w32-dbg/build/obj-firefox/_virtualenv/Scripts/python.exe /e/builds/moz2_slave/try-w32-dbg/build/config/pythonpath.py -I../../config /e/builds/moz2_slave/try-w32-dbg/build/config/expandlibs_exec.py --depend .deps/xul.pp --target xul.dll --uselist -- link -NOLOGO -DLL -OUT:xul.dll -PDB:xul.pdb -SUBSYSTEM:WINDOWS -MACHINE:X86  dlldeps-xul.obj nsStaticXULComponents.obj nsDllMain.obj nsGFXDeps.obj dlldeps-zlib.obj nsUnicharUtils.obj nsBidiUtils.obj nsSpecialCasingData.obj nsUnicodeProperties.obj nsRDFResource.obj   ./module.res -LARGEADDRESSAWARE -NXCOMPAT -DYNAMICBASE -SAFESEH  -DEBUG -DEBUGTYPE:CV    ../../toolkit/xre/xulapp_s.lib  ../../staticlib/components/necko.lib ../../staticlib/components/uconv.lib ../../staticlib/components/i18n.lib ../../staticlib/components/chardet.lib ../../staticlib/components/jar50.lib ../../staticlib/components/startupcache.lib ../../staticlib/components/pref.lib ../../staticlib/components/htmlpars.lib ../../staticlib/components/imglib2.lib ../../staticlib/components/gkgfx.lib ../../staticlib/components/gklayout.lib ../../staticlib/components/docshell.lib ../../staticlib/components/embedcomponents.lib ../../staticlib/components/webbrwsr.lib ../../staticlib/components/nsappshell.lib ../../staticlib/components/txmgr.lib ../../staticlib/components/commandlines.lib ../../staticlib/components/toolkitcomps.lib ../../staticlib/components/pipboot.lib ../../staticlib/components/pipnss.lib ../../staticlib/components/appcomps.lib ../../staticlib/components/jsreflect.lib ../../staticlib/components/composer.lib ../../staticlib/components/telemetry.lib ../../staticlib/components/jsinspector.lib ../../staticlib/components/jsdebugger.lib ../../staticlib/components/storagecomps.lib ../../staticlib/components/rdf.lib ../../staticlib/components/windowds.lib ../../staticlib/components/jsctypes.lib ../../staticlib/components/jsperf.lib ../../staticlib/components/gkplugin.lib ../../staticlib/components/windowsproxy.lib ../../staticlib/components/jsd.lib ../../staticlib/components/autoconfig.lib ../../staticlib/components/auth.lib ../../staticlib/components/cookie.lib ../../staticlib/components/permissions.lib ../../staticlib/components/universalchardet.lib ../../staticlib/components/places.lib ../../staticlib/components/tkautocomplete.lib ../../staticlib/components/satchel.lib ../../staticlib/components/pippki.lib ../../staticlib/components/imgicon.lib ../../staticlib/components/profiler.lib ../../staticlib/components/widget_windows.lib ../../staticlib/components/accessibility.lib ../../staticlib/components/spellchecker.lib ../../staticlib/components/zipwriter.lib ../../staticlib/components/services-crypto.lib ../../staticlib/components/gkdebug.lib ../../staticlib/jsipc_s.lib ../../staticlib/domipc_s.lib ../../staticlib/domplugins_s.lib ../../staticlib/mozipc_s.lib ../../staticlib/mozipdlgen_s.lib ../../staticlib/ipcshell_s.lib ../../staticlib/gfxipc_s.lib ../../staticlib/hal_s.lib ../../staticlib/dombindings_s.lib ../../staticlib/xpcom_core.lib ../../staticlib/ucvutil_s.lib ../../staticlib/chromium_s.lib ../../staticlib/snappy_s.lib ../../staticlib/thebes.lib ../../staticlib/gl.lib ../../staticlib/ycbcr.lib   e:/builds/moz2_slave/try-w32-dbg/build/obj-firefox/dist/lib/mozjs.lib e:/builds/moz2_slave/try-w32-dbg/build/obj-firefox/dist/lib/crmf.lib         e:/builds/moz2_slave/try-w32-dbg/build/obj-firefox/dist/lib/smime3.lib         e:/builds/moz2_slave/try-w32-dbg/build/obj-firefox/dist/lib/ssl3.lib         e:/builds/moz2_slave/try-w32-dbg/build/obj-firefox/dist/lib/nss3.lib         e:/builds/moz2_slave/try-w32-dbg/build/obj-firefox/dist/lib/nssutil3.lib   ../../dist/lib/mozsqlite3.lib  ../../modules/zlib/src/mozz.lib ../../dist/lib/gkmedias.lib    e:/builds/moz2_slave/try-w32-dbg/build/obj-firefox/dist/lib/nspr4.lib e:/builds/moz2_slave/try-w32-dbg/build/obj-firefox/dist/lib/plc4.lib e:/builds/moz2_slave/try-w32-dbg/build/obj-firefox/dist/lib/plds4.lib  ../../dist/lib/mozalloc.lib -DELAYLOAD:psapi.dll -DELAYLOAD:dbghelp.dll -DELAYLOAD:rasapi32.dll -DELAYLOAD:rasdlg.dll -DELAYLOAD:comdlg32.dll -DELAYLOAD:winspool.drv -DELAYLOAD:secur32.dll  -DELAYLOAD:oleacc.dll e:/builds/moz2_slave/try-w32-dbg/build/obj-firefox/dist/lib/mozglue.lib kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib shell32.lib ole32.lib version.lib winspool.lib comdlg32.lib imm32.lib msimg32.lib shlwapi.lib psapi.lib ws2_32.lib dbghelp.lib rasapi32.lib rasdlg.lib iphlpapi.lib uxtheme.lib setupapi.lib secur32.lib sensorsapi.lib portabledeviceguids.lib windowscodecs.lib wininet.lib oleacc.lib delayimp.lib  usp10.lib oleaut32.lib   
   Creating library xul.lib and object xul.exp
LINK : warning LNK4098: defaultlib 'LIBCMTD' conflicts with use of other libs; use /NODEFAULTLIB:library
BrowserElementParent.obj : error LNK2019: unresolved external symbol "public: static unsigned int __cdecl nsEventDispatcher::CreateEventW(class nsPresContext *,class nsEvent *,class nsAString_internal const &,class nsIDOMEvent * *)" (?CreateEventW@nsEventDispatcher@@SAIPAVnsPresContext@@PAVnsEvent@@ABVnsAString_internal@@PAPAVnsIDOMEvent@@@Z) referenced in function "bool __cdecl `anonymous namespace'::DispatchOpenWindowEvent(class mozilla::dom::Element *,class mozilla::dom::Element *,class nsAString_internal const &,class nsAString_internal const &,class nsAString_internal const &)" (?DispatchOpenWindowEvent@?A0x37bc5f95@@YA_NPAVElement@dom@mozilla@@0ABVnsAString_internal@@11@Z)
xul.dll : fatal error LNK1120: 1 unresolved externals

  

  



    
    

    
  
(In reply to Justin Lebar [:jlebar] from comment #78)
> > Small fix: missing interface in b2g package
> 
> I expect I'll have to make this change elsewhere as well; I'll check it out
> before I push.

http://mxr.mozilla.org/mozilla-central/find?text=&string=package-manifest.in is probably the list of files you need to change.

  

  



    
    

    
  
The Windows build error is because somehow windows.h is being included, and that header #defines CreateEvent to CreateEventW.  Awesome.

  

  



    
    

    
  
Comment on attachment 631526 [details] [diff] [review]
Bug 742944 - Part 2: Allow getInterface'ing an nsGlobalWindow to an nsIDocShell.

Review of attachment 631526 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsGlobalWindow.cpp
@@ +8304,5 @@
>    }
> +  else if (aIID.Equals(NS_GET_IID(nsIDocShell))) {
> +    FORWARD_TO_OUTER(GetInterface, (aIID, aSink), NS_ERROR_NOT_INITIALIZED);
> +
> +    nsCOMPtr<nsIDocShell> docShell = do_QueryInterface(mDocShell);

No need to QI here

  

  



    
    

    
  
(In reply to :Ms2ger from comment #83)
> No need to QI here

Yes, bz noticed that in comment 48.

  

  



    
    

    
  
> because somehow windows.h is being included

I blame IPC.

Can you #include qsWinUndefs.h here?  ;)

  

  



    
  
Depends on: 764718

    
  
Depends on: 764414

    
  
Depends on: 766207

    
  
No longer blocks: 766481
No longer blocks: 744451

    
  
Depends on: 768561

    
  
Depends on: 769182

    
  
No longer blocks: 766873
Depends on: 766873

    
    

    
  
Documentation available here:
https://developer.mozilla.org/en-US/docs/Web/Reference/Events/mozbrowseropenwindow

  

  


Keywords: dev-doc-needed → dev-doc-complete

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: