403005 - Popup window briefly pops up, then closes immediately

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Martijn Wargers (dead)]

From bug 390833, comment 11:
"
I found another bug. You can no longer pop up the sub windows into a separate
firefox window. When you click on the button in the top right corner, Firefox
briefly pops up a window and then immediately closes it again.
"

To reproduce:
- Log in with one of the accounts.
- Click on the "open in new window" icon in the "window" at the right, at the top right.

This regressed between 2007-07-26 and 2007-07-27:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-07-26+04&maxdate=2007-07-27+06&cvsroot=%2Fcvsroot
I guess a regression from bug 194404, then.

Comment 1

[Martijn Wargers (dead)]

Attachment: this is the code from the popup window

I haven't looked at making a testcase yet, but this is the code of the popup window.

Note this: var callback = window.opener._fetchCb(window);

the _fetchCb function is in the parent window in meebo_v34_3.js and is this:
window._fetchCb=function(pwin){

  function check(id,win) {
    if(window._popOutCbs[id][0]===win) {
      var cb=window._popOutCbs[id][1];
      delete window._popOutCbs[id];
      if(win[id]){delete win[id];}
      return cb;
    } else{return false;}
  }

  for(var id in window._popOutCbs) {
    var cb;
    if(pwin.id&&(cb=check(pwin.id,pwin))){return cb;}
    if((cb=check(id,pwin))){return cb;}
  }
  return function(){ pwin.close(); };
};

Comment 2

[Johnny Stenback (:jst)]

It looks like we probably end up in the last bit of that function and we return "function(){ pwin.close(); };", and looking early on in that function I see:

    if(window._popOutCbs[id][0]===win) {
...
      return cb;

That triple equals makes me believe that this is probably a regression from mrbkap's cross origin wrapper changes (bug 367911, landed in the stated regression range as well).

Brendan/Blake, if I'm right, I don't see a way to fix that w/o adding a new JSClass hook through which one can override ===, much like the existing equality hook. Is that something we could squeeze in for 1.9?

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

So, XOWs do a nontrivial amount of work trying to try to avoid ever having two XOWs for the same object in the same scope. Where does 'win' come from?

Comment 4

[Johnny Stenback (:jst)]

The popup window calls _fetchCb on its opener, passing in the popup window ("window"), _fetchCb calls check(pwin.id,pwin) where pwin is the window argument that the popup passed in. So there's two different scopes (opener and popup), both of which have a reference to the popup window, and we end up doing a === compare on the openers XOW for the popup window, and the popup windows own XOW for its window.

I haven't verified any of this in a debugger, so I could be wrong, but it definitely seems like a likely possibility to me.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Yeah, this is what I get for playing fast and loose with same-origin stuff. There's no function to rewrap the 2nd scope's window. This bug needs more thought.

Comment 6

[Johnny Stenback (:jst)]

Sounds like we need to figure that out before too long...

Comment 7

[Johnny Stenback (:jst)]

Blake, have you had any time to think more about this one yet? I mentioned this to Brendan the other day and seems like we need to push wrapper creation down into the JS engine, especially in this case.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Fixed by the checkin for bug 397855.

Comment 9

[Johnny Stenback (:jst)]

The fix for bug 397855 didn't really solve the underlying problem here did it? Aren't we leaking XOW's across scopes here, and aren't we not supposed to do that? Reopening just to make sure that issue doesn't get lost, assuming it really is an issue.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

So, it doesn't actually matter if XOWs leak into another scope that happens to be same-origin. If this was a XOW leaking cross-origin, that would be a bad bug indeed. I don't think we have to fix this particular form of leakage -- note that it's impossible for this particular form of leakage to happen cross-origin because all functions that are exposed cross origin *must* be implemented in C++ and therefore not implementable by untrusted script.

Perhaps this should be WONTFIX instead? I'd argue that the symptoms are fixed and the underlying cause isn't worth fixing (unless I've missed something).

Comment 11

[Johnny Stenback (:jst)]

Fair enough, just wanted to be sure. Marking this FIXED (so that this shows up as a fixed beta blocker) as the symptoms are gone and the reason and the remaining issue is understood.